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

[PR #5181] [MERGED] Fix copying wrapped lines by implementing better scrolling #26159

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/5181
Author: @zadjii-msft
Created: 3/30/2020
Status: ✅ Merged
Merged: 4/9/2020
Merged by: @undefined

Base: master ← Head: dev/migrie/b/5113-with-miniksas-fix

📝 Commits (10+)

  • 4bbb63d Add tracing for circling and scrolling operations. Fix improper invalidation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled.
  • a543b1f PR feedback applied. Don't bother making string if no one is listening to ETW. Change the scroll member variable to a til::point and math that directly (and add the operators and related tests to til::point).
  • 9ba2c69 More tracing is always good
  • f10edc0 Merge remote-tracking branch 'origin/dev/miniksa/tmux_draw' into dev/migrie/b/5113-with-miniksas-fix
  • d42f960 So this works but I think it will break the EOL backspace test
  • 22bdf9b This can't possibly be right... can it?
  • 466e8fc mysteriously the existing tests all basically pass
  • 248d223 this is a simple test for the case that already worked
  • b94cfdb This test failure helps explain that this doesn't work
  • e21101b This all wroks far to shockingly well

📊 Changes

12 files changed (+1136 additions, -86 deletions)

View changed files

📝 .github/actions/spell-check/patterns/patterns.txt (+3 -0)
📝 .github/actions/spell-check/whitelist/alphabet.txt (+3 -7)
📝 .github/actions/spell-check/whitelist/whitelist.txt (+1 -0)
📝 src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp (+898 -14)
📝 src/host/ut_host/ConptyOutputTests.cpp (+8 -5)
📝 src/inc/consoletaeftemplates.hpp (+5 -0)
📝 src/renderer/vt/XtermEngine.cpp (+113 -34)
📝 src/renderer/vt/paint.cpp (+26 -14)
📝 src/renderer/vt/state.cpp (+0 -1)
📝 src/renderer/vt/tracing.cpp (+74 -9)
📝 src/renderer/vt/tracing.hpp (+5 -1)
📝 src/renderer/vt/vtrenderer.hpp (+0 -1)

📄 Description

Now that the Terminal is doing a better job of actually marking which
lines were and were not wrapped, we're not always copying lines as
"wrapped" when they should be. We're more correctly marking lines as not
wrapped, when previously we'd leave them marked wrapped.

The real problem is here in the ScrollFrame method - we'd manually
newline the cursor to make the terminal's viewport shift down to a new
line. If we had to scroll the viewport for a wrapped line, this would
cause the Terminal to mark that line as broken, because conpty would
emit an extra \n that didn't actually exist.

This more correctly implements ScrollFrame. Now, well move where we
"thought" the cursor was, so when we get to the next PaintBufferLine,
if the cursor needs to newline for the next line, it'll newline, but if
we're in the middle of a wrapped line, we'll just keep printing the
wrapped line.

A couple follow up bugs were found to be caused by the same bad logic.
See #5039 and #5161 for more details on the investigations there.

References

  • #4741 RwR, which probably made this worse
  • #5122, which I branched off of
  • #1245, #357 - a pair of other conpty wrapped lines bugs
  • #5228 - A followup issue for this PR

PR Checklist

Validation Steps Performed

  • Checked the cases from #1245, #357 to validate that they still work
  • Added more and more tests for these scenarios, and then I added MORE
    tests
  • The entire team played with this in selfhost builds

🔄 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/5181 **Author:** [@zadjii-msft](https://github.com/zadjii-msft) **Created:** 3/30/2020 **Status:** ✅ Merged **Merged:** 4/9/2020 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `dev/migrie/b/5113-with-miniksas-fix` --- ### 📝 Commits (10+) - [`4bbb63d`](https://github.com/microsoft/terminal/commit/4bbb63ddf6763869b0f0a1559625d99b2cd0d60f) Add tracing for circling and scrolling operations. Fix improper invalidation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled. - [`a543b1f`](https://github.com/microsoft/terminal/commit/a543b1ff8ac1bd2f975005d19fe7931ffe6b18ef) PR feedback applied. Don't bother making string if no one is listening to ETW. Change the scroll member variable to a til::point and math that directly (and add the operators and related tests to til::point). - [`9ba2c69`](https://github.com/microsoft/terminal/commit/9ba2c69417788c4512c85abcccac3ba7d56e664c) More tracing is always good - [`f10edc0`](https://github.com/microsoft/terminal/commit/f10edc0d959a440ddaf70a76ce9738d9a92599a4) Merge remote-tracking branch 'origin/dev/miniksa/tmux_draw' into dev/migrie/b/5113-with-miniksas-fix - [`d42f960`](https://github.com/microsoft/terminal/commit/d42f960949a8802901a152abede65280f333c56e) So this works but I think it will break the EOL backspace test - [`22bdf9b`](https://github.com/microsoft/terminal/commit/22bdf9b9874cbc161a3b2ba94b0c0c8de832530f) This can't possibly be right... can it? - [`466e8fc`](https://github.com/microsoft/terminal/commit/466e8fc654793dd655bf5ddd408b15fe8be7b964) mysteriously the existing tests all basically pass - [`248d223`](https://github.com/microsoft/terminal/commit/248d223081f68d048f95d57caa0582e2bd344de8) this is a simple test for the case that already worked - [`b94cfdb`](https://github.com/microsoft/terminal/commit/b94cfdb410da74baaeed5cfb8a5bb3941e682acf) This test failure helps explain that this doesn't work - [`e21101b`](https://github.com/microsoft/terminal/commit/e21101bd1380a6226f75881bb1110ab637feb5a7) This all wroks far to shockingly well ### 📊 Changes **12 files changed** (+1136 additions, -86 deletions) <details> <summary>View changed files</summary> 📝 `.github/actions/spell-check/patterns/patterns.txt` (+3 -0) 📝 `.github/actions/spell-check/whitelist/alphabet.txt` (+3 -7) 📝 `.github/actions/spell-check/whitelist/whitelist.txt` (+1 -0) 📝 `src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp` (+898 -14) 📝 `src/host/ut_host/ConptyOutputTests.cpp` (+8 -5) 📝 `src/inc/consoletaeftemplates.hpp` (+5 -0) 📝 `src/renderer/vt/XtermEngine.cpp` (+113 -34) 📝 `src/renderer/vt/paint.cpp` (+26 -14) 📝 `src/renderer/vt/state.cpp` (+0 -1) 📝 `src/renderer/vt/tracing.cpp` (+74 -9) 📝 `src/renderer/vt/tracing.hpp` (+5 -1) 📝 `src/renderer/vt/vtrenderer.hpp` (+0 -1) </details> ### 📄 Description Now that the Terminal is doing a better job of actually marking which lines were and were not wrapped, we're not always copying lines as "wrapped" when they should be. We're more correctly marking lines as not wrapped, when previously we'd leave them marked wrapped. The real problem is here in the `ScrollFrame` method - we'd manually newline the cursor to make the terminal's viewport shift down to a new line. If we had to scroll the viewport for a _wrapped_ line, this would cause the Terminal to mark that line as broken, because conpty would emit an extra `\n` that didn't actually exist. This more correctly implements `ScrollFrame`. Now, well move where we "thought" the cursor was, so when we get to the next `PaintBufferLine`, if the cursor needs to newline for the next line, it'll newline, but if we're in the middle of a wrapped line, we'll just keep printing the wrapped line. A couple follow up bugs were found to be caused by the same bad logic. See #5039 and #5161 for more details on the investigations there. ## References * #4741 RwR, which probably made this worse * #5122, which I branched off of * #1245, #357 - a pair of other conpty wrapped lines bugs * #5228 - A followup issue for this PR ## PR Checklist * [x] Closes #5113 * [x] Closes #5180 (by fixing DECRST 25) * [x] Closes #5039 * [x] Closes #5161 (by ensuring we only `removeSpaces` on the actual bottom line) * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * Checked the cases from #1245, #357 to validate that they still work * Added more and more tests for these scenarios, and then I added MORE tests * The entire team played with this in selfhost builds --- <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:14:19 +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#26159
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?