Handle window resize event (CSI t, resize) by nukoseer · Pull Request #17721 · microsoft/terminal

nukoseer (nukoseer) · 2024-08-15T07:38:55Z

ResizeWindow event in TerminalApi is handled and bubbled to TerminalApi->ControlCore->TermControl->TerminalPage->AppHost. Resizing is accepted only if the window is not in fullscreen or quake mode, and has 1 tab and pane.

Relevant issues: #5094

nukoseer (nukoseer) · 2024-08-15T07:43:54Z

@microsoft-github-policy-service agree

Leonard Hecker (lhecker)

Nice PR! Thank you so much for working on this! 😻

I found just 1 potential issue (the lack of a weak-ref) and otherwise only a couple nits. We don't use a weak-ref elsewhere in the class either, so this isn't something I'd block over.

Leonard Hecker (lhecker) · 2024-08-15T13:47:49Z


+    winrt::fire_and_forget TerminalPage::_WindowSizeChanged(const IInspectable /*sender*/, const Microsoft::Terminal::Control::WindowSizeChangedEventArgs args)
+    {
+        co_await wil::resume_foreground(Dispatcher());


Since you already switched to the UI thread in TermControl you won't need to do it again here.

Oher functions in this file do it, because our event handling code hasn't been cleaned up in a while and so some functions switch to the UI thread here and some in TermControl. I think it's better to do it in TermControl, because that makes it safer and more consistent.

Leonard Hecker (lhecker) · 2024-08-15T13:51:21Z


+    winrt::fire_and_forget TermControl::_bubbleWindowSizeChanged(const IInspectable& /*sender*/, const Control::WindowSizeChangedEventArgs args)
+    {
+        co_await wil::resume_foreground(Dispatcher());


This will need a weak-ref to this so that we can safely check if the class instance went away while we switched to the UI thread. Like this (you can change the code style / variable names to whatever you prefer):

terminal/src/cascadia/TerminalControl/TermControl.cpp

Lines 1010 to 1012 in 1511d2c

auto weakThis{ get_weak() };

co_await wil::resume_foreground(Dispatcher());

if (auto control{ weakThis.get() })

Leonard Hecker (lhecker) · 2024-08-15T13:52:31Z

+    {
+        co_await wil::resume_foreground(Dispatcher());
+
+        til::size cellSize{ args.Width(), args.Height() };


I think it'd be easier to understand if this was named cellCount (we use cellSize for the size of a cell in pixels elsewhere).

Leonard Hecker (lhecker) · 2024-08-15T13:55:17Z

+    const auto islandWidth = Utils::ClampToShortMax(lrintf(static_cast<float>(newSize.width)), 1);
+    const auto islandHeight = Utils::ClampToShortMax(lrintf(static_cast<float>(newSize.height)), 1);


The float-cast + lrintf seems redundant. I think this could be just

const auto islandWidth = Utils::ClampToShortMax(newSize.width, 1); const auto islandHeight = Utils::ClampToShortMax(newSize.height, 1);

Leonard Hecker (lhecker) · 2024-08-15T13:59:45Z

You can fix the Code Health Check failure by running this in PowerShell in the root of the repository:

Import-Module .\tools\OpenConsole.psm1
Invoke-CodeFormat -IgnoreXaml

Don't worry about the AuditMode failure. It broke because we updated our CI's MSVC version. I'm going to fix that soon.

Emanuel Hoogeveen (ehoogeveen-medweb) · 2024-08-15T15:21:03Z

What happens if a size is requested that exceeds the "maximum" size of the window (maximum in quotes because I'm not sure exactly what the limit would be)? I don't know if strict bounds are necessary, but there might need to be some limit to avoid e.g. hardware-accelerated renderer crashes (IIRC GPUs generally can't handle surfaces larger than 32768 pixels in either dimension).

nukoseer (nukoseer) · 2024-08-15T16:48:35Z

Emanuel Hoogeveen (@ehoogeveen-medweb) At the end dimensions can't be larger than 32767 because there is clamping before calling SetWindowPos (https://github.com/microsoft/terminal/pull/17721/files#diff-d9af234d6dd21db9240dc3733d69471898a79f1c4386cb3c48ac45ab7158cd07R728). When both dimensions are 32767, I think the window becomes as big as screen size (at least this was what I saw when I tested it).

James Holderness (j4james) · 2024-08-15T17:53:41Z

I've just been testing this branch, and there appears to be an issue with the height calculation (I think only when you have the tab bar visible). Because when I request a height of 24, I actually end up with a height of 22.

You can see this when toggling DECCOLM mode, which switches the width while attempting to maintain the current height - that results in the height repeatedly shrinking. For example, try running the first test in Vttest from within a WSL shell.

There is another issue which may be more difficult to fix, but it seems like there is a delay before the size change takes effect. For example, if you request a resize, and then immediately query the new size like this: printf "\e[8;30;100t\e[18t"; read, the reported size doesn't seem to match what you've just requested.

I'm not sure how big a deal this would be in practice, but again you can see the effect in Vttest (even without the shrinking height problem mentioned above), because the output appears to be formatted as if it were in a window of the wrong size, I'm guessing because AdaptDispatch class isn't yet aware of the updated size.

Leonard Hecker (lhecker) · 2024-08-15T18:31:41Z

There is another issue which may be more difficult to fix, but it seems like there is a delay before the size change takes effect.

I fear we may have to fix that issue later on. The resize necessarily needs to happen on the UI thread, because not all state until the SetWindowPos is properly thread-safe right now.

We could hotfix it by unlocking the console lock, raising the WindowSizeChanged event and waiting synchronously until that has completed. But I believe it'd be better if we instead made the necessary information (title bar height primarily; perhaps some other information as well) properly thread-safe or protected by the console lock. That way there would be no need to resume_foreground at all. Hopefully this race condition won't be too big of a deal until then.

nukoseer (nukoseer) · 2024-08-16T14:44:49Z

Apparently I'm not taking the title bar and tab bar height into account when calculating the window height. I noticed that there is some built-in logic in the GetLaunchDimensions function that has the title bar and tab bar dimensions hard-coded. I am trying to use that logic in my case, but I think that logic is also not that correct maybe. I could not make it work properly yet. It keeps shrinking or growing depending on used hard-coded values. I also tried to measure exact pixel size of the title bar and tab bar, and used those values but looks like something is still off or missing.

Dustin L. Howett (DHowett) · 2024-08-16T23:12:14Z

Holy s--t, somebody actually did it! Thanks so much!

We've had a lot of folks on vacation this week, but I'll make sure we get more reviewers on it next week.

Mike Griese (zadjii-msft) · 2024-08-20T20:21:49Z

I am trying to use that logic in my case, but I think that logic is also not that correct maybe.

that literally wouldn't surprise me at all. I wouldn't be shocked if that method was written before some UX refresh years back, but never updated accordingly.

on my 125% display:

on a 100% display:

hmm

Mike Griese (zadjii-msft)

blocking over the resize rejection case, and the TermControl::GetProposedDimensions thing. If fixing the TermControl::GetProposedDimensions doesn't fix the weird off-by-one on the resize, I'm guessing it's cause of snapToGridOnResize. There's known issues with how it handles DPI: #4175

FWIW, on a 100% scale display, this does work like a charm 👌 but yea I'm consistently seeing... inconsitent off-by-ones on the 125% display. Flippin' weird.

Mike Griese (zadjii-msft) · 2024-08-20T20:26:13Z

+    void TerminalWindow::_WindowSizeChanged(const IInspectable&, winrt::Microsoft::Terminal::Control::WindowSizeChangedEventArgs args)
+    {
+        til::size cellCount{ args.Width(), args.Height() };
+        const auto dpi = GetDpiForWindow(_hwnd);


IIRC const auto scaleFactor = DisplayInformation::GetForCurrentView().RawPixelsPerViewPixel(); is the right way to get the DPI from inside XAML land. That would remove the need for the TerminalWindow to actually store it's HWND

This is a temporary workaround that I had suggested. I think it's better to use GetDpiForWindow here because it more closely mirrors other, related code that also uses GetDpiForWindow. I'd like to clean this up all at once in the 1.23 period.

Mike Griese (zadjii-msft) · 2024-08-20T21:05:41Z

+        // - Not in fullscreen
+        // - Only one tab exists
+        // - Only one pane exists
+        if (!_isFullscreen && NumberOfTabs() == 1 && _GetFocusedTabImpl()->GetLeafPaneCount() == 1)


🛑: If we reject this resize, we need to make sure to let conpty know. We need to do something to signal a UserResize() back to the conpty. Otherwise, conpty will go on it's merry way at the new size, with the terminal at the wrong size

Is there any existing mechanism right now that signals back to conpty? Maybe I can check that to have some idea. Resizing is rejected also if the window is in quake mode (in AppHost). Probably that will also need to signal back.

ConptyConnection::Resize is the method you're looking for. The trick is just plumbing the rejection back to the correct layer to actually call that.

It does look like TermControl projects it's Connection, so that might not be so hard to call.

Mike Griese (zadjii-msft) · 2024-08-20T21:15:57Z

+        const auto settings{ TerminalSettings::CreateWithNewTerminalArgs(_settings, nullptr, nullptr) };
+        auto pixelSize = TermControl::GetProposedDimensions(settings.DefaultSettings(), dpi, cellCount.width, cellCount.height);


This might be problematic. The default settings for the default profile might not be what's currently active in that terminal window. Consider the scenario where you ctrl+mouse to increase the font size. The settings object doesn't actually know about that change, only the TermControl raising the event does.

I bet this could be solved by having the TermControl itself handle the WindowSizeChanged from the ControlCore (instead of straight bubbling it). The TermControl would get the size in chars, use that to figure out how big it wants to be in DIPs, then raise the size in DIPs up to the TerminalPage

Mike Griese (zadjii-msft) · 2024-08-20T21:21:22Z

(other than that though, the plumbing on this looks great, and overall I think you did cover a lot of the wacky weird edge cases like the tab row height well. Thanks for all this! Very excited to get this over the finish line)

nukoseer (nukoseer) · 2024-08-21T08:04:39Z

(other than that though, the plumbing on this looks great, and overall I think you did cover a lot of the wacky weird edge cases like the tab row height well. Thanks for all this! Very excited to get this over the finish line)

Thanks for review. I'll check what I can do about the issues you mentioned when I have time.

nukoseer (nukoseer) · 2024-08-22T19:54:08Z

I added GetNewDimensions() function in TermControl.cpp, it is exactly the same with GetProposedDimensions() but it does not get the font size from settings instead it calls _core->FontSizeUnscaled() which returns the font size in points. _core->FontSize() returns the font size in pixels and it was causing problems because engine->UpdateFont() expects the font size in points(?). I did not want to change the GetProposedDimensions to prevent any other side effects.
I multiplied titleBarHeight with dpi scale in NonClientIslandWindow::GetTotalNonClientExclusiveSize(). It is called when hide the title bar and always show tabs both on.
For rejection case, I added ConptyConnection::ResetSize() function, it justs resize the conpty with the current rows and cols. Not sure if this is good idea.

After these changes there was no shrinking/growing problem at different DPI scales (at least on my setup). I hope there are no other flaws. Reviews are welcome. :)

nukoseer (nukoseer) · 2024-08-25T10:41:38Z

I realized there is no need to copy everything from GetProposedDimensions() to GetNewDimensions(). I removed some parts and made it a bit simpler.

The ResizeWindow event in TerminalApi is handled and bubbled to TerminalApi->ControlCore->TermControl->TerminalPage->TerminalWindow->AppHost. Resizing is accepted only if the window is not in fullscreen or quake mode, and has 1 tab and pane.

…window height

… of conpty if resizing is rejected

Mike Griese (zadjii-msft)

Oh this is so close to ready, I can feel it. I'm tempted to just merge this as-is and fix that in post, but since we're not shipping this till 1.23, I don't think there's any big rush ☺️

Mike Griese (zadjii-msft) · 2024-08-26T16:22:28Z

+        {
+            WindowSizeChanged.raise(*this, args);
+        }
+        else if (const auto& control{ _GetActiveControl() })


Okay this is subtle, but this WindowSizeChanged handler might be being called for an inactive tab. So like, (as written) an inactive tab might get a CSI t, and then we'll reset the size of the focused tab, not the one that raised the event.

Fortunately, this is easy to handle! We've already set the sender of this event to the TermControl requesting the resize. So you can uncomment /*sender*/ above, and instead do:

Suggested change

else if (const auto& control{ _GetActiveControl() })

else if (const auto& control{ sender.try_as<TermControl>() })

Mike Griese (zadjii-msft) · 2024-08-27T10:58:00Z

+
+    if (_pfnWindowSizeChanged)
+    {
+        _pfnWindowSizeChanged(width, height);


Looks like the "audit mode" CI caught this:

src\cascadia\TerminalCore\TerminalApi.cpp(112,0): Error C26447: The function is declared 'noexcept' but calls function 'operator()()' which may throw exceptions (f.6).

I bet you can just remove noexcept from Terminal::ResizeWindow. or wrap it in a try{} CATCH_LOG()

Mike Griese (zadjii-msft)

Leonard Hecker (lhecker) approved these changes Aug 15, 2024

View reviewed changes

Mike Griese (zadjii-msft) requested changes Aug 20, 2024

View reviewed changes

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 Aug 20, 2024

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 Aug 21, 2024

Mike Griese (zadjii-msft) added this to the Terminal v1.23 milestone Aug 22, 2024

nukoseer (nukoseer) added 5 commits August 25, 2024 13:51

Handle window resize event (CSI t, resize)

dd3e5e1

The ResizeWindow event in TerminalApi is handled and bubbled to TerminalApi->ControlCore->TermControl->TerminalPage->TerminalWindow->AppHost. Resizing is accepted only if the window is not in fullscreen or quake mode, and has 1 tab and pane.

Clean up and format

7fbf95e

Take the tab bar and title bar into account when calculating the new …

b6b65c8

…window height

Fix shringking/growing when display scale is not %100, reset the size…

230f91b

… of conpty if resizing is rejected

Simplfy and clean some parts

0deb984

nukoseer (nukoseer) force-pushed the dev/handle_resize_event branch from ab8d331 to 0deb984 Compare August 25, 2024 11:06

nukoseer (nukoseer) requested a review from Mike Griese (zadjii-msft) August 25, 2024 11:11

Mike Griese (zadjii-msft) reviewed Aug 26, 2024

View reviewed changes

Mike Griese (zadjii-msft) assigned nukoseer (nukoseer) Aug 26, 2024

Reset the size of the correct tab if the size request is rejected

86d147b

Mike Griese (zadjii-msft) reviewed Aug 27, 2024

View reviewed changes

Remove noexcept from ResizeWindow

4509cec

Mike Griese (zadjii-msft) approved these changes Aug 29, 2024

View reviewed changes

Mike Griese (zadjii-msft) merged commit 837215b into microsoft:main Aug 29, 2024

Mike Griese (zadjii-msft) mentioned this pull request Oct 5, 2024

ANSI sequence "ESC [ 8;<lines>;<cols>t" fails to resize window in Terminal V1.4.3243.0, works in CMD.EXE (old console window) #8673

Closed

James Holderness (j4james) mentioned this pull request Oct 10, 2024

Have a way of actually knowing the geometry of the terminal #18025

Closed

Marco Bueß (MarcoBuess) mentioned this pull request Jan 7, 2025

Inconsistent gutters/margins on HiDPI displays with snapToGridOnResize #4175

Open

James Holderness (j4james) mentioned this pull request Dec 28, 2025

DECDHL double-height (box drawing) characters missing in vttest test case #19682

Open

	auto weakThis{ get_weak() };
	co_await wil::resume_foreground(Dispatcher());
	if (auto control{ weakThis.get() })

		const auto islandWidth = Utils::ClampToShortMax(lrintf(static_cast<float>(newSize.width)), 1);
		const auto islandHeight = Utils::ClampToShortMax(lrintf(static_cast<float>(newSize.height)), 1);

		const auto settings{ TerminalSettings::CreateWithNewTerminalArgs(_settings, nullptr, nullptr) };
		auto pixelSize = TermControl::GetProposedDimensions(settings.DefaultSettings(), dpi, cellCount.width, cellCount.height);

	else if (const auto& control{ _GetActiveControl() })
	else if (const auto& control{ sender.try_as<TermControl>() })

Conversation

nukoseer (nukoseer) commented Aug 15, 2024

Uh oh!

nukoseer (nukoseer) commented Aug 15, 2024

Uh oh!

Leonard Hecker (lhecker) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leonard Hecker (lhecker) commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emanuel Hoogeveen (ehoogeveen-medweb) commented Aug 15, 2024

Uh oh!

nukoseer (nukoseer) commented Aug 15, 2024

Uh oh!

James Holderness (j4james) commented Aug 15, 2024

Uh oh!

Leonard Hecker (lhecker) commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nukoseer (nukoseer) commented Aug 16, 2024

Uh oh!

Dustin L. Howett (DHowett) commented Aug 16, 2024

Uh oh!

Mike Griese (zadjii-msft) commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) commented Aug 20, 2024

Uh oh!

nukoseer (nukoseer) commented Aug 21, 2024

Uh oh!

nukoseer (nukoseer) commented Aug 22, 2024

Uh oh!

nukoseer (nukoseer) commented Aug 25, 2024

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Leonard Hecker (lhecker) commented Aug 15, 2024 •

edited

Loading

Leonard Hecker (lhecker) commented Aug 15, 2024 •

edited

Loading

Mike Griese (zadjii-msft) commented Aug 20, 2024 •

edited

Loading