Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make subsequent windows open in the foreground #17368

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

flyingcat
Copy link
Contributor

@flyingcat flyingcat commented Jun 5, 2024

Sometimes subsequent WT windows open in the background behind other applications. This PR tries to fix it.

Refs #15895
Refs #15479

Mysterious bug (and annoying). There are even some discussions about happening to the first startup, not just subsequent ones. Sometimes the window may show up without animation too. So I don't think this is the final solution, but it did get solved on my computer, for now.

Validation Steps Performed

  1. Quit all WT windows if some.
  2. Open File Explorer, click "Open in Terminal" in context menu.
  3. Move the newly opened window and minimize it.
  4. Back to step 1 and repeat several times.
  5. All the windows should open in the foreground correctly (yet possibly without animation).

@zadjii-msft
Copy link
Member

You know, I never had a super good repro for this. That being said - this is what we all assumed the fix would be. So if this does work for you, then I'm guessing this was the fix after all!

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! We'll make sure to send out the first Canary build with this to the linked threads, to see if that sorts everyone else out too.

src/cascadia/Remoting/WindowManager.cpp Outdated Show resolved Hide resolved
Co-authored-by: Mike Griese <migrie@microsoft.com>
src/cascadia/Remoting/WindowManager.cpp Outdated Show resolved Hide resolved
src/cascadia/Remoting/WindowManager.cpp Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 5, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 6, 2024
@flyingcat
Copy link
Contributor Author

Figure it out. When close one of the minimized WT window from taskbar, then subsequent windows will show up without animation. Latest win10.

I believe that there is one or more problems deep down in the complex window initializing and win32-xaml architecture.

@flyingcat
Copy link
Contributor Author

Also, subsequent (peasant?) processes will call xaml::Hosting::WindowsXamlManager::InitializeForCurrentThread(), which is unnecessary. If we somehow cache the value of _app.Logic().IsolatedMode(), call to ReloadSettings() is unnecessary too. This will save about 100ms + 15ms on my computer.

@tusharsnx
Copy link
Contributor

tusharsnx commented Jun 6, 2024

I'm so glad that @flyingcat brought this up. I was on this ever since #17168, and I believe this PR fixes some missing focus cases:

  1. Launching new window/tabs using shell extension (right click menu of Explorer).
  2. Launching terminal by running wt.exe from the Run dailog.

You can see the similarty here, both will lead to an indirect launch of a new WindowsTerminal.exe window if it was running already. The WT process is a bg thread in both these cases so it won't receive window focus automatically by the system.


For the core team:

@zadjii-msft Just to reiterate on this:

You know, I never had a super good repro for this.

My simple repro is to:

  1. Confirm "New instance behavior" is set to "Create new window".
  2. Open WT,
  3. Drag and move the window to a different location (just so that windows sees the user has interacted with the window),
  4. Click on the desktop (so that the window loses focus = WT is now a background thread/process), and then
  5. Launch another WT window using explorer's right click menu or Run dailog.

Defterm needs a similar fix, and I believe that would solve all missing focus scenarios. When a new defterm request is received and there's a running instance of WT on the system, the window is created by a WT thread that doesn't satisfy any requirements listed for SetForegroundWindow(). The foreground privilege needs to be sent down from Conhost to openconsole.exe to the running instance of WindowsTerminal.exe because in that moment only Conhost was the new process that was created by the foreground application (Windows shell).

@DHowett
Copy link
Member

DHowett commented Jun 6, 2024

Defterm needs a similar fix, and I believe that would solve all missing focus scenarios. When a new defterm request is received and there's a running instance of WT on the system, the window is created by a WT thread that doesn't satisfy any requirements listed for SetForegroundWindow(). The foreground privilege needs to be sent down from Conhost to openconsole.exe to the running instance of WindowsTerminal.exe because in that moment only Conhost was the new process that was created by the foreground application (Windows shell)

So this one, at least, I did implement in Windows - plumbed all the way through stage 1 and stage 2 defterm handoff - and found that it made little difference. Conhost itself may not have foreground rights, technically, until it does some NTUSER magic that we haven't fully discovered. I can't remember which of the four calls in the chain failed.

@flyingcat
Copy link
Contributor Author

I just want this bug get fixed.

Oh, I realized it may sound like a boss ask someone to finish something with no executes (they often do). That's not what I mean. I saw some commits have co-authors, so I think it's the team members add changes before merging, never thought of this ping-pong way. Also I don't care I am a contributor or not.


If this or anything else get merged, can it back port to 1.20 branch? I really want to solve this problem, as it perfectly break my usage scenario.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks you for doing this!

@DHowett DHowett added this pull request to the merge queue Jun 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 10, 2024
@zadjii-msft zadjii-msft added this pull request to the merge queue Jun 10, 2024
Merged via the queue into microsoft:main with commit c52ba7d Jun 10, 2024
15 checks passed
DHowett pushed a commit that referenced this pull request Jun 24, 2024
Sometimes subsequent WT windows open in the background behind other
applications. This PR tries to fix it.

Refs #15895
Refs #15479

Mysterious bug (and annoying). There are even some discussions about
happening to the first startup, not just subsequent ones. Sometimes the
window may show up without animation too. So I don't think this is the
final solution, but it did get solved on my computer, for now.

## Validation Steps Performed
0. Quit all WT windows if some.
1. Open File Explorer, click "Open in Terminal" in context menu.
2. Move the newly opened window and minimize it.
3. Back to step 1 and repeat several times.
4. All the windows should open in the foreground correctly (yet possibly
without animation).

---------

Co-authored-by: Mike Griese <migrie@microsoft.com>
(cherry picked from commit c52ba7d)
Service-Card-Id: 92715122
Service-Version: 1.21
DHowett pushed a commit that referenced this pull request Jun 26, 2024
Sometimes subsequent WT windows open in the background behind other
applications. This PR tries to fix it.

Refs #15895
Refs #15479

Mysterious bug (and annoying). There are even some discussions about
happening to the first startup, not just subsequent ones. Sometimes the
window may show up without animation too. So I don't think this is the
final solution, but it did get solved on my computer, for now.

## Validation Steps Performed
0. Quit all WT windows if some.
1. Open File Explorer, click "Open in Terminal" in context menu.
2. Move the newly opened window and minimize it.
3. Back to step 1 and repeat several times.
4. All the windows should open in the foreground correctly (yet possibly
without animation).

---------

Co-authored-by: Mike Griese <migrie@microsoft.com>
(cherry picked from commit c52ba7d)
Service-Card-Id: 92715121
Service-Version: 1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants