Propagate show/hide window calls against the ConPTY pseudo window to the Terminal #12515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
8 tasks
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
… the terminal to the pty host.
This comment was marked as resolved.
This comment was marked as resolved.
miniksa
commented
Feb 23, 2022
This comment was marked as duplicate.
This comment was marked as duplicate.
ghost
added
Area-Interaction
zadjii-msft
added a commit
that referenced
this pull request
Apr 15, 2022
zadjii-msft
added a commit
that referenced
this pull request
Apr 19, 2022
…12799) ## Window shenanigans, part the third: Hooks the Terminal's focus state up to the underlying ConPTY. This is LOAD BEARING for allowing windows created by console applications to bring themselves to the foreground. We're using the [FocusIn/FocusOut](https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-FocusIn_FocusOut) sequences to communicate to ConPTY when a control gains/loses focus. Theoretically, other terminals could do this as well. ## References #11682 tracks _real_ support for this sequence in Console & conpty. When we do that, we should consider even if a client application disables this mode, the Terminal & conpty should always request this from the hosting terminal (and just ignore internally to ConPTY). See also #12515, #12526, which are the other two parts of this effort. This was tested with all three merged together, and they worked beautifully for all our scenarios. They are kept separate for ease of review. ## PR Checklist * [x] This is prototype 3 for #2988 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This allows windows spawned by console processes to bring themselves to the foreground _when the console is focused_. (Historically, this is also called in the WndProc, when focus changes). Notably, before this, ConPTY was _never_ focused, so windows could never bring themselves to the foreground when run from a ConPTY console. We're not blanket granting the SetForeground right to all console apps when run in ConPTY. It's the responsibility of the hosting terminal emulator to always tell ConPTY when a particular instance is focused. ## Validation Steps Performed (gif below)
ghost
pushed a commit
that referenced
this pull request
Apr 20, 2022
Further builds on #12799. #12799 assumes that the connection is prepared to receive FocusIn/FocusOut events as input. For ConPTY we can be relatively sure of that, but that's not _technically_ correct. In the hypothetical world where the connection is not a ConPTY connection, then the other side might not be expecting those sequences. This remedies the issue by * ConPTY will always request focus event mode (from the terminal) when it starts up * when a client tries to disable focus events in conpty, conpty is gonna note that internally, but never transmit that to the hosting terminal, to leave the terminal in focus event mode. * `TerminalDispatch` and `ControlCore` are hooked up now to only send focus events when the Terminal is in focus event mode (which will be always for conpty) * At this point, it was like, 4LOC in `terminalInput.cpp` to add support for focus events to conhost as well. ## checklist * [x] closes #11682 * This combined with #12515 will finally close out #2988 as well, but we can do that manually. * [x] I work here * [ ] There aren't tests for this. There probably should be.
lhecker
reviewed
Apr 20, 2022
TBH I think we're just gonna commit it at this point. I haven't touched it. |
lhecker
approved these changes
Apr 27, 2022
|
@msftbot merge this in 4 hours |
ghost
added
the
AutoMerge
|
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
DHowett
removed
the
AutoMerge
ghost
merged commit
DHowett
pushed a commit
that referenced
this pull request
May 18, 2022
A bad merge, that actually revealed a horrible bug. There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great. This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post. * fixes #13066. * Tested with the script in that issue. * Window doesn't flash uncontrollably. * `gci | ogv` still works right * I work here. * Opening a new tab doesn't spontaneously cause the window to minimize * Restoring from minimized doesn't yeet focus to an invisible window * Opening a new tab doesn't yeet focus to an invisible window * There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD. There's actually more to this as well. Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad. `SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window. Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need ```c++ GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) ```
DHowett
pushed a commit
that referenced
this pull request
May 19, 2022
A bad merge, that actually revealed a horrible bug. There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great. This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post. * fixes #13066. * Tested with the script in that issue. * Window doesn't flash uncontrollably. * `gci | ogv` still works right * I work here. * Opening a new tab doesn't spontaneously cause the window to minimize * Restoring from minimized doesn't yeet focus to an invisible window * Opening a new tab doesn't yeet focus to an invisible window * There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD. There's actually more to this as well. Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad. `SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window. Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need ```c++ GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) ``` (cherry picked from commit 77215d9) Service-Card-Id: 82170678 Service-Version: 1.14
|
🎉 Handy links: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Propagate show/hide window calls against the ConPTY pseudo window to the Terminal
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
::ShowWindowcommands against the pseudo console "fake window" and observing the real terminal window state