Further simplification of the ConGetSet interface #12662
Assignees
Labels
Area-CodeHealth
Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Issue-Feature
Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Product-Meta
The product is the management of the products.
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
j4james
added
the
Issue-Feature
ghost
added
Needs-Triage
|
Love it! I'm personally entirely fine with reviewing large PRs. The amount of work is the same no matter how many PRs it's split up in after all. I'm not sure whether we could merge something like that within this month. In general however, I'd be absolutely looking forward to this! ...even if this is absolutely shredding my |
DHowett
added
Product-Meta
|
I am absolute in favor of this. Thank you 😄 |
I was kind of worried about that. I hate to ask this, but it would be helpful if we could delay those changes until after |
|
@j4james I'll probably open a draft PR with my changes in April, since I already have them ready anyways (and am just continuing to rebase it on main). I can wait until after your changes are merged to actually open it for review. 🙂 |
|
Oh, I think I misunderstood you. I was under the impression that PR #12207 was your |
|
@j4james If you have to start from scratch, I can just wait with my PR. #12207 indeed is only about 20% of my changes, but almost all of them are basic type and member renames throughout the code base. This makes it it's trivial to rebase it, so please go ahead with your changes, as I'll simply rebase my stuff later. 🙂 |
ghost
pushed a commit
that referenced
this issue
Apr 14, 2022
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
added
the
Resolution-Fix-Committed
6 tasks
ghost
pushed a commit
that referenced
this issue
May 4, 2022
## Summary of the Pull Request This PR replaces the `TerminalDispatch` class with the `AdaptDispatch` class from conhost, so we're no longer duplicating the VT functionality in two places. It also gives us a more complete VT implementation on the Terminal side, so it should work better in pass-through mode. ## References This is essentially part two of PR #12703. ## PR Checklist * [x] Closes #3849 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12662 ## Detailed Description of the Pull Request / Additional comments The first thing was to give the `ConGetSet` interface a new name, since it's now no longer specific to conhost. I went with `ITerminalApi`, since that was the equivalent interface on the terminal side, and it still seemed like a generic enough name. I also changed the way the api is managed by the `AdaptDispatch` class, so it's now stored as a reference rather than a `unique_ptr`, which more closely matches the way the `TerminalDispatch` class worked. I then had to make sure that `AdaptDispatch` actually included all of the functionality currently in `TerminalDispatch`. That meant copying across the code for bracketed paste mode, the copy to clipboard operation, and the various ConEmu OSC operations. This also required a few new methods to the `ConGetSet`/`ITerminalApi` interface, but for now these are just stubs in conhost. Then there were a few thing in the api interface that needed cleaning up. The `ReparentWindow` method doesn't belong there, so I've moved that into `PtySignalInputThread` class. And the `WriteInput` method was too low-level for the Terminal requirements, so I've replaced that with a `ReturnResponse` method which takes a `wstring_view`. It was then a matter of getting the `Terminal` class to implement all the methods in the new `ITerminalApi` interface that it didn't already have. This was mostly mapping to existing functionality, but there are still a number of methods that I've had to leave as stubs for now. However, what we have is still good enough that I could then nuke the `TerminalDispatch` class from the Terminal code and replace it with `AdaptDispatch`. One oddity that came up in testing, though, was the `AdaptDispatch` implementation of `EraseAll` would push a blank line into the scrollback when called on an empty buffer, whereas the previous terminal implementation did not. That caused problems for the conpty connection, because one of the first things it does on startup is send an `ED 2` sequence. I've now updated the `AdaptDispatch` implementation to match the behavior of the terminal implementation in that regard. Another problem was that the terminal implementation of the color table commands had special handling for the background color to notify the application window that it needed to repaint the background. I didn't want to have to push the color table operations through the `ITerminalApi` interface, so I've instead moved the handling of the background update into the renderer, initiated by a flag on the `TriggerRefreshAll` method. ## Validation Steps Performed Surprisingly this PR didn't require a lot of changes to get the unit tests working again. There were just a few methods used from the original `ITerminalApi` that have now been removed, and which needed an equivalent replacement. Also the updated behavior of the `EraseAll` method in conhost resulted in a change to the expected cursor position in one of the screen buffer tests. In terms of manual testing, I've tried out all the different shells in Windows Terminal to make sure there wasn't anything obviously wrong. And I've run a bunch of the tests from _vttest_ to try and get a wider coverage of the VT functionality, and confirmed everything still works at least as well as it used to. I've also run some of my own tests to verify the operations that had to be copied from `TerminalDispatch` to `AdaptDispatch`.
|
🎉This issue was addressed in #12703, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description of the new feature/enhancement
I've been working for a while now on a refactoring the
ConhostInternalGetSetcode (as a follow up to PR #12247), trying to move as much as possible into theAdaptDispatchclass, and condensing theConGetSetinterface down to a minimal set of operations.The idea is that the the
AdaptDispatchclass could then be used in Windows Terminal in place of the existingTerminalDispatch(i.e. issue #3849), and only have a handful of methods in theConGetSetinterface needing terminal-specific implementations.I'm not done yet, but I'm far enough along that I'm satisfied that this is the right approach, and I wanted to let others know my plans, and check whether you're happy with what I'm doing.
Proposed technical implementation details (optional)
The general idea is that we provide the means for
AdaptDispatchto access the high-level structures that are common to both conhost and terminal (like theRendererand theTextBuffer), and then a lot of the operations likeEraseAll,InsertLines, andScrollRegioncan be implemented entirely inAdaptDispatch, avoiding the need to implement them twice.In some cases, the structures we need are static, and can be passed through to
AdaptDispatchin the constructor (e.g. theRenderer,RenderSettings, andTerminalInput). Others are dynamic, but can easily be retrieve via new methods in theConGetSetinterface (e.g. the activeTextBuffer, and the active virtual viewport).Taking this approach, I've so far been able to eliminate 31 methods from the
ConGetSetinterface (adding only 4 new ones), and I think there are still more that could be nuked eventually.My current plan was to submit this as two PRs: the first covering the refactoring of
ConhostInternalGetSet, and the second dealing with the reuse ofAdaptdispatchin the terminal (I haven't looked at that in much detail yet, so it might be more complicated than I imagine). Even just the first bit is quite a large change, though, so maybe you'd prefer it split up even more?The text was updated successfully, but these errors were encountered: