Enable the Terminal to tell ConPTY who the owner is #12526

zadjii-msft · 2022-02-18T15:53:32Z

Window shenanigans, part the first:

This PR enables terminals to tell ConPTY what the owning window for the pseudo window should be. This allows thigs like MessageBoxes created by console applications to work. It also enables console apps to use GetAncestor(GetConsoleWindow(), GA_ROOT) to get directly at the HWND of the Terminal (but don't please).

This is tested with our internal partners and seems to work for their scenario.

See #2988, #12799, #12515, #12570.

PR Checklist

This is 1/3 of Other applications open behind the Terminal window - (ConPTY window handle z-order shenanigans with GetConsoleWindow) #2988.
I work here

TODOS:

Clean up dead code, scratch, etc.
Especially make sure that the reparenting flow makes sense post-startup. (for tear-out, Scenario: Windows Terminal 2.0 Process Model Improvements #5000. Won't be able to test now, but should work then)
Come up with a total list of scenarios where the z ordering is messed up
Sync with auth folks to see if this worked
merge with Propagate show/hide window calls against the ConPTY pseudo window to the Terminal #12515 work
tests?

…y with the actual terminal hwnd too

…owner

…it's not validated with WAM

…owner

zadjii-msft · 2022-03-31T20:33:16Z

Unmarking as draft. Ready for review. IMO, the combo of this and #12799 should be enough to close #2988, and then we can use #12570 for the last prototype bits.

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalConnection/ConptyConnection.h

src/host/outputStream.cpp

miniksa · 2022-04-04T21:59:44Z

src/interactivity/base/InteractivityFactory.cpp

+                // When merging with #12515, we're going to need to adjust these styles.
+                //


Yeah watch out for this.

src/tools/scratch/Scratch.vcxproj

src/winconpty/winconpty.h

DHowett · 2022-04-05T18:18:50Z

@msftbot make sure @DHowett signs off

ghost · 2022-04-05T18:18:56Z

Hello @DHowett!

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:

I'll only merge this pull request if it's approved by @DHowett

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 · 2022-04-05T18:19:19Z

Nits: rework the PR description -- we should be careful about merging things that say "prototype" and then calling them stable and done :)

lhecker

I generally wish the uint64_t would get converted to a HWND as early as possible (if possible). Otherwise this looks good to me.

src/host/PtySignalInputThread.cpp

lhecker · 2022-04-11T21:31:22Z

src/host/PtySignalInputThread.hpp

+        struct SetParentData
+        {
+            uint64_t handle;
+        };


Why's this a struct? Will there be more members in the future?
Why is handle not a HWND?

Meh, they're all structs? I thought it was a nice abstraction to indicate "this is the packet of data that's sent". I don't expect to add more members in the future, but who knows.

As far as the uint64_t vs HWND - IIRC HWND is technically just a void*, so I wanted to be explicit with the size of data that's getting passed on the wire here. It's the same idea as like, every time we use a uint64_t for a HWND in any WinRT things.

I think transmitting it as a unintptr_t would've been more consistent with our other Win32 APIs (which this API is closest to IMO).

However, this is a wire format. 32-bit VSCode might be using 64-bit conhost (in fact: it is using 64-bit conhost). uintptr_t changes size. uint64_t does not.

src/cascadia/TerminalApp/AppLogic.cpp

DHowett · 2022-04-12T21:18:08Z

src/cascadia/TerminalConnection/ConptyConnection.idl

@@ -13,6 +13,7 @@ namespace Microsoft.Terminal.TerminalConnection
        Guid Guid { get; };
        String Commandline { get; };
        void ClearBuffer();
+        void ReparentWindow(UInt64 newParent);


mind filing a followup to make this and the following ones about focus part of a new interface? ITerminalConnectionWindowHosting or something

DHowett · 2022-04-12T21:19:16Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -262,6 +262,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
            const auto height = vp.Height();
            _connection.Resize(height, width);

+            if (_OwningHwnd != 0)
+            {
+                if (auto conpty{ _connection.try_as<TerminalConnection::ConptyConnection>() })


Don't love this coupling, thus the interface :)

DHowett · 2022-04-12T21:22:31Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -2794,4 +2794,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        }
    }

+    void TermControl::OwningHwnd(uint64_t owner)


Nit: vaguely prefer the HostingWindow parlance; we can take that as a followup (look at TSE's IHostedInWindow)

DHowett · 2022-04-12T21:24:06Z

src/host/outputStream.cpp

+    // If the window hasn't been created yet, by some other call to
+    // LocatePseudoWindow, then this will also initialize the owner of the
+    // window.
+    if (const auto psuedoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast<HWND>(handle)) })


psuedo -> pseudo

DHowett · 2022-04-12T21:25:58Z

src/host/outputStream.cpp

@@ -892,3 +892,17 @@ void ConhostInternalGetSet::UpdateSoftFont(const gsl::span<const uint16_t> bitPa
        pRender->UpdateSoftFont(bitPattern, cellSize, centeringHint);
    }
 }
+
+void ConhostInternalGetSet::ReparentWindow(const uint64_t handle)


interesting -- why did we keep it a uint64_t this deep in the stack? We should decap it as early as we can

DHowett · 2022-04-12T21:38:41Z

src/host/outputStream.cpp

+    // window.
+    if (const auto psuedoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast<HWND>(handle)) })
+    {
+        LOG_LAST_ERROR_IF_NULL(::SetParent(psuedoHwnd, reinterpret_cast<HWND>(handle)));


Just making sure . . . so this sets the OWNER, not the PARENT, for the same reason as below (no CHILDWINDOW)

exactly. There's no ::SetOwner, SetParent sets the owner/parent based on if WS_CHILD is set.

DHowett · 2022-04-12T22:42:40Z

I'm not bullish on the spelling fix, but I am curious about decapping the uint64 early so that we don't pass it around as an unguarded integer for too long.

DHowett · 2022-04-12T22:43:24Z

TODOS:

Clean up dead code, scratch, etc.
Especially make sure that the reparenting flow makes sense
post-startup. (for tear-out, Scenario: Windows Terminal 2.0 Process Model Improvements #5000. Won't be able to test now, but
should work then)
Come up with a total list of scenarios where the z ordering is
messed up
Sync with auth folks to see if this worked
merge with Propagate show/hide window calls against the ConPTY pseudo window to the Terminal #12515 work
tests?

DHowett · 2022-04-12T22:44:35Z

If you need to resolve a merge conflict in a stacked PR, merge 26d67d9 into your branch. You can fetch it with git fetch origin 26d67d9

…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)

#### ⚠️ _Targets #12799_ ⚠️ This is an atomic bit of code that partners with #12799. It's separated as an individual PR to keep diffs more simple. This ensures that when a terminal tells ConPTY that it's focused, that ConPTY doesn't do the `ConsoleControl(CONSOLE_FOREGROUND` thing unless the terminal application is actually in the foreground. This prevents a trivial exploit whereby a `malicious.exe` could create a PTY, tell ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground. When the terminal tells us it's in the foreground, we're gonna look at the owner of the ConPTY window handle. If that owner has focus, then cool, this is allowed. Otherwise, we won't grant them the FG right. For this to work, the terminal just have already called `ReparentPseudoConsole`. * built on top of #12799 and #12526 * [x] Part of #2988 * [x] Tested manually.

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) ```

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

ghost · 2022-05-24T17:08:17Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 6 commits February 15, 2022 15:32

working on prototypes. I'm not crazy. You're crazy

30863d2

this is totally a hack but it worked better than expected

555042e

I think this plumbs all the way through. DPI is busted tho

31e9fef

fixed the message pump

cb2ad2b

okay there's a way to get the terminal handle with this. I want to tr…

b257581

…y with the actual terminal hwnd too

a consistent way of getting the console or terminal hwnd? inconceivable!

2009147

This comment was marked as duplicate.

Sign in to view

zadjii-msft marked this pull request as draft February 18, 2022 16:53

zadjii-msft added 4 commits February 18, 2022 10:55

spell, fix test mock

669b1e2

thanks for nothing VS

5d96691

Merge remote-tracking branch 'origin/main' into dev/migrie/f/z-order-…

e46e9a3

…owner

this was a test that Evan wanted me to try. seemed to work okay, but …

23b6fee

…it's not validated with WAM

This comment was marked as resolved.

Sign in to view

This was referenced Mar 29, 2022

Further refactor and simplify the ConGetSet API #12703

Merged

Allow windows created by console apps to appear above the Terminal #12799

Merged

zadjii-msft added 4 commits March 31, 2022 12:59

Merge remote-tracking branch 'origin/main' into dev/migrie/f/z-order-…

cfded0f

…owner

cleanup everywhere

cf65085

revert this guy

659d752

clean, finally

4edeec3

This comment was marked as resolved.

Sign in to view

zadjii-msft marked this pull request as ready for review March 31, 2022 20:31

spellcheck and auditmode, my nemis...ese? nemisises?

000cb83

miniksa approved these changes Apr 4, 2022

View reviewed changes

zadjii-msft added Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Windowing Window frame, quake mode, tearout Needs-Second It's a PR that needs another sign-off labels Apr 5, 2022

ghost requested review from carlos-zamora and DHowett April 5, 2022 18:16

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 5, 2022

zadjii-msft assigned DHowett Apr 6, 2022

zadjii-msft mentioned this pull request Apr 6, 2022

Propagate show/hide window calls against the ConPTY pseudo window to the Terminal #12515

Merged

5 tasks

Merge branch 'main' into dev/migrie/f/z-order-owner

0ba955d

zadjii-msft mentioned this pull request Apr 7, 2022

Other applications open behind the Terminal window - (ConPTY window handle z-order shenanigans with GetConsoleWindow) #2988

Closed

lhecker reviewed Apr 11, 2022

View reviewed changes

guess what, this wasn't needed, hooplah

5e54423

lhecker approved these changes Apr 12, 2022

View reviewed changes

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 12, 2022

DHowett reviewed Apr 12, 2022

View reviewed changes

Update src/cascadia/TerminalApp/AppLogic.cpp

a536bb8

DHowett merged commit 26d67d9 into main Apr 12, 2022

DHowett deleted the dev/migrie/f/z-order-owner branch April 12, 2022 22:44

This was referenced Apr 13, 2022

Ensure a terminal requesting FG rights actually has them #12899

Merged

Make sure to do the window owner thing for WPF too #12911

Open

This was referenced May 16, 2022

[1.14] ShowWindow(SW_MINIMIZE) doesn't work anymore #13066

Closed

Fix ShowWindow(GetConsoleWindow()) #13118

Merged

german-one mentioned this pull request Jun 19, 2023

not working on terminal 1.2+ german-one/termwnd#1

Closed

zadjii-msft mentioned this pull request Aug 2, 2023

Cannot type in new pane or tab until focus is manually activated with mouse click #13388

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the Terminal to tell ConPTY who the owner is #12526

Enable the Terminal to tell ConPTY who the owner is #12526

zadjii-msft commented Feb 18, 2022 •

edited

Loading

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as resolved.

zadjii-msft commented Mar 31, 2022

miniksa Apr 4, 2022

DHowett commented Apr 5, 2022

ghost commented Apr 5, 2022

DHowett commented Apr 5, 2022

lhecker left a comment

lhecker Apr 11, 2022

zadjii-msft Apr 12, 2022

lhecker Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

zadjii-msft Apr 12, 2022

DHowett commented Apr 12, 2022

DHowett commented Apr 12, 2022

DHowett commented Apr 12, 2022

ghost commented May 24, 2022

		// When merging with #12515, we're going to need to adjust these styles.
		//

Enable the Terminal to tell ConPTY who the owner is #12526

Enable the Terminal to tell ConPTY who the owner is #12526

Conversation

zadjii-msft commented Feb 18, 2022 • edited Loading

Window shenanigans, part the first:

PR Checklist

TODOS:

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as resolved.

zadjii-msft commented Mar 31, 2022

Choose a reason for hiding this comment

DHowett commented Apr 5, 2022

ghost commented Apr 5, 2022

DHowett commented Apr 5, 2022

lhecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Apr 12, 2022

DHowett commented Apr 12, 2022

TODOS:

DHowett commented Apr 12, 2022

ghost commented May 24, 2022

zadjii-msft commented Feb 18, 2022 •

edited

Loading