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

[PR #5319] [MERGED] Show a double width cursor for double width characters #26257

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/5319
Author: @leonMSFT
Created: 4/10/2020
Status: ✅ Merged
Merged: 4/15/2020
Merged by: @undefined

Base: master ← Head: dev/lelian/bigcursor

📝 Commits (10+)

  • a2dc867 first lets add back the code that crashes
  • 56392f6 adding a test that may or may not work
  • b86d85e test case
  • 9e149cb i just add debug for now
  • 7574e9c Merge branch 'master' into dev/lelian/bigcursor
  • 5fcbac7 mAYBE
  • 32af07a ridding myself of comments
  • 69b0f27 y fail if it wouldn't matter hmm
  • 21fd1b3 why don't we just take these calls and place them over here
  • 8b46679 Merge branch 'master' into dev/lelian/bigcursor

📊 Changes

7 files changed (+67 additions, -13 deletions)

View changed files

📝 src/buffer/out/textBuffer.cpp (+1 -7)
📝 src/cascadia/TerminalCore/Terminal.cpp (+9 -1)
📝 src/cascadia/TerminalCore/Terminal.hpp (+3 -2)
📝 src/cascadia/TerminalCore/terminalrenderdata.cpp (+4 -2)
📝 src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp (+43 -0)
📝 src/host/screenInfo.cpp (+6 -0)
📝 src/renderer/inc/IRenderData.hpp (+1 -1)

📄 Description

Summary of the Pull Request

This PR will allow the cursor to be double width when on top of a double width character. This required changing IsCursorDoubleWidth to check whether the glyph the cursor's on top of is double width. This code is exactly the same as the original PR that addressed this issue in #2932. That one got reverted at some point due to the crashes related to it, but due to a combination of Terminal having come further since that PR and other changes to address use-after-frees, some of the crashes may/may not be relevant now. The ones that seemed to be relevant/repro-able, I attempt to address in this PR.

The IsCursorDoubleWidth check would fail during the TextBuffer::Reflow call inside of Terminal::UserResize occasionally, particularly when newCursor.EndDeferDrawing() is called. This is because when we tell the newCursor to EndDefer, the renderer will attempt to redraw the cursor. As part of this redraw, it'll ask if IsCursorDoubleWidth, and if the renderer managed to ask this before UserResize swapped out the old buffer with the new one from Reflow, the renderer will be asking the old buffer if its out-of-bounds cursor is double width. This was pretty easily repro'd using cmatrix -u0 and resizing the window like a madman.

As a solution, I've moved the Start/End DeferDrawing calls out of Reflow and into UserResize. This way, I can "clamp" the portion of the code where the newBuffer is getting created and reflowed and swapped into the Terminal buffer, and only allow the renderer to draw once the swap is done. This also means that ConHost's ResizeWithReflow needed to change slightly.

In addition, I've added a WriteLock to SetCursorOn. It was mentioned as a fix for a crash in #2965 (although I can't repro), and I also figured it would be good to try to emulate where ConHost locks with regards to Cursor operations, and this seemed to be one that we were missing.

PR Checklist

Validation Steps Performed

Manual validation that the cursor is indeed chonky, added a test case to check that we are correctly saying that the cursor is double width (not too sure if I put it in the right place). Also open to other test case ideas and thoughts on what else I should be careful for since I am quite nervous about what other crashes might occur.

🔄 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/5319 **Author:** [@leonMSFT](https://github.com/leonMSFT) **Created:** 4/10/2020 **Status:** ✅ Merged **Merged:** 4/15/2020 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `dev/lelian/bigcursor` --- ### 📝 Commits (10+) - [`a2dc867`](https://github.com/microsoft/terminal/commit/a2dc867dfa906cff66da3e804c2085b9a162a9d8) first lets add back the code that crashes - [`56392f6`](https://github.com/microsoft/terminal/commit/56392f62befa024693ec6156e4bcf042598fc12b) adding a test that may or may not work - [`b86d85e`](https://github.com/microsoft/terminal/commit/b86d85ed72366a26ae6bef09321ee9dbb2497bcb) test case - [`9e149cb`](https://github.com/microsoft/terminal/commit/9e149cbfe56239eaed8b2a3ecb3f840f0c1b8457) i just add debug for now - [`7574e9c`](https://github.com/microsoft/terminal/commit/7574e9c5c41a3b6192830b3d8c5930c242343547) Merge branch 'master' into dev/lelian/bigcursor - [`5fcbac7`](https://github.com/microsoft/terminal/commit/5fcbac7a3505240cc2d4c30e833e181a1dcf0596) mAYBE - [`32af07a`](https://github.com/microsoft/terminal/commit/32af07a3847e5c31e1ff7a1b139a5f1e66610b87) ridding myself of comments - [`69b0f27`](https://github.com/microsoft/terminal/commit/69b0f27edacb730fb3b9a0e599ab462c3c0bd2b5) y fail if it wouldn't matter hmm - [`21fd1b3`](https://github.com/microsoft/terminal/commit/21fd1b3476e26763f66163282ec929473dc08693) why don't we just take these calls and place them over here - [`8b46679`](https://github.com/microsoft/terminal/commit/8b46679d428405745c142c91b45eeb984c71ec04) Merge branch 'master' into dev/lelian/bigcursor ### 📊 Changes **7 files changed** (+67 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `src/buffer/out/textBuffer.cpp` (+1 -7) 📝 `src/cascadia/TerminalCore/Terminal.cpp` (+9 -1) 📝 `src/cascadia/TerminalCore/Terminal.hpp` (+3 -2) 📝 `src/cascadia/TerminalCore/terminalrenderdata.cpp` (+4 -2) 📝 `src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp` (+43 -0) 📝 `src/host/screenInfo.cpp` (+6 -0) 📝 `src/renderer/inc/IRenderData.hpp` (+1 -1) </details> ### 📄 Description # Summary of the Pull Request This PR will allow the cursor to be double width when on top of a double width character. This required changing `IsCursorDoubleWidth` to check whether the glyph the cursor's on top of is double width. This code is exactly the same as the original PR that addressed this issue in #2932. That one got reverted at some point due to the crashes related to it, but due to a combination of Terminal having come further since that PR and other changes to address use-after-frees, some of the crashes may/may not be relevant now. The ones that seemed to be relevant/repro-able, I attempt to address in this PR. The `IsCursorDoubleWidth` check would fail during the `TextBuffer::Reflow` call inside of `Terminal::UserResize` occasionally, particularly when `newCursor.EndDeferDrawing()` is called. This is because when we tell the newCursor to `EndDefer`, the renderer will attempt to redraw the cursor. As part of this redraw, it'll ask if `IsCursorDoubleWidth`, and if the renderer managed to ask this before `UserResize` swapped out the old buffer with the new one from `Reflow`, the renderer will be asking the old buffer if its out-of-bounds cursor is double width. This was pretty easily repro'd using `cmatrix -u0` and resizing the window like a madman. As a solution, I've moved the Start/End DeferDrawing calls out of `Reflow` and into `UserResize`. This way, I can "clamp" the portion of the code where the newBuffer is getting created and reflowed and swapped into the Terminal buffer, and only allow the renderer to draw once the swap is done. This also means that ConHost's `ResizeWithReflow` needed to change slightly. In addition, I've added a WriteLock to `SetCursorOn`. It was mentioned as a fix for a crash in #2965 (although I can't repro), and I also figured it would be good to try to emulate where ConHost locks with regards to Cursor operations, and this seemed to be one that we were missing. ## PR Checklist * [x] Closes #2713 * [x] CLA signed * [x] Tests added/passed ## Validation Steps Performed Manual validation that the cursor is indeed chonky, added a test case to check that we are correctly saying that the cursor is double width (not too sure if I put it in the right place). Also open to other test case ideas and thoughts on what else I should be careful for since I am quite nervous about what other crashes might occur. --- <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:58 +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#26257
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?