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

COOKED_READ tab completion is very wrong when the prompt is wrapped #4975

Closed
AnuthaDev opened this issue Mar 18, 2020 · 6 comments · Fixed by #15783
Closed

COOKED_READ tab completion is very wrong when the prompt is wrapped #4975

AnuthaDev opened this issue Mar 18, 2020 · 6 comments · Fixed by #15783
Labels
Area-CookedRead The cmd.exe COOKED_READ handling Area-Input Related to input processing (key presses, mouse, etc.) Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase

Comments

@AnuthaDev
Copy link
Contributor

AnuthaDev commented Mar 18, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.18363.535]
Windows Terminal version (if applicable): 0.10.761.0

Steps to reproduce

  1. Open cmd in Terminal.
  2. Reduce the width to minimum (well, not minimum but small enough, as it occurs after crossing some threshold).
  3. Press and hold Tab.

Expected behavior

cmd cycles through the directories:

ezgif-5-366a3a796ac1

Actual behavior

The names of directories are printed in succession without erasing the previous name which leads to this:

Also, note that this behaviour is not unique to Terminal, the default command promt(cmd/conhost/conPTY??) also has this bug:

image

After resizing the terminal the text cannot be erased.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Mar 18, 2020
@AnuthaDev AnuthaDev changed the title Weird Behaviour of Terminal/shells when window width is small Weird Behaviour of Terminal/cmd when window width is small Mar 18, 2020
@zadjii-msft
Copy link
Member

I'm not saying that this isn't a bug, but it definitely repros in conhost too, so it's not necessarily a conpty or terminal bug.

image

For me, the trick is to resize such that the line the prompt starts on reflows, then hold tab. I'm seeing this on the 10.0.19587.1001 conhost, so that's not a particularly new conhost either.

@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Mar 18, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 18, 2020
@zadjii-msft zadjii-msft added this to the 21H1 milestone Mar 18, 2020
@AnuthaDev
Copy link
Contributor Author

AnuthaDev commented Mar 18, 2020

Hey @zadjii-msft I was not saying that this is a bug in conpty, Its just that I have seen that word used in similar contexts and didn't actually know the name of the default console, so threw it in there.(Note the question marks)

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 19, 2020
@DHowett-MSFT DHowett-MSFT modified the milestones: 21H1, Console Backlog Mar 19, 2020
@DHowett-MSFT
Copy link
Contributor

It looks like this reproduces as far back as RS4, and only impacts the first cooked_read line that wraps after the resize, so I'm going to move it from 21H1 to Backlog and yank Triage. P3.

Thanks all!

@DHowett-MSFT DHowett-MSFT added Priority-3 A description (P3) Help Wanted We encourage anyone to jump in on these. and removed Priority-2 A description (P2) labels Mar 19, 2020
@DHowett-MSFT DHowett-MSFT changed the title Weird Behaviour of Terminal/cmd when window width is small COOKED_READ tab completion is very wrong when the window is shrunk Mar 19, 2020
@DHowett-MSFT DHowett-MSFT changed the title COOKED_READ tab completion is very wrong when the window is shrunk COOKED_READ tab completion is very wrong when the prompt is wrapped Mar 19, 2020
@piotrzarycki
Copy link

can i take care of it :) ?

@DHowett-MSFT
Copy link
Contributor

If you want to take a crack at it, by all means do!

@serd2011
Copy link
Contributor

serd2011 commented Aug 30, 2022

This has something to do with the actual resizing cuz launching conhost with small width (enough to wrap cd path) does not reproduce this. And resizing back to big width causes it to print over the cd path. Interestingly, just writing after resizing works fine.

I have a theory. When window is resizes screen buffer changes its width. This change is not communicated to the connected application. Then it receives tab press and tries to output result on the previous cursor position that is now out of bounds. This causes conhost to disregard the position and just append it to the end. This is in line with observed behavior when conhost is launched with a small width and resized to a bigger one. Cursor position stays the same and prints on top of the cd path.
But when user inputs the coordinates get send with it and it works.
But I might be completely misunderstanding how this even works.

2022-08-30.05-42-00.mp4

@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 2, 2023
@zadjii-msft zadjii-msft modified the milestones: Backlog, Terminal v1.19 Aug 23, 2023
DHowett pushed a commit that referenced this issue Aug 25, 2023
This massive refactoring has two goals:
* Enable us to go beyond UCS-2 support for input editing
* Bring clarity into `COOKED_READ_DATA`'s inner workings

Unfortunately, over time, knowledge about its exact operation was lost.
While the new code is still complex it reduces the amount of code by 4x
which will make preserving knowledge hopefully significantly easier.

The new implementation is simpler and slower than the old one in a way,
because every time the input line is modified it's rewritten to the text
buffer from scratch. This however massively simplifies the underlying
algorithm and the amount of state that needs to be tracked and results
in a significant reduction in code size. It also makes it more robust,
because there's less code now that can be incorrect.

This "optimization laziness" can be afforded due the recent >10x
improvements to `TextBuffer`'s text ingestion performance.
For short inputs (<1000 characters) I still expect this implementation
to outperform the conhost from the past.
It has received one optimization already however: While reading text
from the `InputBuffer` we'll now defer writing into the `TextBuffer`
until we've stopped reading. This improves the overhead of pasting text
from O(n^2) to O(n), which is immediately noticeable for inputs >100kB.

Resizing the text buffer still ends up corrupting the input line
however, which unfortunately cannot be fixed in `COOKED_READ_DATA`.
The issue occurs due to bugs in `TextBuffer::Reflow` itself, as it
misplaces the cursor if the prompt is on the last line of the buffer.

Closes #1377
Closes #1503
Closes #4628
Closes #4975
Closes #5033
Closes #8008

This commit is required to fix #797

## Validation Steps Performed
* ASCII input ✅
* Chinese input (中文維基百科) ❔
  * Resizing the window properly wraps/unwraps wide glyphs ❌
    Broken due to `TextBuffer::Reflow` bugs
* Surrogate pair input (🙂) ❔
  * Resizing the window properly wraps/unwraps surrogate pairs ❌
    Broken due to `TextBuffer::Reflow` bugs
* In cmd.exe
  * Create 2 file: "a😊b.txt" and "a😟b.txt"
  * Press tab: Autocompletes "a😊b.txt" ✅
  * Navigate the cursor right past the "a"
  * Press tab twice: Autocompletes "a😟b.txt" ✅
* Backspace deletes preceding glyphs ✅
* Ctrl+Backspace deletes preceding words ✅
* Escape clears input ✅
* Home navigates to start ✅
* Ctrl+Home deletes text between cursor and start ✅
* End navigates to end ✅
* Ctrl+End deletes text between cursor and end ✅
* Left navigates over previous code points ✅
* Ctrl+Left navigates to previous word-starts ✅
* Right and F1 navigate over next code points ✅
  * Pressing right at the end of input copies characters
    from the previous command ✅
* Ctrl+Right navigates to next word-ends ✅
* Insert toggles overwrite mode ✅
* Delete deletes next code point ✅
* Up and F5 cycle through history ✅
  * Doesn't crash with no history ✅
  * Stops at first entry ✅
* Down cycles through history ✅
  * Doesn't crash with no history ✅
  * Stops at last entry ✅
* PageUp retrieves the oldest command ✅
* PageDown retrieves the newest command ✅
* F2 starts "copy to char" prompt ✅
  * Escape dismisses prompt ✅
  * Typing a character copies text from the previous command up
    until that character into the current buffer (acts identical
    to F3, but with automatic character search) ✅
* F3 copies the previous command into the current buffer,
  starting at the current cursor position,
  for as many characters as possible ✅
  * Doesn't erase trailing text if the current buffer
    is longer than the previous command ✅
  * Puts the cursor at the end of the copied text ✅
* F4 starts "copy from char" prompt ✅
  * Escape dismisses prompt ✅
  * Erases text between the current cursor position and the
    first instance of a given char (but not including it) ✅
* F6 inserts Ctrl+Z ✅
* F7 without modifiers starts "command list" prompt ✅
  * Escape dismisses prompt ✅
  * Minimum size of 40x10 characters ✅
  * Width expands to fit the widest history command ✅
  * Height expands up to 20 rows with longer histories ✅
  * F9 starts "command number" prompt ✅
  * Left/Right paste replace the buffer with the given command ✅
    * And put cursor at the end of the buffer ✅
  * Up/Down navigate selection through history ✅
    * Stops at start/end with <10 entries ✅
    * Stops at start/end with >20 entries ✅
    * Wide text rendering during pagination with >20 entries ✅
  * Shift+Up/Down moves history items around ✅
  * Home navigates to first entry ✅
  * End navigates to last entry ✅
  * PageUp navigates by 20 items at a time or to first ✅
  * PageDown navigates by 20 items at a time or to last ✅
* Alt+F7 clears command history ✅
* F8 cycles through commands that start with the same text as
  the current buffer up until the current cursor position ✅
  * Doesn't crash with no history ✅
* F9 starts "command number" prompt ✅
  * Escape dismisses prompt ✅
  * Ignores non-ASCII-decimal characters ✅
  * Allows entering between 1 and 5 digits ✅
  * Pressing Enter fetches the given command from the history ✅
* Alt+F10 clears doskey aliases ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 25, 2023
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 Area-Input Related to input processing (key presses, mouse, etc.) Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants