CNL and CPL should be constrained by margins #2926
Labels
Area-VT
Virtual Terminal sequence support
Help Wanted
We encourage anyone to jump in on these.
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Product-Conhost
For issues in the Console codebase
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
ghost
added
Needs-Triage
DHowett-MSFT
added
Area-VT
|
Yeah, matching XTerm and VTE sounds correct to me! |
5 tasks
ghost
pushed a commit
that referenced
this issue
Jan 16, 2020
## Summary of the Pull Request Originally there were 3 different methods for implementing VT cursor movement, and between them they still couldn't handle some of the operations correctly. This PR unifies those operations into a single method that can handle every type of cursor movement, and which fixes some of the issues with the existing implementations. In particular it fixes the `CNL` and `CPL` operations, so they're now correctly constrained by the `DECSTBM` margins. ## References If this PR is accepted, the method added here should make it trivial to implement the `VPR` and `HPR` commands in issue #3428. ## PR Checklist * [x] Closes #2926 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments The new [`AdaptDispatch::_CursorMovePosition`](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.cpp#L169) method is based on the proposal I made in issue #3428 for the `VPR` and `HPR` comands. It takes three arguments: a row offset (which can be absolute or relative), a column offset (ditto), and a flag specifying whether the position should be constrained by the `DECSTBM` margins. To make the code more readable, I've implemented the offsets using [a `struct` with some `constexpr` helper functions for the construction](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.hpp#L116-L125). This lets you specify the parameters with expressions like `Offset::Absolute(col)` or `Offset::Forward(distance)` which I think makes the calling code a little easier to understand. While implementing this new method, I noticed a couple of issues in the existing movement implementations which I thought would be good to fix at the same time. 1. When cursor movement is constrained horizontally, it should be constrained by the buffer width, and not the horizontal viewport boundaries. This is an issue I've previously corrected in other parts of the codebase, and I think the cursor movement was one of the last areas where it was still a problem. 2. A number of the commands had range and overflow checks for their parameters that were either unnecessary (testing for a condition that could never occur) or incorrect (if an operation overflows, the correct behavior is to clamp it, and not just fail). The new implementation handles legitimate overflows correctly, but doesn't check for impossible ranges. Because of the change of behavior in point 1, I also had to update the implementations of [the `DECSC` and `CPR` commands](9cf7a9b) to account for the column offset now being relative to the buffer and not the viewport, otherwise those operations would no longer work correctly. ## Validation Steps Performed Because of the two changes in behavior mentioned above, there were a number of adapter tests that stopped working and needed to be updated. First off there were those that expected the column offset to be relative to the left viewport position and constrained by the viewport width. These now had to be updated to [use the full buffer width](49887a3) as the allowed horizontal extent. Then there were all the overflow and out-of-range tests that were testing conditions that could never occur in practice, or where the expected behavior that was tested was actually incorrect. I did spend some time trying to see if there was value in updating these tests somehow, but in the end I decided it was best to just [drop them](6e80d0d) altogether. For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](d6c4f35) to check that those operations now work correctly, both with and without margins.
ghost
added
Resolution-Fix-Committed
DHowett-MSFT
pushed a commit
that referenced
this issue
Jan 26, 2020
## Summary of the Pull Request Originally there were 3 different methods for implementing VT cursor movement, and between them they still couldn't handle some of the operations correctly. This PR unifies those operations into a single method that can handle every type of cursor movement, and which fixes some of the issues with the existing implementations. In particular it fixes the `CNL` and `CPL` operations, so they're now correctly constrained by the `DECSTBM` margins. ## References If this PR is accepted, the method added here should make it trivial to implement the `VPR` and `HPR` commands in issue #3428. ## PR Checklist * [x] Closes #2926 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments The new [`AdaptDispatch::_CursorMovePosition`](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.cpp#L169) method is based on the proposal I made in issue #3428 for the `VPR` and `HPR` comands. It takes three arguments: a row offset (which can be absolute or relative), a column offset (ditto), and a flag specifying whether the position should be constrained by the `DECSTBM` margins. To make the code more readable, I've implemented the offsets using [a `struct` with some `constexpr` helper functions for the construction](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.hpp#L116-L125). This lets you specify the parameters with expressions like `Offset::Absolute(col)` or `Offset::Forward(distance)` which I think makes the calling code a little easier to understand. While implementing this new method, I noticed a couple of issues in the existing movement implementations which I thought would be good to fix at the same time. 1. When cursor movement is constrained horizontally, it should be constrained by the buffer width, and not the horizontal viewport boundaries. This is an issue I've previously corrected in other parts of the codebase, and I think the cursor movement was one of the last areas where it was still a problem. 2. A number of the commands had range and overflow checks for their parameters that were either unnecessary (testing for a condition that could never occur) or incorrect (if an operation overflows, the correct behavior is to clamp it, and not just fail). The new implementation handles legitimate overflows correctly, but doesn't check for impossible ranges. Because of the change of behavior in point 1, I also had to update the implementations of [the `DECSC` and `CPR` commands](9cf7a9b) to account for the column offset now being relative to the buffer and not the viewport, otherwise those operations would no longer work correctly. ## Validation Steps Performed Because of the two changes in behavior mentioned above, there were a number of adapter tests that stopped working and needed to be updated. First off there were those that expected the column offset to be relative to the left viewport position and constrained by the viewport width. These now had to be updated to [use the full buffer width](49887a3) as the allowed horizontal extent. Then there were all the overflow and out-of-range tests that were testing conditions that could never occur in practice, or where the expected behavior that was tested was actually incorrect. I did spend some time trying to see if there was value in updating these tests somehow, but in the end I decided it was best to just [drop them](6e80d0d) altogether. For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](d6c4f35) to check that those operations now work correctly, both with and without margins. (cherry picked from commit 2fec178)
|
🎉This issue was addressed in #3628, which has now been successfully released as Handy links: |
|
🎉This issue was addressed in #3628, 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
Environment
Windows build number: Version 10.0.18362.295
Also test with a recent commit 7faf334
Steps to reproduce
In a conhost WSL shell, execute the following command:
This does the following:
DECSTBMmargins to 6 and 19CNLsequence with a count of 99, to move down 99 linesCPLsequence with a count of 99, to move up 99 linesExpected behavior
I can't find spec text to back this up, but in both XTerm and the Gnome VTE terminal these commands are constrained by the
DECSTBMmargins, soCNLdoesn't move below row 19 (the bottom margin), andCPLdoesn't move above row 6 (the top margin).This is what the output looks like in XTerm:
Actual behavior
The margins are ignored by the Windows console, so
CNLmoves all the way to the bottom of the viewport, andCPLmoves all the way to the top of the viewport.I can't say for certain that this is wrong, but I think we should probably be trying to match the XTerm and VTE behaviour unless there is convincing evidence that they've got it wrong.
The text was updated successfully, but these errors were encountered: