Preview 1.22.2362.0 breaks Terminal.Gui apps

[tig]

Windows Terminal version

1.22.2362.0

Windows build number

10.0.26120.0

Other Software

Terminal.Gui - any version

Steps to reproduce

• Run UICatalog from the Terminal.Gui solution.
• Start any Scenario
• Exit that Scenario

Each of these is like running a Termina.Gui app afresh.

The same thing can be reproduced by just running .\UICatalog\bin\Debug\net8.0\UICatalog.exe and exiting repeatedly.

Expected Behavior

Actual Behavior

Note the bottom of the terminal is corrupt with prior content and the new instance of the app is not getting the correct terminal dimensions (too small).

[tig]

This breaks PowerShell's Out-ConsoleGridView which uses Terminal.Gui, but in a slightly different manner: The prompt is not re-displayed.

[tig]

May I suggest the WT add some Terminal.Gui tests to the WT test suite? It's a pretty popular library for building TUI apps that stresses a ton of terminal capabilites. I'd be happy to collaborate on that!

[lhecker]

I think you could actually argue that the new implementation is more correct than the old one, because as it turns out you're accidentally asking for a window that's 1 row less tall every time you switch between screen buffers. Since we currently forbid changing the screen size, this means that the content that gets copied is 1 row less tall without the window size having changed.

This happens because you call GetConsoleScreenBufferInfoEx followed by SetConsoleScreenBufferSize without accounting for the quirkiness of this API: The Get method returns an inclusive rectangle while the Set method expects an exclusive one. You can read more about this here: #14355

This is the stack trace:

...
2, KernelBase.dll!SetConsoleScreenBufferInfoEx+0xc9
3, ILStubClass.IL_STUB_PInvoke(IntPtr, CONSOLE_SCREEN_BUFFER_INFOEX ByRef) + 0x103 <-- 0x7ff911584db3
4, Terminal.Gui.WindowsConsole.SetConsoleOutputWindow(CONSOLE_SCREEN_BUFFER_INFOEX) + 0x34 <-- 0x7ff9128f2fc4
5, Terminal.Gui.WindowsConsole.SetConsoleOutputWindow(System.Drawing.Point ByRef) + 0x19b <-- 0x7ff9128f2e6b
6, Terminal.Gui.WindowsConsole.Cleanup() + 0x66 <-- 0x7ff9128f28e6
7, Terminal.Gui.WindowsDriver.End() + 0xde <-- 0x7ff9128f280e
8, Terminal.Gui.Application.ResetState(Boolean) + 0x54f <-- 0x7ff91154be9f
9, Terminal.Gui.Application.Shutdown() + 0x20 <-- 0x7ff9128f1cf0
10, UICatalog.UICatalogApp.RunUICatalogTopLevel() + 0xd0 <-- 0x7ff91154afe0
11, UICatalog.UICatalogApp.UICatalogMain(Options) + 0x2f4 <-- 0x7ff91154a1a4
12, UICatalog.UICatalogApp.Main(System.String[]) + 0x669 <-- 0x7ff911521f99
...

The issue is in SetConsoleOutputWindow(Point). Changing the code to this fixes the issue:

// ...
if (!GetConsoleScreenBufferInfoEx (_screenBuffer, ref csbi))
{
    throw new Win32Exception (Marshal.GetLastWin32Error ());
}

csbi.srWindow.Bottom += 1;                                                   // <---
csbi.srWindow.Right += 1;                                                    // <---

Size sz = new (
                   csbi.srWindow.Right - csbi.srWindow.Left,                 // <---
                   Math.Max (csbi.srWindow.Bottom - csbi.srWindow.Top, 0));  // <---
position = new (csbi.srWindow.Left, csbi.srWindow.Top);
SetConsoleOutputWindow (csbi);
// ...

[tig]

I think you could actually argue that the new implementation is more correct than the old one, because as it turns out you're accidentally asking for a window that's 1 row less tall every time you switch between screen buffers. Since we currently forbid changing the screen size, this means that the content that gets copied is 1 row less tall without the window size having changed.

I agree that the old behavior was odd/incorrect and the new behavior is more correct. We can work on a fix to both Terminal.Gui v1 and v2. Tracking here gui-cs/Terminal.Gui#3752

However, I would like to understand the WT team's tenets around not breaking existing apps.

Back in the olden days for Windows we were quite hardcore about this. Even if we had a bug in Windows we went to extraordinary lengths to not break existing apps.

Have you exhausted all options for how you can implement the new behavior in a way that won't break existing apps that use the API as it worked before?

[lhecker]

There seems to have been a misunderstanding... I haven't even thought about closing this bug yet. 😅 I was just working on other issues first after my initial investigation above. This was surely due to me being too terse in my response. I apologize for that.

That said, I believe we can solve this by using the window size assigned via the signal pipe for all VtIo operations instead of passing the oldSize to WriteScreenInfo.

[tig]

There seems to have been a misunderstanding... I haven't even thought about closing this bug yet. 😅 I was just working on other issues first after my initial investigation above. This was surely due to me being too terse in my response. I apologize for that.

No problem! I get it. Thanks for taking the time.

That said, I believe we can solve this by using the window size assigned via the signal pipe for all VtIo operations instead of passing the oldSize to WriteScreenInfo.