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 #5122] [MERGED] Correct scrolling invalidation region for tmux in pty w/ bitmap #26115

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/5122
Author: @miniksa
Created: 3/25/2020
Status: ✅ Merged
Merged: 3/27/2020
Merged by: @undefined

Base: master ← Head: dev/miniksa/tmux_draw

📝 Commits (7)

  • 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).
  • 6061cae Part 1: Set up conpty round trip test pattern that we will use to do the test.
  • 85da64d Finish the test. It validates the movement and repaint.
  • e42965c code format pass.
  • 8560b2c Merge branch 'master' into dev/miniksa/tmux_draw
  • 3973998 Update spell whitelists, rename a few things to make spelling happier. Add the NO that Dustin called out.

📊 Changes

14 files changed (+505 additions, -20 deletions)

View changed files

📝 .github/actions/spell-check/whitelist/whitelist.txt (+4 -0)
📝 src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp (+247 -0)
📝 src/host/_stream.cpp (+27 -1)
📝 src/host/ut_host/ConptyOutputTests.cpp (+3 -0)
📝 src/inc/til/point.h (+24 -0)
📝 src/renderer/vt/XtermEngine.cpp (+13 -13)
📝 src/renderer/vt/invalidate.cpp (+2 -0)
📝 src/renderer/vt/math.cpp (+2 -2)
📝 src/renderer/vt/paint.cpp (+2 -2)
📝 src/renderer/vt/state.cpp (+1 -1)
📝 src/renderer/vt/tracing.cpp (+17 -0)
📝 src/renderer/vt/tracing.hpp (+1 -0)
📝 src/renderer/vt/vtrenderer.hpp (+1 -1)
📝 src/til/ut_til/PointTests.cpp (+161 -0)

📄 Description

Correct scrolling invalidation region for tmux in pty w/ bitmap

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.

References

Detailed Description of the Pull Request / Additional comments

  • This occurs when there is a scroll region restriction applied and a
    newline operation is performed to attempt to spin the contents of just
    the scroll region. This is a frequent behavior of tmux.
  • Right now, the Terminal doesn't support any sort of "scroll content"
    operation, so what happens here generally speaking is that the PTY in
    the ConHost will repaint everything when this happens.
  • The PTY when doing AdjustCursorPosition with a scroll region
    restriction would do the following things:
  1. Slide literally everything in the direction it needed to go to take
    advantage of rotating the circular buffer. (This would force a
    repaint in PTY as the PTY always forces repaint when the buffer
    circles.)
  2. Copy the lines that weren't supposed to move back to where they were
    supposed to go.
  3. Backfill the "revealed" region that encompasses what was supposed to
    be the newline.
  • The invalidations for the three operations above were:
  1. Invalidate the number of rows of the delta at the top of the buffer
    (this part was wrong)
  2. Invalidate the lines that got copied back into position (probably
    unnecessary, but OK)
  3. Invalidate the revealed/filled-with-spaces line (this is good).

  • When we were using a simple single rectangle for invalidation, the
    union of the top row of the buffer from 1 and the bottom row of the
    buffer from 2 (and 3 was irrelevant as it was already unioned it)
    resulted in repainting the entire buffer and all was good.

  • When we switched to a bitmap, it dutifully only repainted the top line
    and the bottom two lines as the middle ones weren't a consequence of
    intersect.

  • The logic was wrong. We shouldn't be invalidating rows-from-the-top
    for the amount of the delta. The 1 part should be invalidating
    everything BUT the lines that were invalidated in parts 2 and 3.
    (Arguably part 2 shouldn't be happening at all, but I'm not optimizing
    for that right now.)

  • So this solves it by restoring an entire screen repaint for this sort
    of slide data operation by giving the correct number of invalidated
    lines to the bitmap.

Validation Steps Performed

  • Manual validation with the steps described in #5104
  • Automatic test ConptyRoundtripTests::ScrollWithMargins.

Closes #5104

🔄 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/5122 **Author:** [@miniksa](https://github.com/miniksa) **Created:** 3/25/2020 **Status:** ✅ Merged **Merged:** 3/27/2020 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `dev/miniksa/tmux_draw` --- ### 📝 Commits (7) - [`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). - [`6061cae`](https://github.com/microsoft/terminal/commit/6061cae4c37971c1722b4f9271d12e62bcab098a) Part 1: Set up conpty round trip test pattern that we will use to do the test. - [`85da64d`](https://github.com/microsoft/terminal/commit/85da64d7548f4508aa1fcc4d29f9e6fee7dbc7d3) Finish the test. It validates the movement and repaint. - [`e42965c`](https://github.com/microsoft/terminal/commit/e42965cd4465bc5034ea5fc7d9c50ec88f94385b) code format pass. - [`8560b2c`](https://github.com/microsoft/terminal/commit/8560b2cb3e952ab05bf0fc1bbaabaa2eacc74721) Merge branch 'master' into dev/miniksa/tmux_draw - [`3973998`](https://github.com/microsoft/terminal/commit/3973998247ca3b805c24d617344e31e5302170ef) Update spell whitelists, rename a few things to make spelling happier. Add the NO that Dustin called out. ### 📊 Changes **14 files changed** (+505 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `.github/actions/spell-check/whitelist/whitelist.txt` (+4 -0) 📝 `src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp` (+247 -0) 📝 `src/host/_stream.cpp` (+27 -1) 📝 `src/host/ut_host/ConptyOutputTests.cpp` (+3 -0) 📝 `src/inc/til/point.h` (+24 -0) 📝 `src/renderer/vt/XtermEngine.cpp` (+13 -13) 📝 `src/renderer/vt/invalidate.cpp` (+2 -0) 📝 `src/renderer/vt/math.cpp` (+2 -2) 📝 `src/renderer/vt/paint.cpp` (+2 -2) 📝 `src/renderer/vt/state.cpp` (+1 -1) 📝 `src/renderer/vt/tracing.cpp` (+17 -0) 📝 `src/renderer/vt/tracing.hpp` (+1 -0) 📝 `src/renderer/vt/vtrenderer.hpp` (+1 -1) 📝 `src/til/ut_til/PointTests.cpp` (+161 -0) </details> ### 📄 Description Correct scrolling invalidation region for tmux in pty w/ bitmap 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. ## References - Introduced with #5024 ## Detailed Description of the Pull Request / Additional comments - This occurs when there is a scroll region restriction applied and a newline operation is performed to attempt to spin the contents of just the scroll region. This is a frequent behavior of tmux. - Right now, the Terminal doesn't support any sort of "scroll content" operation, so what happens here generally speaking is that the PTY in the ConHost will repaint everything when this happens. - The PTY when doing `AdjustCursorPosition` with a scroll region restriction would do the following things: 1. Slide literally everything in the direction it needed to go to take advantage of rotating the circular buffer. (This would force a repaint in PTY as the PTY always forces repaint when the buffer circles.) 2. Copy the lines that weren't supposed to move back to where they were supposed to go. 3. Backfill the "revealed" region that encompasses what was supposed to be the newline. - The invalidations for the three operations above were: 1. Invalidate the number of rows of the delta at the top of the buffer (this part was wrong) 2. Invalidate the lines that got copied back into position (probably unnecessary, but OK) 3. Invalidate the revealed/filled-with-spaces line (this is good). - When we were using a simple single rectangle for invalidation, the union of the top row of the buffer from 1 and the bottom row of the buffer from 2 (and 3 was irrelevant as it was already unioned it) resulted in repainting the entire buffer and all was good. - When we switched to a bitmap, it dutifully only repainted the top line and the bottom two lines as the middle ones weren't a consequence of intersect. - The logic was wrong. We shouldn't be invalidating rows-from-the-top for the amount of the delta. The 1 part should be invalidating everything BUT the lines that were invalidated in parts 2 and 3. (Arguably part 2 shouldn't be happening at all, but I'm not optimizing for that right now.) - So this solves it by restoring an entire screen repaint for this sort of slide data operation by giving the correct number of invalidated lines to the bitmap. ## Validation Steps Performed - Manual validation with the steps described in #5104 - Automatic test `ConptyRoundtripTests::ScrollWithMargins`. Closes #5104 --- <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:02 +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#26115
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?