starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #3628] [MERGED] Improve the VT cursor movement implementation #25449

New Issue
Open
opened 2026-01-31 09:09:35 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:09:35 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/3628
Author: @j4james
Created: 11/19/2019
Status: ✅ Merged
Merged: 1/16/2020
Merged by: @undefined

Base: master ← Head: fix-cursor-movement

📝 Commits (7)

  • 69e110f Unify all the cursor movement operations into a single method that can handle every situation, and which fixes some of the issues with the existing implementations.
  • d9fa0ab Get rid of the ConGetSet::MoveCursorVertically API which isn't used anymore.
  • 18c00f4 Update DECSC and CPR commands to account for the column offset being relative to the buffer and not the viewport.
  • 5db4159 Update cursor adapter tests to account for the column offset being relative to the buffer and not the viewport.
  • 3814ad8 Removed unnecessary and incorrect overflow/out-of-range adapter tests.
  • 0b780ba Add some screen buffer tests for the CNL and CPL escape sequences.
  • 570bbb7 Add required const to pass the audit mode.

📊 Changes

9 files changed (+189 additions, -489 deletions)

View changed files

📝 src/host/getset.cpp (+0 -61)
📝 src/host/getset.h (+0 -1)
📝 src/host/outputStream.cpp (+0 -17)
📝 src/host/outputStream.hpp (+0 -2)
📝 src/host/ut_host/ScreenBufferTests.cpp (+66 -0)
📝 src/terminal/adapter/adaptDispatch.cpp (+71 -228)
📝 src/terminal/adapter/adaptDispatch.hpp (+11 -11)
📝 src/terminal/adapter/conGetSet.hpp (+0 -2)
📝 src/terminal/adapter/ut_adapter/adapterTest.cpp (+41 -167)

📄 Description

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

  • Closes Spelling error in bot.md doc (#2926)
  • CLA signed. If not, go over here and sign the CLA
  • 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 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. 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 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 and CPL 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.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/3628 **Author:** [@j4james](https://github.com/j4james) **Created:** 11/19/2019 **Status:** ✅ Merged **Merged:** 1/16/2020 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `fix-cursor-movement` --- ### 📝 Commits (7) - [`69e110f`](https://github.com/microsoft/terminal/commit/69e110fca870f6d85eb55448f5062158ec79eee5) Unify all the cursor movement operations into a single method that can handle every situation, and which fixes some of the issues with the existing implementations. - [`d9fa0ab`](https://github.com/microsoft/terminal/commit/d9fa0ab4d6c8ca3f0e3719e177dbe7ce7b928994) Get rid of the ConGetSet::MoveCursorVertically API which isn't used anymore. - [`18c00f4`](https://github.com/microsoft/terminal/commit/18c00f4ea97327c4e19be6d646771efeeebe8c7b) Update DECSC and CPR commands to account for the column offset being relative to the buffer and not the viewport. - [`5db4159`](https://github.com/microsoft/terminal/commit/5db4159233cff11ff3b5a8acbd8b30f0aa8af9a6) Update cursor adapter tests to account for the column offset being relative to the buffer and not the viewport. - [`3814ad8`](https://github.com/microsoft/terminal/commit/3814ad8acc08d47a31ac802fe3427e1b68f725fd) Removed unnecessary and incorrect overflow/out-of-range adapter tests. - [`0b780ba`](https://github.com/microsoft/terminal/commit/0b780bacb1568f35634e1bb13ba82a9b04d4cdf4) Add some screen buffer tests for the CNL and CPL escape sequences. - [`570bbb7`](https://github.com/microsoft/terminal/commit/570bbb7878671381067c1ea3e39881c5c19f34d1) Add required const to pass the audit mode. ### 📊 Changes **9 files changed** (+189 additions, -489 deletions) <details> <summary>View changed files</summary> 📝 `src/host/getset.cpp` (+0 -61) 📝 `src/host/getset.h` (+0 -1) 📝 `src/host/outputStream.cpp` (+0 -17) 📝 `src/host/outputStream.hpp` (+0 -2) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+66 -0) 📝 `src/terminal/adapter/adaptDispatch.cpp` (+71 -228) 📝 `src/terminal/adapter/adaptDispatch.hpp` (+11 -11) 📝 `src/terminal/adapter/conGetSet.hpp` (+0 -2) 📝 `src/terminal/adapter/ut_adapter/adapterTest.cpp` (+41 -167) </details> ### 📄 Description ## 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](https://github.com/microsoft/terminal/pull/3628/commits/9cf7a9b577ed7831908bb9353d4f8e0a6e6fcc5e) 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](https://github.com/microsoft/terminal/pull/3628/commits/49887a3589169b2724f4046c1773836384543c10) 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](https://github.com/microsoft/terminal/pull/3628/commits/6e80d0de19cb3313cfdd4fea555f6be41cc9fcd8) altogether. For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](https://github.com/microsoft/terminal/pull/3628/commits/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884) to check that those operations now work correctly, both with and without margins. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:09:35 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#25449
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?