Show a double width cursor for double width characters #5319

leonMSFT · 2020-04-10T21:41:34Z

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

Closes Feature Request - filledBox cursor should cover the whole unicode character #2713
CLA signed
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.

skyline75489 · 2020-04-12T03:59:45Z

A note about performance: when #2932 was born, the overall performace penalty is somewhat observable, because IsCursorDoubleWidth is called very frequently. Perhaps not like #5288 kinda frequency, but still there are many routines are using IsCursorDoubleWidth.

By the way, I can not repro the crash described in #2965, either.

carlos-zamora

Just a few questions, not blocking, but I'll sign off after I get a response. 😊

src/cascadia/TerminalCore/Terminal.cpp

carlos-zamora · 2020-04-13T18:12:01Z

src/cascadia/TerminalCore/Terminal.cpp

-    CATCH_RETURN();
+    catch (...)
+    {
+        _buffer->GetCursor().EndDeferDrawing();


In screenInfo, you said...

// Only tell _textBuffer to EndDefer (and not newTextBuffer) because whether or not Reflow failed, // _textBuffer will end up being the buffer that we use after returning.

Should we just CATCH_RETURN() here then?

Also, if we return on line 221, would we be fine?

So I wanted to make sure _buffer needs to EndDefer no matter what before we get out of this function. If the try failed at all and entered the catch, I need to call EndDeferDrawing() before we return the exception. ScreenInfo seemed to be slightly different in that there wasn't a try-catch wrapping the Reflow call.

I actually did not notice the RETURN_IF_FAILED for Reflow... Does that just totally skip the catch block? If so, I need to figure out where to EndDefer for _buffer 🤔

I actually did not notice the RETURN_IF_FAILED for Reflow... Does that just totally skip the catch block? If so, I need to figure out where to EndDefer for _buffer 🤔

From a glimpse at RETURN_IF_FAILED, yup. We'd return so the catch block never gets executed.

You could probably get around it with a wil::scope_exit though.

Ok, I basically just separated out the RETURN_IF_FAILED call into its innards, but slapped an EndDefer call before it.

I'd somewhat prefer a scope_exit if possible. I'm happy to merge this as-is if a scoped end-defer isn't easy, but i'm worried that we have three different "end defer" locations here, and they refer to multiple different text buffers. idk

Oh crap I'm so sorry @carlos-zamora 😢 , I didn't refresh my tab this morning when I replied to this thread so I didn't see the scope_exit comment before pushing my wacky change! Both of you are totally right, it's a much better approach 😅

leonMSFT · 2020-04-13T18:41:15Z

@skyline75489 That's a good callout with the performance hit, the cursor redraws quite frequently. Maybe it would be better if I somehow saved the answer to IsCursorDoubleWidth as a boolean that's figured out ahead of time (not sure where yet though).

zadjii-msft

Maybe it would be better if I somehow saved the answer to IsCursorDoubleWidth as a boolean that's figured out ahead of time (not sure where yet though).

I'm not sure that's something that's totally cacheable. We might be able to stash that information as the cursor gets moved, but I'd think that the cursor is drawn fewer times than it's moved.

I think the perf hit is probably fine, considering this seems like it's the same implementation that conhost has in RenderData::IsCursorDoubleWidth. It might be a useful exercise though to have a perf trace taken and compare before and after this change though, to prove that nothing dramatic has regressed.

skyline75489 · 2020-04-14T11:58:49Z

Yeah the caching isn't really a good idea. My point is that we should be vigilant against issues like #5288 . It would be more useful to focus on 'how to update cursor less frequently' than 'how to update cursor fast enough'. The doubled-width cursor may help us find more hot paths, for that matter.

leonMSFT · 2020-04-14T18:15:22Z

After running the bigtext.txt test from Terminal Not Speed Enough, the time taken seems to be roughly in the same range (sometimes even faster but maybe because of many other changes since) when comparing Terminal versions 0.10.781 to my branch on release.

DHowett-MSFT · 2020-04-14T20:27:36Z

src/cascadia/TerminalCore/Terminal.cpp

-    CATCH_RETURN();
+    catch (...)
+    {
+        _buffer->GetCursor().EndDeferDrawing();


I'd somewhat prefer a scope_exit if possible. I'm happy to merge this as-is if a scoped end-defer isn't easy, but i'm worried that we have three different "end defer" locations here, and they refer to multiple different text buffers. idk

…into dev/lelian/bigcursor

DHowett-MSFT · 2020-04-14T21:55:09Z

src/cascadia/TerminalCore/Terminal.cpp

@@ -190,6 +190,10 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting
    // bottom in the new buffer as well. Track that case now.
    const bool originalOffsetWasZero = _scrollOffset == 0;

+    // skip any drawing updates that might occur until we swap _buffer with the new buffer or if we exit early.
+    _buffer->GetCursor().StartDeferDrawing();
+    auto endDefer = wil::scope_exit([&]() noexcept { _buffer->GetCursor().EndDeferDrawing(); });


nit: might be worth a comment that we're capturing _buffer by reference because either way, when we exit, we want to undefer the current active one

DHowett-MSFT · 2020-04-14T21:55:23Z

src/host/screenInfo.cpp

@@ -1403,6 +1403,10 @@ bool SCREEN_INFORMATION::IsMaximizedY() const
    // Save cursor's relative height versus the viewport
    SHORT const sCursorHeightInViewportBefore = _textBuffer->GetCursor().GetPosition().Y - _viewport.Top();

+    newTextBuffer->GetCursor().StartDeferDrawing();
+    _textBuffer->GetCursor().StartDeferDrawing();
+    auto endDefer = wil::scope_exit([&]() noexcept { _textBuffer->GetCursor().EndDeferDrawing(); });


same comment as above :)

DHowett-MSFT · 2020-04-14T22:09:00Z

mr static

🤣 your commit messages kill me

leonMSFT · 2020-04-14T22:15:21Z

mr static

🤣 your commit messages kill me

they rule with a firm hand 😢

skyline75489 · 2020-04-14T22:34:03Z

Yeah I totally feel you. I was once bickering with Mr. Static. And I got burned in a way that I’ve never experienced before. I learned a valuable lesson: do not mess with Miss. Noexcept unless you absolutely need to. She looks harmless and gorgeous but in fact she’s very dangerous.

leonMSFT · 2020-04-15T18:11:38Z

/azp run

azure-pipelines · 2020-04-15T18:11:49Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-04-15T19:22:37Z

Hello @leonMSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Improve wide glyph support in UIA (GH-4946) Add enhanced key support for ConPty (GH-5021) Set DxRenderer non-text alias mode (GH-5149) Reduce CursorChanged Events for Accessibility (GH-5196) Add more object ID tracing for Accessibility (GH-5215) Add SS3 cursor key encoding to ConPty (GH-5383) UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399) Make CodepointWidthDetector::GetWidth faster (CC-3727) add til::math, use it for float conversions to point, size (GH-5150) Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353) Fix a deadlock and a bounding rects issue in UIA (GH-5385) Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398) Reimplement the VT tab stop functionality (CC-5173) Clamp parameter values to a maximum of 32767. (CC-5200) Prevent the cursor type being reset when changing the visibility (CC-5251) Make RIS switch back to the main buffer (CC-5248) Add support for the DSR-OS operating status report (CC-5300) Update the virtual bottom location if the cursor moves below it (CC-5317) ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352) Set Cascadia Code as default font (GH-5121) Show a double width cursor for double width characters (GH-5319) Delegate all character input to the character event handler (CC-4192) Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092) Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079 Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122) Render row-by-row instead of invalidating entire screen (GH-5185) Make conechokey use ReadConsoleInputW by default (GH-5148) Manually pass mouse wheel messages to TermControls (GH-5131) This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208) Fix copying wrapped lines by implementing better scrolling (GH-5181) Emit lines wrapped due to spaces at the end correctly (GH-5294) Remove unneeded whitespace (CC-5162)

ghost · 2020-04-22T17:14:36Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

leonMSFT added 14 commits March 31, 2020 14:08

first lets add back the code that crashes

a2dc867

adding a test that may or may not work

56392f6

test case

b86d85e

i just add debug for now

9e149cb

Merge branch 'master' into dev/lelian/bigcursor

7574e9c

mAYBE

5fcbac7

ridding myself of comments

32af07a

y fail if it wouldn't matter hmm

69b0f27

why don't we just take these calls and place them over here

21fd1b3

Merge branch 'master' into dev/lelian/bigcursor

8b46679

added test case to idk if this is right place

91e4fb8

remove this

51f6cce

reverting this file

640bc69

wtf

ae430ce

carlos-zamora reviewed Apr 13, 2020

View reviewed changes

zadjii-msft approved these changes Apr 13, 2020

View reviewed changes

zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Apr 13, 2020

leonMSFT added 2 commits April 13, 2020 14:43

build is complaining, more returns

3509c9f

what about nOW

8e70d83

carlos-zamora approved these changes Apr 14, 2020

View reviewed changes

DHowett-MSFT approved these changes Apr 14, 2020

View reviewed changes

leonMSFT added 4 commits April 14, 2020 14:30

this is a much better thing

1a3a312

Merge branch 'dev/lelian/bigcursor' of github.com:microsoft/terminal …

4c7a47e

…into dev/lelian/bigcursor

conhost can also use this, and i forgot to put something back in

3e4252b

i am getting absolutely destroyed by mr static

cf32371

DHowett-MSFT reviewed Apr 14, 2020

View reviewed changes

comment

2216cb9

leonMSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 15, 2020

ghost merged commit fe3f528 into master Apr 15, 2020

ghost deleted the dev/lelian/bigcursor branch April 15, 2020 19:23

ghost mentioned this pull request Apr 22, 2020

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a double width cursor for double width characters #5319

Show a double width cursor for double width characters #5319

leonMSFT commented Apr 10, 2020

skyline75489 commented Apr 12, 2020

carlos-zamora left a comment

carlos-zamora Apr 13, 2020

leonMSFT Apr 13, 2020 •

edited

Loading

carlos-zamora Apr 13, 2020

leonMSFT Apr 14, 2020

DHowett-MSFT Apr 14, 2020

leonMSFT Apr 14, 2020

leonMSFT commented Apr 13, 2020

zadjii-msft left a comment

skyline75489 commented Apr 14, 2020

leonMSFT commented Apr 14, 2020

DHowett-MSFT Apr 14, 2020

DHowett-MSFT Apr 14, 2020

DHowett-MSFT Apr 14, 2020

DHowett-MSFT commented Apr 14, 2020

leonMSFT commented Apr 14, 2020

skyline75489 commented Apr 14, 2020

leonMSFT commented Apr 15, 2020

azure-pipelines bot commented Apr 15, 2020

ghost commented Apr 15, 2020

ghost commented Apr 22, 2020

Show a double width cursor for double width characters #5319

Show a double width cursor for double width characters #5319

Conversation

leonMSFT commented Apr 10, 2020

Summary of the Pull Request

PR Checklist

Validation Steps Performed

skyline75489 commented Apr 12, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT commented Apr 13, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

skyline75489 commented Apr 14, 2020

leonMSFT commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Apr 14, 2020

leonMSFT commented Apr 14, 2020

skyline75489 commented Apr 14, 2020

leonMSFT commented Apr 15, 2020

azure-pipelines bot commented Apr 15, 2020

ghost commented Apr 15, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Apr 22, 2020

leonMSFT Apr 13, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.