windows-terminal-quake doesn't start both the terminal and itself. (exception thrown: process is null)

webbertakken commented 4 years ago

Hey @flyingpie, thanks a lot for creating this. I was so very happy to find this plugin. I'm sure I will be using this all the time like I did in Cmder.

I've downloaded v0.5 (also tried v0.4) and read the readme, but could not find how it is supposed to open both terminal and this plugin at the same time. I had hoped there would be some setting that allows the plugin to start with terminal automatically.

Expected behaviour

Option 1:

Open terminal executable
See that terminal + quake style plugin work
This clearly documented in the readme

Option 2;

Open windows-terminal-quake executable
See that terminal + quake style plugin work
This clearly documented in the readme

Actual behaviour

First scenario worked perfectly;

Open terminal
Added executable and the config to "C:\Users\(username)\"
Open windows-terminal-quake.exe
See that it works! awesome.

But now I'd like the plugin to automatically start;

Close terminal
Open terminal again
See that quake style no longer works (which essentially just means that nothing was "installed")

So I'm thinking perhaps I should just open terminal with the executable;

Close terminal
Open windows-terminal-quake.exe
See a non-maximised terminal window without the plugin enabled.
Open windows-terminal-quake.exe again with the window already open
See that now it works.

Considered solutions

I tried looking in the following places for clues how to make it work out of the box:

The readme of this repository
This plugins configuration
The configuration file from Terminal

Reason for starting both with 1 executable

General convenience
Opening it when clicking the terminal tab within an IDE will not work if there are two executable files to be called (I'm looking to replace Cmder now quake style is here for terminal)

Thanks again for your great work.

webbertakken commented 4 years ago

Looking at the code I think it is already supposed to perform what I described above as "Expected behaviour, option 2".

I tried debugging the program and noticed that for me the process is already ended.

flyingpie commented 4 years ago

@webbertakken Thank you for taking the time to look into this issue!

I'm a little confused as to what the exact scenario is. Do you want to start the plugin whenever the terminal starts, as opposed to starting the terminal through the plugin?

webbertakken commented 4 years ago

@flyingpie the two cases you're describing I tried to describe in Expected behaviour, option 1 and 2 (and neither works for me in practice). From my perspective it doesn't really matter much which of the two options works, as long as one of the two does.

I suspect the behaviour may be different between machines (i.e. the wrapper doesn't start on all machines because the process is null if it wasn't already running). The PR fixes this issue by making sure the "Windows Terminal" process is selected before wrapping it (which it didn't do before). The PR should not break any current behaviour and fixes the problem; Both the Terminal and the plugin launch when the plugin is launched.

Just to be completely unambiguous about this; on my machine it seems that windows terminal is started by a process that immediately ends. Even though that process ended, the terminal is running and works fine. However the terminal is not captured in the process variable, when means that null is selected as the process to wrap.

Please let me know if you need more information or if you would like to see any changes.

flyingpie commented 4 years ago

@webbertakken Thank you for the explanation, I wonder if there's indeed a difference in behavior between machines, since I can't seem to reproduce the issue.

I also get the "Wrapper was unable to start Windows Terminal" error consistently, I'm assuming you don't see this error at all?

Also, I'm not sure if I - on my machines - have the same behavior, where the WindowsTerminal process exits immediately after starting. It doesn't seem to be the case at least.

Could you tell me what version of Windows and the Windows Terminal you're using?

webbertakken commented 4 years ago

I also get the "Wrapper was unable to start Windows Terminal" error consistently, I'm assuming you don't see this error at all?

Exactly, I don't see that error at all. I now realise that the PR actually breaks current behaviour for your case, where you get the process consistently.

Could you tell me what version of Windows and the Windows Terminal you're using?

Windows Terminal Preview, Version: 1.2.2234.0 Windows 10, Version 1909, Build 18363.1016

What version are you using? I'll try to also reproduce your case.

webbertakken commented 4 years ago

@flyingpie assuming that we're both using one of the latest versions of terminal, do you think we should account for both cases?

I can update the PR with any necessary changes, if you have a suggested approach.

webbertakken commented 4 years ago

@flyingpie I know this doesn't happen in your case, but do you have any suggestions on how to solve this?

flyingpie commented 4 years ago

@webbertakken You mentioned that when you start the terminal app, it starts a process, that kicks off another process, then the first process exits. Is that correct?

If so, could you show me what the "intermediate" process is? I'm very confused by this, and why it would be different on my machine ™.

webbertakken commented 4 years ago

Correct.

Just to check if it's a one-of issue, I have just downloaded the latest (stable) version of terminal (from the store this time) after uninstalling the last version and downloaded the latest version from here too, but still observe the same behaviour.

I don't see why your OS or terminal would be different from mine either. The only thing I can think of is that my OS it's on top of Hyper-V but I do not think that would be related. So I guess I can't reliably answer your last question.

If so, could you show me what the "intermediate" process is?

Yes I'll see if I can find out some info about the signature of that process.

webbertakken commented 4 years ago

Observations:

The left hand side of the diff shows all accessible properties when the wt process is started from inside quake console whereas the right hand side shows the process information about the same process when it was already running (both cases: when started from a previous run of quake console or started as terminal standalone)

This looks to be a permission issue that blocks Toggler from getting the mainWindowHandle. I have found other people that were experiencing [similar] [issues].

I tried to work around this by:

calling process.Refresh() as suggested here without luck.
calling process.WaitForInputIdle as suggested in the official docs here.

reassinging process using

process = Process.GetProcessesByName("WindowsTerminal").First();

In the last case I acquired access to the process finally. The wrapper is assigned to the process main window, which at this point is not maximised. It creates the console slider either way. I recorded a quick video to illustrate this.

Solution

While typing all this out I tried the combination of the first and the last thing, which happened to work perfectly.

I'm expecting that solution should work for everyone, as all it does is remap the process to gain the proper permissions to access its properties.

I have updated https://github.com/flyingpie/windows-terminal-quake/pull/14 to reflect the full solution. But found that there is a race condition.

flyingpie commented 4 years ago

@webbertakken Thank you for working on this some more.

With "race condition", do you mean merge conflict? I'm seeing a lot of changes that sort of conflict with changes that have been done in the mean time.

I've taken the non-conflicting changes and put that into a build.

This is working perfectly fine on my machine, could you see what happens on yours?

Here is the change: https://github.com/flyingpie/windows-terminal-quake/pull/21/files Here is the release: https://github.com/flyingpie/windows-terminal-quake/releases/tag/v0.8

(Close was accidental btw).

webbertakken commented 4 years ago

Hey @flyingpie, and thank you for merging my PR!

When I said race condition I didn't mean a merge conflict. That post was made after the second commit last night. See this comment pointing out the logging of all properties.

But the third commit in that same PR was adding a proper fix. It wasn't showing any merge conflicts for me but perhaps it was about making process a static variable in my PR.

I have checked v0.8 pre-release. It works but I added 1 comment to that PR, because some of the logic shouldn't be necessary if the process is running initially.

By the way, the ticket was closed because i mentioned Fixes #13 in that PR. If you don't want it to auto close it you can remove the reference action from the bar on the right hand side.

Please let me know if you need anything and thanks again for making this awesome plugin!

webbertakken commented 4 years ago

Btw may I ask why you force pushed away my 3 commits that you merged into master? These fixes cost me quite a few hours to make and I'd appreciate it if you didn't just copy the changes into your own PR and merge it.

I have replayed the changes and fixed the merge conflict you mentioned in https://github.com/flyingpie/windows-terminal-quake/pull/22

flyingpie commented 4 years ago

@webbertakken The PR merge and force push fuckery is basically all accidental, since I'm not too familiar with multiple git remotes yet, sorry for that. I definitely intend to ultimately use your PR with your code, I was just trying to make a version on top of the changes I made in the mean time.

That being said, could you make it so that the process doesn't become a static field, but is pull from a single method, like in the PR I made?

webbertakken commented 4 years ago

@flyingpie fair enough, I understand.

I made these changes the way you mentioned in #22.

flyingpie commented 4 years ago

@webbertakken Thank you for the changes, looks very nice indeed!

Just to confirm, the code as per #22 is working on your machine without issue? Cause it works over here, so that would be nice :)

One comment, this here line:

Log.Logger.Warning($"Error: {ex.Message}\n{ex.StackTrace}");

Can be simplified to this:

Log.Logger.Warning(ex, $"Error: {ex.Message}");

The Serilog logging library will take care of adding the stacktrace. In the case of a JSON formatter (not too useful for this app), the stack trace gets put into its own structured property, which makes it way easier to parse using eg. logstash or some other tool.

webbertakken commented 4 years ago

Oh really cool! I updated the logging statement.

I have tested one more time to double check and everything works from my side.

flyingpie commented 4 years ago

@webbertakken Thank you! Just merged the PR, so now we should have a version that works for the both of us :)

https://github.com/flyingpie/windows-terminal-quake/releases/tag/v0.8

flyingpie / windows-terminal-quake