Terminal: git: cursor droppings have returned to the world

[miniksa]

Ported from internal issue MSFT: 19744641

Use GDI renderer
This time, it's in interactive git scenarios like git add -p.

The "cursor droppings" issue is where there are just left behind cursors somewhere on the renderer window. I know this affects conhost.exe. I'm not sure if it affects windowsterminal.exe yet. The filing says use GDI renderer, but that doesn't mean the underlying problem isn't in the Cursor class or something and would affect the DX renderer too.

[miniksa]

Also needs repro because I didn't come up with a perfectly consistent repro of this yet to narrow it down.

[zadjii-msft]

From #1128

---

Environment

Microsoft Windows [Version 10.0.18362.30]
Windows Terminal version (if applicable): N/A

Steps to reproduce

1. Open a PowerShell (5.1) window with default settings
2. Hit enter while the cursor is showing (you'll need to be quick to get it between blinks)

Note that this seems to happen intermittently but was very easy to reproduce (noticed it within seconds of upgrading last night).

Expected behavior

The cursor from the previous line should dissappear after you hit Enter.

Actual behavior

The cursor stays drawn on the previous line:

e.g.

---

[fgimian]

I'm sorry for being a pain, but was just curious what the plan is to resolve this one? Is it something that can be rolled out with a hotfix or will it need to wait until the next major release?

Kindest Regards
Fotis

[zadjii-msft]

This kind of thing won't be serviceable (hotfixable") unfortunately. Hopefully it'll be in the 20H1 release of Windows 10, so long as the fix is found before that.

[fgimian]

This kind of thing won't be serviceable (hotfixable") unfortunately. Hopefully it'll be in the 20H1 release of Windows 10, so long as the fix is found before that.

No worries, thanks a lot for the reply!

[j4james]

I'm not convinced this is the source of the issue, but note that PowerShell generates a new line, and scrolls the viewport, via a completely different code path to the one used by the cmd.exe shell, or a WSL bash shell. PowerShell relies on ApiRoutines::SetConsoleCursorPositionImpl, while the others will ultimately use the AdjustCursorPosition function. I suspect neither of those routines is correct in every aspect, though. Hopefully that is something that'll be sorted out with issue #2739.

Looking at some of the differences between those two code paths, though, one thing I found that seemed to help some of the time, was adding a call to Cursor::SetIsOn(true) before the call to buffer.SetCursorPosition in SetConsoleCursorPositionImpl. That at least fixed the issue for me in PowerShell, but not everywhere I've had the problem (e.g. the credentials prompt in the Windows git client was still broken).

I'm not sure this really tells you anything, but that's all I've been able to figure out so far.

[greg904]

I think the issue is because when you scroll:

1. In ScrollFrame the renderer inverts the old cursor's rect in both the in-memory context and the window context: https://github.com/microsoft/terminal/blob/9dc922fc37a479dd560675ac4ea1ce3713ae0ada/src/renderer/gdi/paint.cpp#L74-L79
2. But in PaintCursor the renderer inverts the new cursor's rect in only the in memory context: https://github.com/microsoft/terminal/blob/9dc922fc37a479dd560675ac4ea1ce3713ae0ada/src/renderer/gdi/paint.cpp#L615-L618

So the 2 inverts don't cancel each other out and the old cursor cursor ends up being painted in the window context. If I comment out line 78 the bug is fixed for me.

[j4james]

If I comment out line 78 the bug is fixed for me.

I can confirm that works for me too, at least in both of the cases that I knew would fail before. But I'm curious why it was only a problem for certain types of scrolling. Why was this fix only needed for PowerShell when all the shells are using the same renderer?

[tamlin-mike]

@j4james I suspect that fix takes care of both scenarios. The problem was surfacing with (at least) a blinking cursor, I believe when the scroll operation was performed while the cursor was in a visible state -- potentially even in a race between the mem dc and the ps dc. Let's say you have visible lines 1-3, already scrolled, and line 2 in error displays a static cursor due to this bug. On next line-scroll, line 2 could turn off that cursor (since the two DC's were out of synch), and instead render cursors for line 1, 3.

That bug was almost a given, since it's pretty much impossible to maintain synchronizations like that (be it GDI or data across some other arbitrary asynchronous domains). Let this be a lesson on data synchronization across domains, and the difficulty thereof.

[j4james]

OK I think I know what's going on now, and why commenting out line 78 is probably not the right solution.

Whenever the cursor renders with an InvertRect, the affected areas of the screen are saved in the cursorInvertRects variable. If the screen is then scrolled while the cursor is visible, those rects are "uninverted" in the ScrollFrame method before the scrolling takes place.

When the cursor has blinked off, though, the PaintCursor method won't set the cursorInvertRects variable, but it also doesn't clear it. So if the screen is scrolled at that point, the ScrollFrame method tries to "uninvert" the area where the cursor had _previously_ been painted. And since the cursor is no longer there, this has the opposite effect, leaving an unwanted mark on the screen.

The reason this is a problem in Powershell, but not in the cmd shell, is because the latter turns the cursor on when scrolling (as I mentioned above). This results in both the old and new cursor positions being repainted when the cursor is moved, so any incorrect ScrollFrame inversions will be cleaned up in the next paint cycle.

Now commenting out line 78 does kind of fix the problem, because that essentially disables this ScrollFrame clean up completely. Losing that clean up is not that big a deal, because it'll eventually happen anyway when the old cursor position is repainted. However, this can produce a kind of ghost effect, as there is a lag between when the screen is scrolled and the cursor is repainted, so it's not ideal.

What I think is the _correct_ solution, is that we need to reset the cursorInvertRects variable whenever the cursor isn't rendered. I thought at first we could do that at the top of the PaintCursor method, before it checks options.isOn. However there are still certain scenarios in which PaintCursor isn't even called, so my current recommendation would be to reset cursorInvertRects in the PaintBackground method.

That seems to be fix the problem for me in Powershell, and without the ghosting effect, but I haven't been able to reproduce the git case anymore, so I can't be absolutely certain that one is fixed too. However, I'm confident enough in this approach that I'd like to submit a PR for it, if nobody has any objections. At the very least it'll be an improvement on the current situation.

[zadjii-msft]

Wow that's a great analysis. I certainly don't have any objections. These cursor droppings issues always come and go as we meddle with the renderer, so I wouldn't be surprised if the original issue doesn't repro anymore. However, if you've got a repro and fix for #1128, then let's do it ☺️