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.

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

Cursor invalidation failing when line renditions are used #17226

Closed
j4james opened this issue May 9, 2024 · 2 comments · Fixed by #17234
Closed

Cursor invalidation failing when line renditions are used #17226

j4james opened this issue May 9, 2024 · 2 comments · Fixed by #17234
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@j4james
Copy link
Collaborator

j4james commented May 9, 2024

Windows Terminal version

1.21.1272.0

Windows build number

10.0.19045.4291

Other Software

No response

Steps to reproduce

  1. Open a WSL bash shell
  2. Execute the following statement:
    printf "\ec\e[999B\n\e[H\e#6\n\e[1 q"; read
    
  3. Type some text and note whether the cursor is blinking.
  4. Backspace to the start of the line and check the cursor again.

Expected Behavior

The final escape sequence in that printf is a DECSCUSR command, setting the cursor style to a blinking block, so it should be blinking the whole time (except maybe when it's actually moving).

Actual Behavior

The cursor blinks at the start of the line, but doesn't blink in any other column. The issue isn't specifically related to blinking, though. It can also manifest as almost constant cursor "droppings", but this test case was the easiest to reproduce.

And note that it also fails on openconsole/conhost if you've configured it to use the Atlas engine, so it's not a conpty issue, but it appears to be Atlas only.

Line rendition has got something to do with it - the first line is set to double-width in this test - but it's not only the double-width line that is affected. And I think the scroll offset is also a factor, which is why this test case forces a linefeed at the bottom of the page.

As far as I can tell from a git bisect, it first broke in commit 20b0bed (PR #15500).

@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 9, 2024
Copy link

github-actions bot commented May 9, 2024

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:

Closed similar issues:

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

@j4james
Copy link
Collaborator Author

j4james commented May 9, 2024

It looks to me like the problem is here:

const auto coord = _currentCursorOptions.coordCursor;
const auto lineRendition = buffer.GetLineRendition(coord.y);
const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;

The _currentCursorOptions.coordCursor value is relative to the top of the screen, while the GetLineRendition call is expecting a buffer row offset.

That said, I don't think we need that call anyway, since the line renditions should already be cached in the _currentCursorOptions structure. Same goes for the IsCursorDoubleWidth call on the next line.

So unless I'm missing something, I'd suggest replacing those two lines with:

const auto lineRendition = _currentCursorOptions.lineRendition;
const auto cursorWidth = _currentCursorOptions.fIsDoubleWidth ? 2 : 1;

github-merge-queue bot pushed a commit that referenced this issue May 10, 2024
## Summary of the Pull Request

When the renderer calculates the invalidate region for the cursor, it
needs to take the line rendition into account. But it was using a
relative coordinate rather than absolute coordinate when looking up the
line rendition for the row, so the calculated region could easily be
incorrect.

With this PR we now use the line rendition that was already being cached
in the `CursorOptions` structure, so we avoid needing to look it up
anyway. Similarly I've replaced the `IsCursorDoubleWidth` lookup with
the value that was already cached in the `CursorOptions` structure.

## Validation Steps Performed

I've confirmed that the test case in issue #17226 is now working as
expected.

## PR Checklist
- [x] Closes #17226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant