Enable using the alt buffer in the Terminal #12561

zadjii-msft · 2022-02-23T22:32:24Z

This PR allows the Terminal to actually use the alt buffer
appropriately. Currently, we just render the alt buffer state into the
main buffer and that is wild. It means things like vim will let the
user scroll up to see the previous history (which it shouldn't).

Very first thing this PR does: updates the
{Trigger|Invalidate}Circling methods to instead be
{Trigger|Invalidate}Flush(bool circling). We need this so that when an
app requests the alt buffer in conpty, we can immediately flush the
frame before asking the Terminal side to switch to the other buffer. The
Circling methods was a great place to do this, but we don't actually
want to set the circled flag in VtRenderer when that happens just for a
flush.

The Terminal's implementation is a little different than conhost's.
Conhost's implementation grew organically, so I had it straight up
create an entire new screen buffer for the alt buffer. The Terminal
doesn't need all that! All we need to do is have a separate TextBuffer
for the alt buffer contents. This makes other parts easier as well - we
don't really need to do anything with the _mutableViewport in the alt
buffer, because it's always in the same place. So, we can just leave it
alone and when we come back to the main buffer, there it is. Helper
methods have been updated to account for this.

Closes ConPTY: pass through requests for the alt buffer #381
Closes Alternate screen shouldn't write to scrollback or normal screen #3492
I work here
Tests would be nice
ED3 in the alt buffer should erase the main scrollback buffer #3686, DEC private 47/1047 not supported (alt buffer) #3082, Scrolling in the Alternate Screen Buffer should send up/down keystrokes? #3321, Consider not wrapping the alternate screen on resize #3493 are all good follow-ups here.

Resizing is surely boned but this is 1000% percent better than nothing at all.

_⚠️ targets #12561 ⚠️_ "Alternate scroll mode" is a neat little mode where the app wants mouse wheel events to come through as arrow keypresses instead, when in the alternate buffer. Now that we've got support for the alt buffer in the Terminal, we can support this as well. * [x] Closes #3321 * [x] I work here * [ ] Tests would be nice Tested manually with ```bash printf "\e[?1007h" ; man ps ```

DHowett · 2022-02-24T20:07:10Z

/cc @j4james for opinions since he's intimately familiar with these codepaths :)

src/cascadia/TerminalCore/Terminal.cpp

src/cascadia/TerminalCore/TerminalApi.cpp

src/cascadia/TerminalCore/TerminalDispatch.cpp

DHowett · 2022-02-25T18:38:27Z

Does TermControl's scroll bar automatically do the right thing here? I'm pleasantly surprised to not see anything in TC (whole project!) changing :)

zadjii-msft · 2022-02-25T20:02:42Z

Does TermControl's scroll bar automatically do the right thing here? I'm pleasantly surprised to not see anything in TC (whole project!) changing :)

sure does, we send a scroll position changed event, and it just works

codeofdusk · 2022-02-25T20:34:00Z

I'm a bit concerned about how this PR will interact with #12358 (would try building a branch with both integrated to test, but am having trouble building). Do we know what text will be sent as a notification when the alt buffer is exited? (for whatever it's worth, NVDA's current diffing approach sees the entire contents of the regular buffer as new, so reports it all, which it shouldn't).

zadjii-msft · 2022-02-25T20:47:00Z

At the moment, no. Ideally... nothing would get sent? not sure that makes sense either.

The complicating factor here is that ConPTY might just be repainting the whole screen when we switch buffers. Obviously not ideal but IMO not something that we should hold the whole fix over at this time. Lemme verify that (once I get my branch building again 😛)

EDIT:

Money sequences are between the red input.
Interestingly, we clear the screen... then switch back to the main buffer? Might be a artifact of mc being weird, but I didn't have time today to set up simpler tests.

Another test:

Form that one, it's clearer that conpty is re-emitting the buffer contents. Drat. So it'll get re-echoed, which sure isn't worse than it is today.

@DHowett - EDITed to add screen reader alt text.

…f that we're supposed to do when entering/exiting the alt buffer

…er-terminal

zadjii-msft · 2022-04-12T11:05:26Z

Alrighty so the dependent PRs here (#12569, #12719) have the signoffs, so we just need eyes on this one. I want to merge them all in one day, since yea, there's something weird about this one without #12719, but the combined diff is A LOT to review.

lhecker · 2022-04-12T19:42:25Z

src/cascadia/TerminalCore/Terminal.cpp

+    // any scrollback, so we just need to resize it and presto, we're done.
+    if (_inAltBuffer())
+    {
+        _altBuffer->GetCursor().StartDeferDrawing();


This isn't needed is it? Just move the endDefer down to where you call StartDeferDrawing()?

lhecker · 2022-04-12T19:44:54Z

src/cascadia/TerminalCore/Terminal.cpp

 Viewport Terminal::_GetMutableViewport() const noexcept
 {
-    return _mutableViewport;
+    return _inAltBuffer() ? Viewport::FromDimensions(_mutableViewport.Dimensions()) :
+                            _mutableViewport;
 }

 short Terminal::GetBufferHeight() const noexcept
 {
-    return _mutableViewport.BottomExclusive();
+    return _GetMutableViewport().BottomExclusive();
 }

 // ViewStartIndex is also the length of the scrollback
 int Terminal::ViewStartIndex() const noexcept
 {
-    return _mutableViewport.Top();
+    return _inAltBuffer() ? 0 : _mutableViewport.Top();
 }

 int Terminal::ViewEndIndex() const noexcept
 {
-    return _mutableViewport.BottomInclusive();
+    return _inAltBuffer() ? _mutableViewport.Height() - 1 : _mutableViewport.BottomInclusive();
 }

 // _VisibleStartIndex is the first visible line of the buffer
 int Terminal::_VisibleStartIndex() const noexcept
 {
-    return std::max(0, ViewStartIndex() - _scrollOffset);
+    return _inAltBuffer() ? ViewStartIndex() :
+                            std::max(0, ViewStartIndex() - _scrollOffset);
 }

 int Terminal::_VisibleEndIndex() const noexcept
 {
-    return std::max(0, ViewEndIndex() - _scrollOffset);
+    return _inAltBuffer() ? ViewEndIndex() :
+                            std::max(0, ViewEndIndex() - _scrollOffset);
 }


I don't think these changes are necessary. I believe it'd be better to leave the code as it is, because it'd better to get rid of the Viewport class altogether which is easier with the previous code.

Also since these methods might be used for clamping values, I believe that this might introduce bugs more easily.

I am guessing they are necessary because of _scrollOffset... which makes me wonder how we account for the scroll offset being per-buffer but global state

We could use a getter for _scrollOffset which returns 0 for the alt buffer. 🤔

lhecker · 2022-04-12T19:49:07Z

src/cascadia/TerminalCore/TerminalApi.cpp

+                                              cursorSize,
+                                              true,
+                                              _mainBuffer->GetRenderer());
+    _mainBuffer->SetAsActiveBuffer(false);


Unrelated to this PR, I just realized that distributing the active state as a boolean across 2 instances, makes it possible to exceptions to get our app in an invalid state were both or none of the TextBuffers are active.

…er-terminal

DHowett

37/37 but I have some questions, plus the selection-related followup from Teams

DHowett · 2022-04-12T20:14:06Z

src/renderer/vt/state.cpp

+HRESULT VtEngine::SwitchScreenBuffer(const bool useAltBuffer) noexcept
+{
+    RETURN_IF_FAILED(_SwitchScreenBuffer(useAltBuffer));
+    RETURN_IF_FAILED(_Flush());


order of operations -- interesting. Can this cause torn state?

Hmm. So when we switch to the alt buffer, we first

flush out the current frame

emit this sequence, and flush again.

so I think this is basically atomic, and then we return back out and continue processing more output.

src/renderer/vt/VtSequences.cpp

DHowett · 2022-04-12T20:15:43Z

src/renderer/vt/VtSequences.cpp

+// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
+[[nodiscard]] HRESULT VtEngine::_SwitchScreenBuffer(const bool useAltBuffer) noexcept
+{
+    return _Write(useAltBuffer ? "\x1b[?1049h" : "\x1b[?1049l");


This is technically only valid for the xterm-derived engines; wintelnet might B E E F

True but I doubt that users of telnet.exe hit anything using the alt buffer...

DHowett · 2022-04-12T20:20:42Z

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

+
+void ConptyRoundtripTests::AltBufferToAltBufferTest()
+{
+    Log::Comment(L"When we request the alt buffer when we're already in the alt buffer, we should still clear it out and replace it.");


technically only true for DECSET 1049; other modes leave it dirty (might be interesting to consider)

DHowett · 2022-04-12T20:26:23Z

src/cascadia/TerminalCore/TerminalDispatch.cpp

+    auto row = savedCursorState.Row;
+    const auto col = savedCursorState.Column;
+
+    // The saved coordinates are always absolute, so we need reset the origin mode temporarily.


it doesn't look like we are doing anything with the origin mode

yea I mean, terminal-side doesn't have that implemented at all, iirc. I just kinda assumed in the long view of "merging adaptdispatch and termdispatch" we'd fix it in post.

src/cascadia/TerminalCore/Terminal.hpp

DHowett · 2022-04-12T20:32:55Z

src/cascadia/TerminalCore/Terminal.cpp

 Viewport Terminal::_GetMutableViewport() const noexcept
 {
-    return _mutableViewport;
+    return _inAltBuffer() ? Viewport::FromDimensions(_mutableViewport.Dimensions()) :
+                            _mutableViewport;
 }

 short Terminal::GetBufferHeight() const noexcept
 {
-    return _mutableViewport.BottomExclusive();
+    return _GetMutableViewport().BottomExclusive();
 }

 // ViewStartIndex is also the length of the scrollback
 int Terminal::ViewStartIndex() const noexcept
 {
-    return _mutableViewport.Top();
+    return _inAltBuffer() ? 0 : _mutableViewport.Top();
 }

 int Terminal::ViewEndIndex() const noexcept
 {
-    return _mutableViewport.BottomInclusive();
+    return _inAltBuffer() ? _mutableViewport.Height() - 1 : _mutableViewport.BottomInclusive();
 }

 // _VisibleStartIndex is the first visible line of the buffer
 int Terminal::_VisibleStartIndex() const noexcept
 {
-    return std::max(0, ViewStartIndex() - _scrollOffset);
+    return _inAltBuffer() ? ViewStartIndex() :
+                            std::max(0, ViewStartIndex() - _scrollOffset);
 }

 int Terminal::_VisibleEndIndex() const noexcept
 {
-    return std::max(0, ViewEndIndex() - _scrollOffset);
+    return _inAltBuffer() ? ViewEndIndex() :
+                            std::max(0, ViewEndIndex() - _scrollOffset);
 }


I am guessing they are necessary because of _scrollOffset... which makes me wonder how we account for the scroll offset being per-buffer but global state

src/cascadia/TerminalCore/Terminal.cpp

src/cascadia/TerminalCore/TerminalApi.cpp

DHowett · 2022-04-12T20:40:02Z

src/cascadia/TerminalCore/TerminalApi.cpp

+    _mainBuffer->SetAsActiveBuffer(true);
+
+    // destroy the alt buffer
+    _altBuffer = nullptr;
+
+    // update all the hyperlinks on the screen
+    UpdatePatternsUnderLock();
+
+    // Update scrollbars
+    _NotifyScrollEvent();
+
+    // redraw the screen
+    try
+    {
+        _activeBuffer().TriggerRedrawAll();
+    }
+    CATCH_LOG();


this seems like it should be a helper for switching buffers, but I understand if that is not tenable

phil-blain · 2022-04-13T17:26:09Z

Thanks a lot @DHowett for this !! It was probably the most annoying Terminal bug in my day to day use.

Please tell me this is a backport candidate for the 1.12 series :D (stuck on 1909...)

DHowett · 2022-04-13T17:27:57Z

@zadjii-msft deserves all the credit :)

Unfortunately, a backport isn't in the cards just yet; the 1.13 and 1.14 arcs have taken some changes to how we handle and dispatch VT sequences, so it probably wouldn't just slot in . . . and I'm a bit worried about maintaining our stable release as we backport features into it 😄

phil-blain · 2022-04-13T17:31:07Z

I understand. Thanks @zadjii-msft for this :)

By curisority, if I clone the code and build it myself form main on 1909, will it not build ? or will it build, but some of the feature not work ?

DHowett · 2022-04-13T17:34:13Z

That's an excellent question. I can't recall which feature it was that we added which will break on 19H1...

If you edit the Package-Dev.appxmanifest to specify a minimum version of 10.0.18362.0 it will probably build and deploy, and it might even work. YMMV!

phil-blain · 2022-04-13T17:35:41Z

Thanks. I'll try it.

zadjii-msft · 2022-04-13T17:36:00Z

I think it might just work. The quirks around opacity that we're hoping will be fixed in Vb aren't removed in main yet, and we haven't added nested context menus yet, so that crash shouldn't be an issue.

zadjii-msft · 2022-04-13T17:40:24Z

@phil-blain FYI You may want to wait until #12719 merges - there's a fix somewhere in there for some weird bugs around resizing the alt buffer that are present in this PR. I was hoping to merge them a little closer together, still waiting for CI to spin on that one.

phil-blain · 2022-04-14T14:15:14Z

Sorry to bother you guys again - do you happen to know off-hand how much disk space does the build need ? I tried building in GitHub actions which has 14 GB of disk space and it seems it is not enough...

zadjii-msft · 2022-04-14T14:17:53Z

Oh it requires absolutely enormous amounts 😭 My obj folder is in excess of 26GB, just for Debug:

cppwinrt is just like that 🤷

phil-blain · 2022-04-14T14:20:13Z

OK thanks for confirming :) local VM it is then!

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

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

Handy links:

zadjii-msft added 5 commits February 23, 2022 11:20

I think this is the conhost half of the changes

83466a4

This quite nearly implements everything for the Terminal half

e94e08c

Resizing is surely boned but this is 1000% percent better than nothing at all.

Most of the remaining todos, comments

dd702c7

one last comment

1209fa4

more comments are always good

bf24cdd

This comment was marked as resolved.

Sign in to view

zadjii-msft mentioned this pull request Feb 24, 2022

Scrolling in the Alternate Screen Buffer should send up/down keystrokes? #3321

Closed

zadjii-msft mentioned this pull request Feb 24, 2022

Add support for alternate scroll mode in Terminal #12569

Merged

3 tasks

spel

dff1b94

j4james mentioned this pull request Feb 24, 2022

Eliminate the IRenderTarget interface #12568

Merged

6 tasks

fix tests

60d2c2e

j4james reviewed Feb 24, 2022

View reviewed changes

src/cascadia/TerminalCore/Terminal.cpp Show resolved Hide resolved

j4james reviewed Feb 24, 2022

View reviewed changes

src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalCore/TerminalDispatch.cpp Outdated Show resolved Hide resolved

zadjii-msft added 3 commits February 28, 2022 12:19

Stashing this for now. I think this does the save/restore cursor stuf…

54dc304

…f that we're supposed to do when entering/exiting the alt buffer

start writing tests

57094b7

make this test way more elaborate

69d0973

This was referenced Mar 23, 2022

FR: IDE-style marks on scrollbar #1527

Closed

Conpty "pass-through" doesn't maintain order of operations #8698

Closed

Merge remote-tracking branch 'origin/main' into dev/migrie/b/alt-buff…

7c3e808

…er-terminal

zadjii-msft assigned DHowett and lhecker Apr 12, 2022

lhecker approved these changes Apr 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/b/alt-buff…

002fbbc

…er-terminal

DHowett requested changes Apr 12, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 12, 2022

fix the urls in the alt buffer

f209d98

DHowett approved these changes Apr 12, 2022

View reviewed changes

DHowett merged commit 13fb1f5 into main Apr 12, 2022

DHowett deleted the dev/migrie/b/alt-buffer-terminal branch April 12, 2022 22:19

This was referenced May 24, 2022

Alternate screen shouldn't write to scrollback or normal screen #3492

Closed

ConPTY: pass through requests for the alt buffer #381

Closed

codeofdusk mentioned this pull request May 26, 2022

Terminal crashes when starting nano in bash when screen reader is enabled #13183

Closed

zadjii-msft mentioned this pull request Aug 7, 2023

why isn't possible to scroll anymore in a linux screen #15796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable using the alt buffer in the Terminal #12561

Enable using the alt buffer in the Terminal #12561

zadjii-msft commented Feb 23, 2022 •

edited

Loading

This comment was marked as resolved.

DHowett commented Feb 24, 2022

DHowett commented Feb 25, 2022

zadjii-msft commented Feb 25, 2022

codeofdusk commented Feb 25, 2022

zadjii-msft commented Feb 25, 2022 •

edited by DHowett

Loading

zadjii-msft commented Apr 12, 2022

lhecker Apr 12, 2022

lhecker Apr 12, 2022

DHowett Apr 12, 2022

lhecker Apr 12, 2022

lhecker Apr 12, 2022

DHowett left a comment

DHowett Apr 12, 2022

zadjii-msft Apr 12, 2022

DHowett Apr 12, 2022

zadjii-msft Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

zadjii-msft Apr 12, 2022

DHowett Apr 12, 2022

DHowett Apr 12, 2022

phil-blain commented Apr 13, 2022

DHowett commented Apr 13, 2022

phil-blain commented Apr 13, 2022

DHowett commented Apr 13, 2022

phil-blain commented Apr 13, 2022

zadjii-msft commented Apr 13, 2022

zadjii-msft commented Apr 13, 2022

phil-blain commented Apr 14, 2022

zadjii-msft commented Apr 14, 2022

phil-blain commented Apr 14, 2022

ghost commented May 24, 2022

Enable using the alt buffer in the Terminal #12561

Enable using the alt buffer in the Terminal #12561

Conversation

zadjii-msft commented Feb 23, 2022 • edited Loading

This comment was marked as resolved.

DHowett commented Feb 24, 2022

DHowett commented Feb 25, 2022

zadjii-msft commented Feb 25, 2022

codeofdusk commented Feb 25, 2022

zadjii-msft commented Feb 25, 2022 • edited by DHowett Loading

zadjii-msft commented Apr 12, 2022

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

phil-blain commented Apr 13, 2022

DHowett commented Apr 13, 2022

phil-blain commented Apr 13, 2022

DHowett commented Apr 13, 2022

phil-blain commented Apr 13, 2022

zadjii-msft commented Apr 13, 2022

zadjii-msft commented Apr 13, 2022

phil-blain commented Apr 14, 2022

zadjii-msft commented Apr 14, 2022

phil-blain commented Apr 14, 2022

ghost commented May 24, 2022

zadjii-msft commented Feb 23, 2022 •

edited

Loading

zadjii-msft commented Feb 25, 2022 •

edited by DHowett

Loading