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

Feature Request - filledBox cursor should cover the whole unicode character #2713

Closed
dotSlashLu opened this issue Sep 10, 2019 · 9 comments · Fixed by #2932 or #5319
Closed

Feature Request - filledBox cursor should cover the whole unicode character #2713

dotSlashLu opened this issue Sep 10, 2019 · 9 comments · Fixed by #2932 or #5319
Assignees
@leonMSFT
Labels
Area-Fonts Related to the font Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.0

Comments

@dotSlashLu
Copy link

Description of the new feature/enhancement

The filledBox cursor should not hide the covered character and it behaves differently for ASCII and wide characters:

  • For ASCII, the cursor covers the whole character:
    image
  • For wide characters, the cursor only covers part of the characters:
    image

Other terminal applications typically handle this well, take XShell for an example:
image
image
And the cursor automatically invert the covered character's color to show it properly without hiding it behind.
@dotSlashLu dotSlashLu added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Sep 10, 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 Sep 10, 2019
@mdtauk
Copy link

mdtauk commented Sep 10, 2019

Perhaps along with the UI category, there should be one for rendering?

@DHowett-MSFT
Copy link
Contributor

@mdtauk You mean like Area-Rendering?

@zadjii-msft
Copy link
Member

Is this not the same as #1203?

@mdtauk
Copy link

mdtauk commented Sep 10, 2019

@mdtauk You mean like Area-Rendering?

I was looking at the Projects section, and only saw Design and Specification

:D

@DHowett-MSFT
Copy link
Contributor

@zadjii-msft nah this is "my character is double-cell, but my cursor is single-cell."

@zadjii-msft zadjii-msft added Area-Fonts Related to the font Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Sep 11, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 11, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Sep 11, 2019
@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 16, 2019
@DHowett-MSFT
Copy link
Contributor

Something something RenderData::GetCursorWidth

@skyline75489 skyline75489 mentioned this issue Sep 27, 2019
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Sep 27, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR labels Sep 27, 2019
@zadjii-msft zadjii-msft mentioned this issue Oct 24, 2019
Closed
5 tasks
@DHowett-MSFT DHowett-MSFT reopened this Oct 24, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 24, 2019
@zadjii-msft zadjii-msft removed the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Oct 25, 2019
@cinnamon-msft cinnamon-msft removed the Issue-Task It's a feature request, but it doesn't really need a major design. label Jan 23, 2020
@cinnamon-msft cinnamon-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) v1-Scrubbed labels Jan 23, 2020
@cinnamon-msft cinnamon-msft added Priority-2 A description (P2) and removed Priority-3 A description (P3) labels Feb 28, 2020
@zadjii-msft
Copy link
Member

You know, it's been like 5 months since the fix was proposed in #2932 and subsequently reverted. I don't totally remember what all went down with that, and reading through the notes didn't really clear it up for me. I think I'd be more comfortable with @miniksa handling this one.

We did a bunch to help our use-after-free's in the Terminal codebase since then, so maybe the original fix would just work by itself now.

@miniksa
Copy link
Member

miniksa commented Mar 16, 2020

I would like to do this, it's just that my stack of work is already so high and the world is sort of on fire. So we'll see.

@leonMSFT leonMSFT mentioned this issue Apr 10, 2020
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Apr 10, 2020
@ghost ghost closed this as completed in #5319 Apr 15, 2020
@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 Apr 15, 2020
ghost pushed a commit that referenced this issue Apr 15, 2020
@leonMSFT
Show a double width cursor for double width characters (#5319)
fe3f528 
# 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.
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #5319, which has now been successfully released as Windows Terminal Preview v0.11.1121.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

@leonMSFT leonMSFT

Labels
Area-Fonts Related to the font Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.0
7 participants
@dotSlashLu @mdtauk @DHowett-MSFT @miniksa @zadjii-msft @cinnamon-msft @leonMSFT