-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the VT cursor movement implementation #3628
Conversation
e2e138d
to
9792498
Compare
Argh, we waited 28 days before even looking at this PR? I'm sorry. |
9792498
to
d6c4f35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. Nothing major. Thanks and apologies for the delay!
Also notes: I gave some suggestions about using I also mentally had a thought about a week or two ago that maybe we need to implement a To me, they're all in a similar bucket of attempting to convey semantic meaning with the code itself instead of having all sorts of +1/-1, pairs of offset functions, and casts all over the place following the same pattern. |
…n handle every situation, and which fixes some of the issues with the existing implementations.
…relative to the buffer and not the viewport.
…lative to the buffer and not the viewport.
d6c4f35
to
0b780ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot. Much easier to read than the mess that we had before, and having it all handled in the adapter is way better. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for answering my questions/concerns.
Hello @miniksa! Because this pull request has the 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 (
|
This commit moves the handling of the `BEL`, `BS`, `TAB`, and `CR` controls characters into the state machine (when in VT mode), instead of forwarding them on to the default string writer, which would otherwise have to parse them out all over again. This doesn't cover all the control characters, but `ESC`, `SUB`, and `CAN` are already an integral part of the `StateMachine` itself; `NUL` is filtered out by the `OutputStateMachineEngine`; and `LF`, `FF`, and `VT` are due to be implemented as part of PR #3271. Once all of these controls are handled at the state machine level, we can strip out all the VT-specific code from the `WriteCharsLegacy` function, which should simplify it considerably. This would also let us simplify the `Terminal::_WriteBuffer` implementation, and the planned replacement stream writer for issue #780. On the conhost side, the implementation is handled as follows: * The `BS` control is dispatched to the existing `CursorBackward` method, with a distance of 1. * The `TAB` control is dispatched to the existing `ForwardTab` method, with a tab count of 1. * The `CR` control required a new dispatch method, but the implementation was a simple call to the new `_CursorMovePosition` method from PR #3628. * The `BEL` control also required a new dispatch method, as well as an additional private API in the `ConGetSet` interface. But that's mostly boilerplate code - ultimately it just calls the `SendNotifyBeep` method. On the Windows Terminal side, not all dispatch methods are implemented. * There is an existing `CursorBackward` implementation, so `BS` works OK. * There isn't a `ForwardTab` implementation, but `TAB` isn't currently required by the conpty protocol. * I had to implement the `CarriageReturn` dispatch method, but that was a simple call to `Terminal::SetCursorPosition`. * The `WarningBell` method I've left unimplemented, because that functionality wasn't previously supported anyway, and there's an existing issue for that (#4046). ## Validation Steps Performed I've added a state machine test to confirm that the updated control characters are now forwarded to the appropriate dispatch handlers. But since the actual implementation is mostly relying on existing functionality, I'm assuming that code is already adequately tested elsewhere. That said, I have also run various manual tests of my own, and confirmed that everything still worked as well as before. References #3271 References #780 References #3628 References #4046
## Summary of the Pull Request This PR adds support for the `HPR` and `VPR` escape sequences from the VT510 terminal. `HPR` moves the cursor position forward by a given number of columns, and `VPR` moves the cursor position downward by a given number of rows. They're similar in function to the `CUF` and `CUD` escape sequences, except that they're not constrained by the scrolling margins. ## References #3628 provided the new `_CursorMovePosition` method that made these operations possible ## PR Checklist * [x] Closes #3428 * [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 Most of the implementation is in the new `_CursorMovePosition` method that was created in PR #3628, so all we're really doing here is hooking up the escape sequences to call that method with the appropriate parameters. ## Validation Steps Performed I've extended the existing state machine tests for CSI cursor movement to confirm that the `HPR` and `VPR` sequences are dispatched correctly, and also added screen buffer tests to make sure the movement is clamped by the screen boundaries and not the scrolling margins (we don't yet support horizontal margins, but the test is at least in place for when we do eventually add that support). I've also checked the `HPR` and `VPR` tests in Vttest (under _Test non-VT100 / ISO-6429 cursor-movement_) and confirmed that they are now working as expected.
Since this helps with the the (common 😦) neofetch crash, I'm going to pull it into v0.8 servicing. If that proves to be the wrong choice, sorry all ;) |
## 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)
🎉 Handy links: |
🎉 Handy links: |
🎉 Once again, thanks for the contribution! This pull request was included in a set of conhost changes that was just |
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
andCPL
operations, so they're now correctly constrained by theDECSTBM
margins.References
If this PR is accepted, the method added here should make it trivial to implement the
VPR
andHPR
commands in issue #3428.PR Checklist
Detailed Description of the Pull Request / Additional comments
The new
AdaptDispatch::_CursorMovePosition
method is based on the proposal I made in issue #3428 for theVPR
andHPR
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 theDECSTBM
margins.To make the code more readable, I've implemented the offsets using a
struct
with someconstexpr
helper functions for the construction. This lets you specify the parameters with expressions likeOffset::Absolute(col)
orOffset::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.
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.
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
andCPR
commands 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 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 altogether.
For the
CNL
andCPL
operations, there didn't appear to be any existing tests, so I added some new screen buffer tests to check that those operations now work correctly, both with and without margins.