Refactor and simplify the ConGetSet API by j4james · Pull Request #12247 · microsoft/terminal

j4james · 2022-01-25T16:39:22Z

Summary of the Pull Request

This PR refactors the ConGetSet API, eliminating some of the bloat produced by the DoSrvPrivateXXXX functions, simplifying the method naming to more closely match the ITerminalApi interface, and making better use of exceptions for error conditions in place of boolean return values.

References

This is another small step towards merging the AdaptDispatch and TerminalDispatch classes (#3849).

PR Checklist

Closes IL esape sequence doesn't work correctly in non-active buffer #12193
Closes DECAWM escape sequence doesn't work correctly in non-active buffer #12194
CLA signed.
Tests added/passed
Documentation updated.
Schema updated.
I've discussed this with core contributors already. Issue number where discussion took place: De-duplicate code for TerminalDispatch and AdaptDispatch #3849

Detailed Description of the Pull Request / Additional comments

There are two main parts to this. The first step was to get rid of all the DoSrvPrivateXXX functions, and move their implementation directly into the ConhostInternalGetSet class. For the most part this was just copying and pasting the code, but I also fixed a couple of bugs where we were using the wrong output buffer (the global buffer rather than the one associated with the output handle), and got rid of some unnecessary calls to GetActiveBuffer.

The second part was to make better use of exceptions for error conditions. Instead of catching the exceptions at the ConGetSet level, we now allow them to fall through all the way to the StateMachine. This greatly simplifies the AdaptDispatch implementation since it no longer needs to check a boolean return value on every ConGetSet call. This also enables the getter methods to return properties directly instead of having to use a reference parameter.

Validation Steps Performed

A number of the unit tests had to be updated to match the new API. Sometimes this just required changes to method names, but in other cases error conditions that were previously detected with boolean returns now needed to be caught as exceptions.

There were also a few direct calls to DoSrvPrivateXXX functions that now needed to be invoked through other means: either by generating an equivalent escape sequence, or calling a lower level API serving the same purpose.

And in the adapter tests, the mock ConGetSet implementation required significant refactoring to match the new interface, mostly to account for the changes in error handling.

j4james · 2022-01-25T17:47:50Z

Note to reviewers: it will make things a lot easier if you disable whitespace before reviewing these changes. It may also help to review outputStream.cpp and getset.cpp side by side, since many of those changes are just moving code from the one file to the other.

Also, if you prefer, I could actually split this up into two separate PRs, with the DoSrvPrivateXXX cleanup in the first, and the the new exception handling in the second. I just kept them together because of the overlap in updating the ConGetSet interface, which I figured might be easier to review in one go.

miniksa · 2022-01-28T00:44:48Z

src/host/outputStream.cpp

+void ConhostInternalGetSet::SetAutoWrapMode(const bool wrapAtEOL)
 {
-    return NT_SUCCESS(DoSrvPrivateSetAutoWrapMode(wrapAtEOL));
+    auto& outputMode = _io.GetActiveOutputBuffer().OutputMode;


This is the site of the #12194 fix, if you're following along from getset.cpp like I am.

miniksa · 2022-01-28T20:27:01Z

I was going to get you this today, but my performance review stuff is due this week. So I have it half done and will hopefully finish on Monday. Thanks for your patience @j4james. I'm very excited by this. I feel like you keep taking on all the stuff I wish I had time to do of your own volition and it's amazing.

miniksa

Overall, looks great. I figure all the things that are like "always returning true" will end up getting worked out into something more sensible as this refactoring continues into its next phases, so I didn't cry about it.

Just a few comments that you could take or leave... or things I noted out for other readers.

As always, @j4james, thank you for your contribution. I love seeing this get cleaner and cleaner!

miniksa · 2022-01-28T00:48:21Z

src/host/outputStream.cpp

-// - true if successful (see DoSrvPrivateSetScrollingRegion). false otherwise.
-bool ConhostInternalGetSet::PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins)
+// - <none>
+void ConhostInternalGetSet::SetScrollingRegion(const SMALL_RECT& scrollMargins)


It looks like the Top>Bottom check is gone now. Was that intentional?

Yeah. All the validation is already handled in AdaptDispatch::_DoSetTopBottomScrollingMargins, since that's where the margins are really being managed. This method is just forwarding a copy to SCREEN_INFORMATION so they can be accessed from the AdjustCursorPosition function. I'm hoping one day we'll be able to avoid that though.

miniksa · 2022-01-28T00:51:00Z

src/host/outputStream.cpp

+            coordDestination.Y = viewport.Top + 1;
+
+            // Note the revealed lines are filled with the standard erase attributes.
+            ScrollRegion(srScroll, srScroll, coordDestination, true);


Was about to ask where the wrapper went, but you're just letting the exception fly here. OK.

miniksa · 2022-01-28T00:54:34Z

src/host/outputStream.cpp

+// - insert - true if inserting lines, false if deleting lines
+void ConhostInternalGetSet::_modifyLines(const size_t count, const bool insert)
+{
+    auto& screenInfo = _io.GetActiveOutputBuffer();


And this is the site of the #12193 fix (the absence of .GetActiveBuffer() hanging off of GetActiveOutputBuffer())

The GetActiveBuffer thing wasn't the bug - those calls were all just redundant, because GetActiveOutputBuffer is already required to return the active buffer (in some cases it's literally an alias for GetActiveBuffer).

The bug was that the original code was getting the buffer from the global CONSOLE_INFORMATION class, which is not necessarily the same buffer as the one obtained from _io (which is tracking the current output handle).

miniksa · 2022-01-28T00:55:37Z

src/host/outputStream.cpp

+        ScrollRegion(srScroll, srScroll, coordDestination, true);
+
+        // The IL and DL controls are also expected to move the cursor to the left margin.
+        // For now this is just column 0, since we don't yet support DECSLRM.
+        THROW_IF_NTSTATUS_FAILED(screenInfo.SetCursorPosition({ 0, cursorPosition.Y }, false));


Errors now propagate up here (by exception throwing) instead of a silent eat and log like before.

Yeah, I don't think it matters much either way, but this seemed more consistent with how we're handling cursor position failures elsewhere. And at some point we're probably going to want to normalize all the different ways we're handling cursor positioning anyway.

miniksa · 2022-01-28T00:56:58Z

src/host/outputStream.cpp

+    {
+        return;
+    }
+


Lock is now gone here, but that is fine as we had to have been on lock already to hit the internal get/set from the VT callbacks anyway. Eliminates a recursive entry/exit.

(and for the other places where the lock is now missing)

miniksa · 2022-01-28T01:02:52Z

src/terminal/adapter/InteractDispatch.cpp

    case DispatchTypes::WindowManipulationType::RefreshWindow:
-        success = DispatchCommon::s_RefreshWindow(*_pConApi);
-        break;
+        DispatchCommon::s_RefreshWindow(*_pConApi);


Reminder, these are now throwing (here and below). So the early return true is fine and the error propagates on exception with the false return being "I didn't do nothin', boss".

miniksa · 2022-01-28T01:03:24Z

src/terminal/adapter/InteractDispatch.cpp

-    size_t rowFixed = row;
-    size_t colFixed = col;
+    // The parser should never return 0 (0 maps to 1), so this is a failure condition.
+    THROW_HR_IF(E_INVALIDARG, row == 0);


I apologize for the point in time where somehow there was an allergy to early returns.

miniksa · 2022-01-28T01:09:16Z

src/terminal/adapter/InteractDispatch.cpp

+    THROW_IF_FAILED(SizeTToShort(colFixed, &coordCursor.X));
+
+    // Set the line and column values as offsets from the viewport edge. Use safe math to prevent overflow.
+    THROW_IF_FAILED(ShortAdd(coordCursor.Y, csbiex.srWindow.Top, &coordCursor.Y));


Hmmm. I thought we had something in the Numerics library that would just throw on fail here with a typed math exception instead of packing up the HRESULT in a generic wil_exception. @lhecker am I crazy or does this exist?

(@j4james, not strictly necessary for this PR... but moving to the base:: numerics might make things look cleaner around these conversions.)

Yeah. We've got an issue for this somewhere I think. I definitely do want to clean this stuff up at some point, but I was trying to keep the PR focused on just moving the code out of getset.cpp and eliminating the bool returns.

miniksa · 2022-02-01T20:19:34Z

src/terminal/parser/stateMachine.cpp

+template<typename TLambda>
+bool StateMachine::_SafeExecute(TLambda&& lambda) noexcept
+try
+{
+    return lambda();
+}
+catch (...)
+{
+    LOG_HR(wil::ResultFromCaughtException());
+    return false;
+}
+
+template<typename TLambda>
+bool StateMachine::_SafeExecuteWithLog(const wchar_t wch, TLambda&& lambda) noexcept
+{
+    const bool success = _SafeExecute(std::forward<TLambda>(lambda));
+    if (!success)
+    {
+        TermTelemetry::Instance().LogFailed(wch);
+    }
+    return success;
+}


(maybe something like this belongs in til... but that can be for future consideration)

Yeah, that's a good point.

miniksa · 2022-02-01T20:20:38Z

src/terminal/parser/stateMachine.cpp

+// - string - Characters to dispatch.
+// Return Value:
+// - <none>
+void StateMachine::_ActionPrintString(const std::wstring_view string)


Yeah I mean in hindsight that feels obvious to put this as a helper. hahaha.

zadjii-msft

I'm at 21/24 but figured if Niksa sent his feedback I should chime in real quick. Sorry for the delays here - release engineering is always a bit hectic

zadjii-msft · 2022-01-26T16:00:05Z

src/host/outputStream.cpp

+void ConhostInternalGetSet::SetScrollingRegion(const SMALL_RECT& scrollMargins)
 {
-    return NT_SUCCESS(DoSrvPrivateSetScrollingRegion(_io.GetActiveOutputBuffer(), scrollMargins));
+    auto& screenInfo = _io.GetActiveOutputBuffer();


note to self: did we keep the STATUS_INVALID_PARAMETER check when Top > Bottom?

Yeah, see my comment to @miniksa above - all the validation is already handled in AdaptDispatch::_DoSetTopBottomScrollingMargins.

zadjii-msft · 2022-01-26T16:07:20Z

src/host/outputStream.cpp

+                                         const bool standardFillAttrs)
+{
+    auto& screenInfo = _io.GetActiveOutputBuffer();
+


note to self: this had a lock before. Does ::ScrollRegion take the lock itself? Was the lock redundant?

This code is only triggered by a VT sequence, so we would have already obtained the lock in the ApiRoutines::WriteConsoleWImpl method.

I don't think the original code actually needed it, but functions in getset.cpp were often also used in console API calls (which would have required a lock), so it was probably just following the pattern of other functions there.

j4james · 2022-02-03T01:51:52Z

I figure all the things that are like "always returning true" will end up getting worked out into something more sensible as this refactoring continues into its next phases, so I didn't cry about it.

I'm not sure how this will ultimately work out, but if we get to the point where we can rely on #1173 exclusively, then we shouldn't ever need to pass-through individual sequences, and all of the bool returns could eventually be dropped.

But even if we don't get to that, I'm of the opinion that we'd be better off only passing through selective sequences that we know to be safe, and handling that via the VtEngine rather than the current pass-through mechanism. Again that would eliminate the remaining bool returns.

For now, though, if we're going to indicate pass-through via a bool return, I think it's easier to just keep all the dispatch methods following that pattern.

ghost · 2022-02-04T12:00:04Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This PR updates the `ITerminalApi` and `TerminalDispatch` classes to allow exceptions to be thrown in case of errors instead of using boolean return values. ## References This brings the Terminal code into alignment with the `AdaptDispatch` and `ConGetSet` changes made in PR #12247. And while this isn't exactly a fix for #12378, it does at least stop the app from crashing now. ## Detailed Description of the Pull Request / Additional comments All the `TerminalDispatch` methods have had their `noexcept` specifiers dropped, and any `try`/`catch` wrapping removed, so exceptions will now fall through to the `StateMachine` class where they should be safely caught and logged. The same goes for the `ITerminalApi` interface and its implementation in the `Terminal` class. And many of the methods in this interface have also had their `bool` return values changed to `void`, since there is usually not a need for error return values now. ## Validation Steps Performed I've manually tested the `OSC 9;9` sequence described in #12378 and confirmed that it no longer crashes.

This PR updates the `ITerminalApi` and `TerminalDispatch` classes to allow exceptions to be thrown in case of errors instead of using boolean return values. ## References This brings the Terminal code into alignment with the `AdaptDispatch` and `ConGetSet` changes made in PR #12247. And while this isn't exactly a fix for #12378, it does at least stop the app from crashing now. ## Detailed Description of the Pull Request / Additional comments All the `TerminalDispatch` methods have had their `noexcept` specifiers dropped, and any `try`/`catch` wrapping removed, so exceptions will now fall through to the `StateMachine` class where they should be safely caught and logged. The same goes for the `ITerminalApi` interface and its implementation in the `Terminal` class. And many of the methods in this interface have also had their `bool` return values changed to `void`, since there is usually not a need for error return values now. ## Validation Steps Performed I've manually tested the `OSC 9;9` sequence described in #12378 and confirmed that it no longer crashes. (cherry picked from commit 9c11e02)

This is an attempt to simplify the `ConGetSet` interface down to the smallest set of methods necessary to support the `AdaptDispatch` class. The idea is that it should then be easier to implement that interface in Windows Terminal, so we can replace the `TerminalDispatch` class with `AdaptDispatch`. This is a continuation of the refactoring started in #12247, and a significant step towards #3849. ## Detailed Description of the Pull Request / Additional comments The general idea was to give the `AdaptDispatch` class direct access to the high-level structures on which it needs to operate. Some of these structures are now passed in when the class is constructed (the `Renderer`, `RenderSettings`, and `TerminalInput`), and some are exposed as new methods in `ConGetSet` (`GetStateMachine`, `GetTextBuffer`, and `GetViewport`). Many of the existing `ConhostInternalGetSet` methods could easily then be reimplemented in `AdaptDispatch`, since they were often simply forwarding to methods in one of the above structures. Some were a little more complicated, though, and require further explanation. * `GetConsoleScreenBufferInfoEx`: What we were typically using this for was to obtain the viewport, although what we really wanted was the virtual viewport, which is now accessible via the `GetViewport` method. This was also used to obtain the cursor position and buffer width, which we can now get via the `GetTextBuffer` method. * `SetConsoleScreenBufferInfoEx`: This was only really used for the `AdaptDispatch::SetColumns` implementation (for `DECCOLM` mode), and that could be replaced with `ResizeWindow`. This is a slight change in behaviour (it sizes the window rather than the buffer), but neither is technically correct for `DECCOLM`, so I think it's good enough for now, and at least it's consistent with the other VT sizing operations. * `SetCursorPosition`: This could mostly be replaced with direct manipulation of the `Cursor` object (accessible via the text buffer), although again this is a slight change in behavior. The original code would also have made a call to `ConsoleImeResizeCompStrView` (which I don't think is applicable to VT movement), and would potentially have moved the viewport (not essential for now, but could later be supported by `DECHCCM`). It also called `VtIo::SetCursorPosition` to handle cursor inheritance, but that should only apply to `InteractDispatch`, so I've moved that to the `InteractDispatch::MoveCursor` method. * `ScrollRegion`: This has been replaced by two simple helper methods in `AdaptDispatch` which better meet the VT requirements - `_ScrollRectVertically` and `_ScrollRectHorizontally`. Unlike the original `ScrollRegion` implementation, these don't generate `EVENT_CONSOLE_UPDATE_SCROLL` events (see #12656 for more details). * `FillRegion`: This has been replaced by the `_FillRect` helper method in `AdaptDispatch`. It differs from the original `FillRegion` in that it takes a rect rather than a start position and length, which gives us more flexibility for future operations. * `ReverseLineFeed`: This has been replaced with a somewhat refactored reimplementation in `AdaptDispatch`, mostly using the `_ScrollRectVertically` helper described above. * `EraseAll`: This was previously handled by `SCREEN_INFORMATION::VtEraseAll`, but has now been entirely reimplemented in the `AdaptDispatch::_EraseAll` method. * `DeleteLines`/`InsertLines`/`_modifyLines`: These have been replaced by the `_InsertDeleteLineHelper` method in `AdaptDispatch`, which mostly relies on the `_ScrollRectVertically` helper described above. Finally there were a few methods that weren't actually needed in the `ConGetSet` interface: * `MoveToBottom`: This was really just a hack to get the virtual viewport from `GetConsoleScreenBufferInfoEx`. We may still want something like in the future (e.g. to support `DECVCCM` or #8879), but I don't think it's essential for now. * `SuppressResizeRepaint`: This was only needed in `InteractDispatch` and `PtySignalInputThread`, and they could easily access the `VtIo` object to implement it themselves. * `ClearBuffer`: This was only used in `PtySignalInputThread`, and that could easily access the buffer directly via the global console information. * `WriteControlInput`: This was only used in `InteractDispatch`, and that could easily be replaced with a direct call to `HandleGenericKeyEvent`. As part of these changes, I've also refactored some of the existing `AdaptDispatch` code: * `_InsertDeleteHelper` (renamed `_InsertDeleteCharacterHelper`) is now just a straightforward call to the new `_ScrollRectHorizontally` helper. * `EraseInDisplay` and `EraseInLine` have been implemented as a series of `_FillRect` calls, so `_EraseSingleLineHelper` is no longer required. * `_EraseScrollback` is a essentially a special form of scrolling operation, which mostly depends on the `TextBuffer::ScrollRows` method, and with the filling now provided by the new `_FillRect` helper. * There are quite a few operations now in `AdaptDispatch` that are affected by the scrolling margins, so I've pulled out the common margin setup into a new `_GetVerticalMargins` helper method. This also fixes some edge cases where margins could end up out of range. ## Validation Steps Performed There were a number of unit tests that needed to be updated to work around functions that have now been removed, but these substitutions were fairly straightforward for the most part. The adapter tests were a different story, though. In that case we were explicitly testing how operations were passed through to the `ConGetSet` interface, but with more than half those methods now gone, a significant rewrite was required. I've tried to retain the crux of the original tests, but we now have to validate the state changes on the underlying data structures, where before that state would have been tracked in the `TestGetSet` mock. And in some cases we were testing what happened when a method failed, but since that scenario is no longer possible, I've simply removed those tests. I've also tried to manually test all the affected operations to confirm that they're still working as expected, both in vttest as well as my own test scripts. Closes #12662

ghost · 2022-05-24T17:04:28Z

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

Handy links:

j4james added 29 commits January 25, 2022 14:52

Remove DoSrvPrivateWriteConsoleInputW.

e9a3992

Remove DoSrvPrivateSetAutoWrapMode.

ce0f0d1

Remove DoSrvPrivateShowCursor.

3eb03bd

Remove DoSrvPrivateAllowCursorBlinking.

e5b141c

Remove DoSrvPrivateSetScrollingRegion.

841a357

Remove DoSrvPrivateLineFeed.

d5cc106

Remove DoSrvPrivateReverseLineFeed.

e1992ed

Remove DoSrvPrivateUseAlternateScreenBuffer.

6d13028

Remove DoSrvPrivateUseMainScreenBuffer.

5568c9e

Remove DoSrvPrivateEraseAll.

7b05ba5

Remove DoSrvPrivateClearBuffer.

662321d

Remove DoSrvSetCursorStyle.

91edb2b

Remove DoSrvPrivateRefreshWindow.

cff545d

Remove DoSrvPrivateWriteConsoleControlInput.

c1d60c7

Remove DoSrvGetConsoleOutputCodePage.

b3ee852

Remove DoSrvPrivateSuppressResizeRepaint.

1e9796b

Remove DoSrvIsConsolePty.

e19f603

Remove DoSrvPrivateDeleteLines and DoSrvPrivateInsertLines.

190edc3

Remove DoSrvPrivateMoveToBottom.

4ef5075

Remove DoSrvPrivateFillRegion.

4368bdd

Remove DoSrvPrivateScrollRegion.

dee2974

Remove DoSrvAddHyperlink and DoSrvEndHyperlink.

1ddf056

Remove DoSrvUpdateSoftFont.

0340543

Remove DoSrvSetConsoleTitleW.

6eadadb

Clean up ConGetSet method names.

38a5b50

Add exception trapping in the state machine.

407ec4b

Remove exception trapping downstream of state machine.

4af4b2f

Only return bool to indicate conpty passthrough.

f0da56a

Update adapter tests to handle API changes.

a3a305b

ghost added the Area-VT Virtual Terminal sequence support label Jan 25, 2022

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Jan 25, 2022

j4james marked this pull request as ready for review January 25, 2022 17:47

miniksa self-assigned this Jan 27, 2022

miniksa reviewed Jan 28, 2022

View reviewed changes

Merge branch 'main' into refactor-congetset

576b6bb

miniksa approved these changes Feb 1, 2022

View reviewed changes

zadjii-msft reviewed Feb 1, 2022

View reviewed changes

j4james mentioned this pull request Feb 1, 2022

Can we get rid of the AdaptDefaults interface? #12318

Closed

zadjii-msft approved these changes Feb 4, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 4, 2022

ghost merged commit 5238235 into microsoft:main Feb 4, 2022

j4james deleted the refactor-congetset branch February 5, 2022 11:20

This was referenced Feb 6, 2022

Empty path to OSC 9;9; crashes Terminal window #12378

Closed

Allow exceptions from ITerminalApi and TerminalDispatch #12432

Merged

This was referenced Mar 10, 2022

Further simplification of the ConGetSet interface #12662

Closed

Further refactor and simplify the ConGetSet API #12703

Merged

This was referenced May 24, 2022

DECAWM escape sequence doesn't work correctly in non-active buffer #12194

Closed

IL esape sequence doesn't work correctly in non-active buffer #12193

Closed

This pull request was closed.

Conversation

j4james commented Jan 25, 2022

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

j4james commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miniksa commented Jan 28, 2022

Uh oh!

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

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!

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!

zadjii-msft left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

j4james commented Feb 3, 2022

Uh oh!

ghost commented Feb 4, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zadjii-msft left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.