Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

DHowett-MSFT · 2020-04-17T21:25:21Z

The logic here, regarding deleting the spaces and just instantly adding
them bad, is incredibly suspect. Given that we're close to 0.11, I don't
think I can change it.

I've added a TODO with an issue number to figure out the right logic
here.

Fixes #5386.

PR Checklist

Closes Lines that wrap on space characters at the bottom of the buffer (“new bottom line”) receive an extra line break #5386
CLA signed
Tests added/passed
Requires documentation to be updated
I'm horrified.

Validation Steps Performed

Tests, manual validation of the scenario in 5386 and a repro program.

The logic here, regarding deleting the spaces and just instantly adding them bad, is incredibly suspect. Given that we're close to 0.11, I don't think I can change it. Fixes #5386.

zadjii-msft

Okay so the whole logic of this case is now:

This line wrapped, so we didn't remove the spaces (Line 460)
This line is a new bottom line (from earlier TriggerCircling / Scrolling)
Because we didn't remove the spaces, we shouldn't print them down here (the case under L566), because we already printed them

That checks out to me.

DHowett-MSFT · 2020-04-20T19:01:43Z

Because we didn't remove the spaces, we shouldn't print them down here (the case under L566), because we already printed them

I would like to eventually consolidate this, because. Will we ever print spaces in this branch?

SPACES REMOVED WHEN:
* line was not wrapped

AND

* It is optimal to prefer ECH/EL
* We are not on a new bottom line
* We did not clear all this frame
OR
* We cleared all this frame
OR
* It is a new bottom line and we are on it

SPACES RE-ADDED WHEN:
* It is not optimal to prefer ECH/EL
OR
* It is a new bottom line
OR
* We cleared all this frame

AND
* It is a new bottom line and we are on it
* The line had spaces at the end.
* We deleted the spaces previously <<< NEW IN THIS PR

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

miniksa · 2020-04-20T20:56:25Z

src/renderer/vt/paint.cpp

@@ -563,7 +563,7 @@ using namespace Microsoft::Console::Types;
        {
            _deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y };
        }
-        else if (numSpaces > 0)
+        else if (numSpaces > 0 && removeSpaces) // if we deleted the spaces... re-add them


Leave the "why" we're readding them.

so part of my confusion is I actually don't know why we would re-add them if we chose to delete them. we should just not choose to delete them.

@zadjii-msft can you remember why we'd re-add them if we'd deleted them?

Eh if we can't totally suss it out, just leave the MSFT to the code health item here.

okay, follow-up filing.

I can't remember tbh. This screams organic growth to me - I think we first just decided to do ECH and removing spaces, when we didn't have wrapping (and spaces didn't matter). Then we did some deferring the cursor movement & not erasing optimizations in 20H1, which added cases where we wouldn't ECH. Now we're adding cases where we still need the spaces. I suppose if we'd written it from scratch now, accounting for wrapping, we'd do it better.

I'd TODO it tbh.

#5430, linking in the comments

I bet we were trying to optimize out the cursor positioning that we'd do after a new line is printed. If we did optimize that scenario, and used ECH to erase some spaces, we also used CUF to move the cursor. However, if we couldn't do that, we'd still want to leave the cursor in the right place...

yea we probably should have just not removed them in the first place.

src/renderer/vt/paint.cpp

DHowett-MSFT · 2020-04-20T21:28:19Z

@msftbot merge this in about 4 minutes

ghost · 2020-04-20T21:28:23Z

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Mon, 20 Apr 2020 21:32:21 GMT, which is in 4 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

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)

This PR adds a test for #5428. Mysteriously, after #5398 merged, 5428 went away. However, I already wrote this test for it, so we might as well add it to our collection. * [x] Closes #5428 * [x] I work here * [x] Is a test

Don't duplicate spaces from potentially-wrapped EOL-deferred lines

501871d

The logic here, regarding deleting the spaces and just instantly adding them bad, is incredibly suspect. Given that we're close to 0.11, I don't think I can change it. Fixes #5386.

DHowett-MSFT requested a review from zadjii-msft April 17, 2020 21:25

zadjii-msft approved these changes Apr 20, 2020

View reviewed changes

zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Apr 20, 2020

miniksa mentioned this pull request Apr 20, 2020

Adjusts High DPI scaling to enable differential rendering #5345

Merged

9 tasks

DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Apr 20, 2020

ghost requested review from miniksa, carlos-zamora and leonMSFT April 20, 2020 18:55

zadjii-msft mentioned this pull request Apr 20, 2020

Decreasing the width of vim.exe corrupts buffer #5428

Closed

miniksa reviewed Apr 20, 2020

View reviewed changes

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp Show resolved Hide resolved

miniksa reviewed Apr 20, 2020

View reviewed changes

miniksa approved these changes Apr 20, 2020

View reviewed changes

DHowett-MSFT mentioned this pull request Apr 20, 2020

Understand why the PTY might delete and then re-add spaces on certain lines #5430

Open

DHowett-MSFT commented Apr 20, 2020

View reviewed changes

src/renderer/vt/paint.cpp Show resolved Hide resolved

Update src/renderer/vt/paint.cpp

876b2e3

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

ghost merged commit 152b399 into master Apr 20, 2020

ghost deleted the dev/duhowett/what_on_earth branch April 20, 2020 21:45

zadjii-msft mentioned this pull request Apr 21, 2020

Add a test to cover #5428 #5449

Merged

3 tasks

zadjii-msft mentioned this pull request May 12, 2020

[Scenario] 2 Resize 2 Reflow #5800

Open

31 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

DHowett-MSFT commented Apr 17, 2020 •

edited

Loading

zadjii-msft left a comment

DHowett-MSFT commented Apr 20, 2020 •

edited

Loading

miniksa Apr 20, 2020

DHowett-MSFT Apr 20, 2020

DHowett-MSFT Apr 20, 2020

miniksa Apr 20, 2020

DHowett-MSFT Apr 20, 2020

zadjii-msft Apr 20, 2020

DHowett-MSFT Apr 20, 2020

zadjii-msft Apr 20, 2020

DHowett-MSFT commented Apr 20, 2020

ghost commented Apr 20, 2020

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

Conversation

DHowett-MSFT commented Apr 17, 2020 • edited Loading

PR Checklist

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Apr 20, 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

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 20, 2020

ghost commented Apr 20, 2020

DHowett-MSFT commented Apr 17, 2020 •

edited

Loading

DHowett-MSFT commented Apr 20, 2020 •

edited

Loading