Remove unwanted DECSTBM clipping #2764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rk outside the DECSTBM margin boundaries.
…t affected by the DECSTBM margins.
|
FYI, the build failure is in the |
…e margins can persist into following tests.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DHowett-MSFT
approved these changes
Sep 23, 2019
|
@j4james thank you, as always. |
|
🎉 Handy links: |
ghost
mentioned this pull request
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 26, 2019
The `DECSTBM` margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the `ICH` and `DCH` escape sequences (which are only affected by the horizontal margins, which we don't yet support), the `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This commit moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. With the margin clipping removed from the `ScrollRegion` function, it now had to be applied manually in the places it was actually required. This included: * The `DoSrvPrivateReverseLineFeed` function (for the `RI` control): This was * just a matter of updating the bottom of the scroll rect to the bottom margin * (at least when the margins were actually set), since the top of the scroll * rect would always be the top of the viewport. The * `DoSrvPrivateModifyLinesImpl` function (for the `IL` and `DL` commands): * Again this was just a matter of updating the bottom of the scroll rect, since * the cursor position would always determine the top of the scroll rect. The * `AdaptDispatch::_ScrollMovement` method (for the `SU` and `SD` commands): * This required updating both the top and bottom coordinates of the scroll * rect, and also a simpler destination Y coordinate (the way the `ScrollRegion` * function worked before, the caller was expected to take the margins into * account when determining the destination). On the plus side, there was now no longer a need to override the margins when calling `ScrollRegion` in the `AdjustCursorPosition` function. In the first case, the margins had needed to be cleared (_stream.cpp 143-145), but that is now the default behaviour. In the second case, there had been a more complicated adjustment of the margins (_stream.cpp 196-209), but that code was never actually used so could be removed completely (to get to that point either _fScrollUp_ was true, so _scrollDownAtTop_ couldn't also be true, or _fScrollDown_ was true, but in that case there is a check to make sure _scrollDownAtTop_ is false). While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so I've fixed that now as well. ## Validation Steps Performed For commands like `RI`, `IL`, `DL`, etc. where we've changed the implementation but not the behaviour, there were already unit tests that could confirm that the new implementation was still producing the correct results. Where there has been a change in behaviour - namely for the `ICH` and `DCH` commands, and the `ScrollConsoleScreenBuffer` API - I've extended the existing unit tests to check that they still function correctly even when the `DECSTBM` margins are set (which would previously have caused them to fail). I've also tested manually with the test cases in issues #2543 and #2659, and confirmed that they now work as expected. Closes #2543 Closes #2659 (cherry picked from commit 0c8a4df)
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 26, 2019
- Remember last-used string in the Find dialog in conhost (GH-2845) (cherry picked from commit bfb1484) - Bugfix: TextBuffer Line Wrapping from VTRenderer (GH-2797) Change VT renderer to do erase line instead of a ton of erase chars (cherry picked from commit 8afc5b2) - Add some retry support to Renderer::PaintFrame (GH-2830) If _PaintFrameForEngine returns E_PENDING, we'll give it another two tries to get itself straight. If it continues to fail, we'll take down the application. We observed that the DX renderer was failing to present the swap chain and failfast'ing when it did so; however, there are some errors from which DXGI guidance suggests we try to recover. We'll now return E_PENDING (and destroy our device resources) when we hit those errors. Fixes GH-2265. (cherry picked from commit 277acc3) - Enable VT processing by default for ConPTY (GH-2824) This change enables VT processing by default for _all_ conpty clients. See GH-1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. **Validation Steps Performed** Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes GH-1965 (cherry picked from commit 1c412d4) - Remove unwanted DECSTBM clipping (GH-2764) The `DECSTBM` margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the `ICH` and `DCH` escape sequences (which are only affected by the horizontal margins, which we don't yet support), the `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This commit moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so ... Related work items: #23507749, #23563809, #23563819, #23563837, #23563841, #23563844
|
This went out for conhost in insider build 19002! Thanks 😄 |
This was referenced Apr 10, 2021
ghost
pushed a commit
that referenced
this pull request
Apr 12, 2021
This PR removes the `GetConsoleCursorInfo` and `SetConsoleCursorInfo` methods from the `ConGetSet` interface, and the `GetScrollingRegion` method from the `SCREEN_INFORMATION` class. None of these methods are used anymore. PR #2764 removed the last usage of `GetScrollingRegion`. The `Get/SetConsoleCursorInfo` methods don't seem to have ever been used in the time that the terminal code has been open source, so whatever purpose they originally served must have been replaced a long time ago. The `GetScrollingRegion` method was originally called from the `ScrollRegion` function, but that usage was removed in PR #2764 when the margin handling was moved to a higher level. I've checked that the code still compiles. Closes #9771
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
The
DECSTBMmargins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in theScrollRegionfunction, which is also used in a number of places that shouldn't be affected byDECSTBM.This includes the
ICHandDCHescape sequences (which are only affected by the horizontal margins, which we don't yet support), theScrollConsoleScreenBufferAPI (which is public Console API, not meant to be affected by the VT terminal emulation), and theCSI 3 Jerase scrollback extension (which isn't really scrolling as such, but uses theScrollRegionfunction to manipulate the scrollback buffer).This PR moves the margin clipping out of the
ScrollRegionfunction, so it can be applied exclusively in the places that need it.PR Checklist
Detailed Description of the Pull Request / Additional comments
With the margin clipping removed from the
ScrollRegionfunction, it now had to be applied manually in the places it was actually required. This included:DoSrvPrivateReverseLineFeedfunction (for theRIcontrol): This was just a matter of updating the bottom of the scroll rect to the bottom margin (at least when the margins were actually set), since the top of the scroll rect would always be the top of the viewport.DoSrvPrivateModifyLinesImplfunction (for theILandDLcommands): Again this was just a matter of updating the bottom of the scroll rect, since the cursor position would always determine the top of the scroll rect.AdaptDispatch::_ScrollMovementmethod (for theSUandSDcommands): This required updating both the top and bottom coordinates of the scroll rect, and also a simpler destination Y coordinate (the way theScrollRegionfunction worked before, the caller was expected to take the margins into account when determining the destination).On the plus side, there was now no longer a need to override the margins when calling
ScrollRegionin theAdjustCursorPositionfunction. In the first case, the margins had needed to be cleared, but that is now the default behaviour. In the second case, there had been a more complicated adjustment of the margins, but that code was never actually used so could be removed completely (to get to that point either fScrollUp was true, so scrollDownAtTop couldn't also be true, or fScrollDown was true, but in that case there is a check to make sure scrollDownAtTop is false).While testing, I also noticed that one of the
ScrollRegioncalls in theAdjustCursorPositionfunction was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so I've fixed that now as well.Validation Steps Performed
For commands like
RI,IL,DL, etc. where we've changed the implementation but not the behaviour, there were already unit tests that could confirm that the new implementation was still producing the correct results.Where there has been a change in behaviour - namely for the
ICHandDCHcommands, and theScrollConsoleScreenBufferAPI - I've extended the existing unit tests to check that they still function correctly even when theDECSTBMmargins are set (which would previously have caused them to fail).I've also tested manually with the test cases in issues #2543 and #2659, and confirmed that they now work as expected.