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

Backspace in wrapped cooked read erases char on line above #17554

Closed
j4james opened this issue Jul 12, 2024 · 2 comments · Fixed by #17556
Closed

Backspace in wrapped cooked read erases char on line above #17554

j4james opened this issue Jul 12, 2024 · 2 comments · Fixed by #17556
Assignees
Labels
Area-CookedRead The cmd.exe COOKED_READ handling 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 Product-Conhost For issues in the Console codebase

Comments

@j4james
Copy link
Collaborator

j4james commented Jul 12, 2024

Windows Terminal version

Commit ac5b4f5

Windows build number

10.0.19045.4529

Other Software

No response

Steps to reproduce

  1. Checkout the latest code from main (ac5b4f5 or later).
  2. Start a cmd shell in OpenConsole.
  3. Type some content at the prompt until it wraps onto the second line.
  4. Backspace until the cursor is at the start of that second line.

Expected Behavior

Backspaces on the second line should have no effect on the characters in the first line.

Actual Behavior

The last character on the first line gets erased when backspacing over the first character in the second line.

This is what it looks like after wrapping.
image

And this is what it looks like after two backspaces. Notice the r erased from the first line.
image

The same thing occurs in both OpenConsole and Windows Terminal. I think this was introduced by the new VT cooked read implementation (PR #17445).

@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 Jul 12, 2024
@j4james
Copy link
Collaborator Author

j4james commented Jul 12, 2024

I should note that this is just a visual glitch. The actual prompt content is correct. It just looks like a character is missing.

@lhecker
Copy link
Member

lhecker commented Jul 12, 2024

Your issue occurs because if (pagerPromptEnd.x >= size.width) must be after if (pagerPromptEnd.y <= _pagerPromptEnd.y) and not before it (I think).

I found two more bugs:

  • Writing a wide glyph into the last column breaks the cursor position.
    Fixed by moving dist += state.len; in TextBuffer::FitTextIntoColumns to the end of the loop.
  • Having a fairly full prompt (e.g. 80% of the viewport height) and pressing F7 with a rather full history will move the prompt contents upwards. But when dismissing F7 via ESC it'll not restore the prompt to its previous state.

@lhecker lhecker added Product-Conhost For issues in the Console codebase Area-CookedRead The cmd.exe COOKED_READ handling labels Jul 12, 2024
@lhecker lhecker self-assigned this Jul 12, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 15, 2024
* Wide glyphs that don't fit into the last column got treated
  as narrow glyphs which broke line layout.
* Wide glyphs that don't fit into the last column got manually
  padded with whitespace which broke Ctrl+A + Ctrl+C in conhost.
* Sudden increases/decreases in the pager height would leave
  parts of the viewport with leftover text and not clear it away.
* Deleting an entire word at the start of a line would only delete
  its first two characters.

Closes #17554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CookedRead The cmd.exe COOKED_READ handling 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 Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants