Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for the VPR and HPR escape sequences #3428

Closed
j4james opened this issue Nov 4, 2019 · 1 comment · Fixed by #4297
Closed

Add support for the VPR and HPR escape sequences #3428

j4james opened this issue Nov 4, 2019 · 1 comment · Fixed by #4297
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. 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
Windows vNext

Comments

@j4james
Copy link
Collaborator

j4james commented Nov 4, 2019

Description of the new feature/enhancement

The VPR (vertical position relative) and HPR (horizontal position relative) escape sequences were introduced in the VT510 terminal, and are similar to CUD and CUF, except that CUD and CUF are constrained by the vertical and horizontal margins, while VPR and HPR are not (unless the DECOM origin mode is set to relative, in which case everything is constrained by the margins).

There's probably not a lot of demand for these, since they don't appear to be widely supported, but they are required to pass some of the higher level tests in Vttest.

I should point out that the ECMA-048 standard also defines VPB and HPB operations, which are essentially the reverse of VPR and HPR. However, those don't seem to have been supported by any of the VT terminals, and they aren't supported by XTerm either, so I'm not sure they're worth adding.

Proposed technical implementation details (optional)

As mentioned above, VPR and HPR are quite similar to CUD and CUF, but not similar enough to easily share any of the existing code. So I thought this might be a good opportunity to evalute all the cursor movement operations, and see if we could find a way to make the code more reusable between them.

There are currently three different routines used for cursor movement:

None of these routines are really appropriate for the proposed VPR and HPR commands. Nor do they correctly handle the CNL and CPL commands (see issue #2926). However, I think it should be possible to unify them into a single routine that could handle all of the above cases.

Let's say we created a method that could take x and y offsets, each of which could be absolute or relative, and then also a flag specifying whether the operation should be constrained by the margins. We could then handle the various operations as follows:

Operation X Offset Y Offset Margin Constraint
CUU/CUD relative 0 relative distance yes
CUF/CUB relative distance relative 0 yes
CNL/CPL absolute 0 relative distance yes
HPA/CHA absolute column relative 0 no
VPA relative 0 absolute row no
CUP absolute column absolute row no
HPR relative distance relative 0 no
VPR relative 0 relative distance no

Note that a relative offset of 0 is used when you don't want one of the coordinates to change. And an absolute offset of 0 in the x coordinate is used to force a carriage return.

Such a routine shouldn't be any more complicated than the existing methods, but could still handle all of their functionality and more.
@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 4, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 4, 2019
@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Nov 4, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 4, 2019
@zadjii-msft zadjii-msft added this to the 21H1 milestone Nov 4, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 4, 2019
@j4james j4james mentioned this issue Nov 19, 2019
Merged
5 tasks
ghost pushed a commit that referenced this issue Jan 16, 2020
@j4james
Improve the VT cursor movement implementation (#3628)
2fec178 
## 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.
@j4james j4james mentioned this issue Jan 19, 2020
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Jan 19, 2020
@ghost ghost closed this as completed in #4297 Jan 21, 2020
ghost pushed a commit that referenced this issue Jan 21, 2020
@j4james
Add support for the HPR and VPR escape sequences (#4297)
cbb87b9 
## 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.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 21, 2020
DHowett-MSFT pushed a commit that referenced this issue Jan 26, 2020
@j4james @DHowett
Improve the VT cursor movement implementation (#3628)
95cca54 
## 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)
@ghost
Copy link

ghost commented Feb 13, 2020

🎉This issue was addressed in #4297, which has now been successfully released as Windows Terminal Preview v0.9.433.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. 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.
Projects
None yet
Milestone
Windows vNext
Development

Successfully merging a pull request may close this issue.

Add support for the HPR and VPR escape sequences j4james/terminal
3 participants
@j4james @DHowett-MSFT @zadjii-msft