Resizing while alt buffer is active, then disabling it, blows away renderer #17407

Closed
opened 2026-01-31 05:41:37 +00:00 by claunia · 6 comments
Owner

Originally created by @DHowett on GitHub (May 4, 2022).

  1. WSL, in a small window
  2. vim -u NONE
  3. Maximize the window
  4. Esc :q Enter
  5. Observe the window get "stuck" on vim

It looks like we're failing to render a dirty region that is outside of the new, smaller main buffer.

It appears as though Terminal is not resizing the main buffer, as it believes it is already the correct size.

That check is happening here:

9edf55de75/src/cascadia/TerminalCore/Terminal.cpp (L252-L256)

At the time of failure, _GetMutableViewport returns _mutableViewport, since we are no longer in the alt buffer.

Setting a write breakpoint on _mutableViewport to determine who is setting it, it's coming from EraseAll -> SetViewportPosition:

9edf55de75/src/cascadia/TerminalCore/TerminalApi.cpp (L42-L47)

Potentially regressed in #13024 (/cc @j4james)

Originally created by @DHowett on GitHub (May 4, 2022). 1. WSL, in a small window 2. `vim -u NONE` 3. Maximize the window 4. <kbd>Esc</kbd> `:q` <kbd>Enter</kbd> 5. Observe the window get "stuck" on vim It looks like we're failing to render a dirty region that is outside of the new, smaller main buffer. It appears as though Terminal is not resizing the main buffer, as it believes it is already the correct size. That check is happening here: https://github.com/microsoft/terminal/blob/9edf55de7564912cb8e19ef3a3f88b689be08af7/src/cascadia/TerminalCore/Terminal.cpp#L252-L256 At the time of failure, `_GetMutableViewport` returns `_mutableViewport`, since we are no longer in the alt buffer. Setting a write breakpoint on `_mutableViewport` to determine who is setting it, it's coming from `EraseAll` -> `SetViewportPosition`: https://github.com/microsoft/terminal/blob/9edf55de7564912cb8e19ef3a3f88b689be08af7/src/cascadia/TerminalCore/TerminalApi.cpp#L42-L47 Potentially regressed in #13024 (/cc @j4james)
Author
Owner

@j4james commented on GitHub (May 5, 2022):

Yeah, it's definitely regressed in #13024. I remember thinking at the time that the assignment to _mutableViewport looked suspicious, but I got that from the original "EraseAll" implementation here:

6b936d9a74/src/cascadia/TerminalCore/TerminalApi.cpp (L301-L303)

So I'm not sure why that worked before. There must be something else different between the conhost and terminal architectures. I'll do some digging.

@j4james commented on GitHub (May 5, 2022): Yeah, it's definitely regressed in #13024. I remember thinking at the time that the assignment to `_mutableViewport` looked suspicious, but I got that from the original "EraseAll" implementation here: https://github.com/microsoft/terminal/blob/6b936d9a74a5d677387f3edc9a68e6b33d566606/src/cascadia/TerminalCore/TerminalApi.cpp#L301-L303 So I'm not sure why that worked before. There must be something else different between the conhost and terminal architectures. I'll do some digging.
Author
Owner

@j4james commented on GitHub (May 5, 2022):

OK, I see now why it worked before. The terminal "EraseAll" implementation had an early exit check for _inAltBuffer() which meant the viewport wouldn't be set at all. But I think that's one of those things that only worked in Terminal because conpty would redraw the whole screen afterwards anyway. It's not actually clearing the screen in that case, so we can't do that in the current implementation.

@j4james commented on GitHub (May 5, 2022): OK, I see now why it worked before. The terminal "EraseAll" implementation had an early exit check for `_inAltBuffer()` which meant the viewport wouldn't be set at all. But I think that's one of those things that only worked in Terminal because conpty would redraw the whole screen afterwards anyway. It's not actually clearing the screen in that case, so we can't do that in the current implementation.
Author
Owner

@j4james commented on GitHub (May 5, 2022):

But now that I think about it we can just make the Terminal::SetViewportPosition method a no-op when in the alt buffer - that should fix it.

@j4james commented on GitHub (May 5, 2022): But now that I think about it we can just make the `Terminal::SetViewportPosition` method a no-op when in the alt buffer - that should fix it.
Author
Owner

@DHowett commented on GitHub (May 5, 2022):

That makes sense to me. The viewport in the alt buffer is always the entire buffer, right?

@DHowett commented on GitHub (May 5, 2022): That makes sense to me. The viewport in the alt buffer is always the entire buffer, right?
Author
Owner

@j4james commented on GitHub (May 5, 2022):

Yeah, if you look at the _GetMutableViewport method, it's just returning a Viewport object with the alt buffer dimensions, so it's always at 0,0.

f4e0d9f2bd/src/cascadia/TerminalCore/Terminal.cpp (L970-L977)

@j4james commented on GitHub (May 5, 2022): Yeah, if you look at the `_GetMutableViewport` method, it's just returning a `Viewport` object with the alt buffer dimensions, so it's always at 0,0. https://github.com/microsoft/terminal/blob/f4e0d9f2bd1f5da3a6c378453b90320fb6b1c542/src/cascadia/TerminalCore/Terminal.cpp#L970-L977
Author
Owner

@ghost commented on GitHub (May 24, 2022):

:tada:This issue was addressed in #13039, which has now been successfully released as Windows Terminal Preview v1.14.143.🎉

Handy links:

@ghost commented on GitHub (May 24, 2022): :tada:This issue was addressed in #13039, which has now been successfully released as `Windows Terminal Preview v1.14.143`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.14.143) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#17407