ICH and DCH shouldn't be affected by vertical margins #2543
Comments
ghost
added
Needs-Triage
DHowett-MSFT
added
Area-VT
DHowett-MSFT
removed
the
Needs-Triage
|
@j4james I've taken to assigning these bugs to you because you seem so keen on fixing them. Thanks for all the well-written and investigated issue reports! |
|
@DHowett-MSFT I'm definitely happy for you to assign these bugs to me. Just bear in mind that I might be a bit slow in getting things done, because I don't always have that much time to work on this stuff. Regarding this particular bug, there is one easy way to fix it, which is to add a check in the It's not only the I can raise additional issues to document the problems mentioned above in more detail, but I think it would be easiest to fix them all at the same time. What it comes down to is some refactoring of the Which brings me to the next complication. These changes are quite closely tied to the fixes in PR #2505, so I wouldn't be able to submit a PR for this until that one is merged. Not that that stops me from working on the solution - so there's no urgency - I just wanted you to be aware of the dependency. |
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
ghost
added
Needs-Tag-Fix
|
🎉This issue was addressed in #2764, which has now been successfully released as Handy links: |
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)
Environment
Steps to reproduce
Open a WSL conhost shell, and execute the following command:
This sets the vertical scroll margins to lines 3 to 4, inserts 5 characters at the start of the first line (using the
ICHescape sequence), and then resets the margins again.Now open a new WSL conhost shell, and execute the following command:
This sets the vertical scroll margins to lines 3 to 4, deletes 5 characters from the start of the first line (using the
DCHescape sequence), and then resets the margins again.Expected behavior
In the first case I'd expect to see the first line indented by 5 characters. In the second case I'd expect to see 5 characters deleted from the first line.
This is how those commands look in XTerm:
Actual behavior
Both commands are ignored, because they're executed above the top of the scroll margins.
Now I can't say for sure that this is the wrong behaviour. The VT510 documentation says that both
ICHandDCHhave "no effect outside the scrolling margins", but it's possible they were referring only to the horizontal margins in that particular context. If you look at the DEC STD 070 manual, the wording is a lot clearer: "If the Active Position is outside the left or right margins, this control is ignored."However, just to confuse things some more, the DEC STD 070 manual also provides a kind of pseudo code algorithm for each command, and the algorithm for
ICHseems to suggest that it is limited by both the horizontal and vertical margins, whileDCHis only affected by the horizontal margins. I suspect that might just be a bug in theICHalgorithm, but who knows.Bottom line: the documentation isn't particularly clear about this, but I do know that we don't currently match XTerm's behaviour.
The text was updated successfully, but these errors were encountered: