Issues with cursor invalidation in GDI

[j4james]

Windows Terminal version

Commit 378b659

Windows build number

10.0.19045.4291

Other Software

No response

Steps to reproduce

1. Make sure the conhost render engine is set to GDI (i.e. the UseDx registry entry is 0).
2. Open a WSL shell in a recent build of OpenConsole (commit 20b0bed or later)
3. Execute the following statements:

   printf "\e[2 q"; sleep 2; printf "\e[A"; sleep 2; printf "\e[B"
   

Expected Behavior

The above script is setting the cursor to a steady block so it's easy to see, sleeping for 2 seconds, moving up a line and sleeping for another 2 seconds, then moving the cursor back down again. You should be able to see the cursor the entire time.

Actual Behavior

Initially the cursor is visible, but as soon as we move up a line it becomes invisible for 2 seconds.

I believe this is a regression from PR #15500. Originally we would call InvalidateCursor (via TriggerRedrawCursor) whenever the cursor was moved, both to invalidate the old position and the new position. But since PR #15500, TriggerRedrawCursor has been nuked, and InvalidateCursor is now called at the start of every frame (regardless of whether the cursor is moved or not), and it only invalidates the old cursor position. So when the cursor moves, it's erased from the old position, but not redrawn in the new position.

This typically only affects vertical movement (other than some edge cases), because at some point I think we stopped caring about the horizontal extent of the invalidated region and just refresh the full width of the screen regardless. This also doesn't seem to affect the DX and Atlas engines, but I have no idea why.

[github-actions]

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

• Flickering in conhost when scrolling top part of screen (#11274), similarity score: 0.76

Closed similar issues:

• More cursor droppings (#8213), similarity score: 0.80
• Cursor droppings are back in conhost (#12739), similarity score: 0.77
• Weird scrolling regression in the GDI renderer (#13270), similarity score: 0.76

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

[j4james]

I love you github-actions bot, and I hope you'll spare my life when the robot revolution comes, but I feel a little bit hurt when you suggest a bunch of my own issues back to me, like idiot human can't remember that he's already raised this issue before. Although to be fair, that's not an unreasonable assumption.

[j4james]

As to how we fix this issue, I can get it to work again by replicating most of the following code a couple of lines down:

terminal/src/renderer/base/renderer.cpp

Lines 124 to 160 in 378b659

	if (_currentCursorOptions)
	{
	const auto& buffer = _pData->GetTextBuffer();
	const auto view = buffer.GetSize();
	const auto coord = _currentCursorOptions->coordCursor;
	// If we had previously drawn a composition at the previous cursor position
	// we need to invalidate the entire line because who knows what changed.
	// (It's possible to figure that out, but not worth the effort right now.)
	if (_compositionCache)
	{
	til::rect rect{ 0, coord.y, til::CoordTypeMax, coord.y + 1 };
	if (view.TrimToViewport(&rect))
	{
	FOREACH_ENGINE(pEngine)
	{
	LOG_IF_FAILED(pEngine->Invalidate(&rect));
	}
	}
	}
	else
	{
	const auto lineRendition = buffer.GetLineRendition(coord.y);
	const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;
	til::rect rect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 };
	rect = BufferToScreenLine(rect, lineRendition);
	if (view.TrimToViewport(&rect))
	{
	FOREACH_ENGINE(pEngine)
	{
	LOG_IF_FAILED(pEngine->InvalidateCursor(&rect));
	}
	}
	}
	}

That is, after we've reinitialized _currentCursorOptions with the new position, we need to invalidate that new position as well (I'm not sure about the _compositionCache, so I just ignored that bit). I don't know if this is a good solution, though.