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

WSL Terminal Line Endings (the "exact wrap" bug) #3088

Closed
aeiplatform opened this issue Oct 6, 2019 · 6 comments · Fixed by #15701
Closed

WSL Terminal Line Endings (the "exact wrap" bug) #3088

aeiplatform opened this issue Oct 6, 2019 · 6 comments · Fixed by #15701
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) 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-2 A description (P2) Product-Conhost For issues in the Console codebase
Milestone

Comments

@aeiplatform
Copy link

Put into terminal and then increase the window size:
printf "%*s\n" "$(tput cols)" "" | tr " " "X"

Line endings acts like they disappeared if the string length matches exactly the window columns.
The terminal has different behavior for columns +- 1.
The behavior does not depend on the type of shell.

@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 Oct 6, 2019
@egmontkob
Copy link

egmontkob commented Oct 6, 2019

I discovered this yesterday and wanted to report today, LOL.

There's more to the story.

First, let's note that most shells redraw their prompt after a resize, potentially overwriting some of the previously printed Xs, or just making it harder to follow what's happening. So the given line that's exactly as wide as the screen should be followed by some other lines to clearly see what's going on (e.g. just tap on Enter once or twice to get new prompts).

Then, let's observe that if you have a shorter line than the width, e.g. shorter by one:

printf "%*s\n" "$(( $(tput cols) - 1 ))" "" | tr " " "Y"

(followed by some other lines), and then you narrow the window, this line wraps one step too early (e.g. in this case immediately as you shrink the window by 1), introducing an unnecessary empty line below (which in turn copy-pastes as tons of spaces).

Another thing I noticed is that double clicking stops the selection at the current physical row boundary, rather than selecting an entire word.

I haven't dug into the code yet, but I have a guts feeling that we might be seeing a conceptionally unusual and problematic handling (or rather: lack thereof) of newlines, rather than a simple off-by-one.

I have the feeling (and apologies if I'm wrong, I'm planning to explore the code soon) that it's not remembered whether a row ends in a newline or not; instead, the presence or absence of a trailing speical character ("unused space" or so), which occupies its own cell, is used for that. Or something similar.

What most terminals do, and I believe is the right thing, is that they remember for each physical row whether it ended in an explicit newline or an overflow. This bit belongs to the row itself, rather than a particular cell thereof, and is obviously not directly visible (you could somehow display it in some debug mode). This information is used for multiple things:

  • Rewrapping on a resize
  • Selecting on double click (across physical wrap within a logical line, but not across newline characters)
  • Selecting entire logical lines on triple click
  • Automatically recognizing text patterns, e.g. URLs or user supplied regexps
  • Running the BiDi algorithm on logical lines as units (i.e. mapping them to Unicode Bidi Algorithm's notion of "paragraph")

Probably there's no official specification anywhere how exactly the line ending status needs to be set. It's mostly common sense, with the only nontrivial bit: a NL character does not set the line to end in an explicit newline, it leaves its state unchanged. My BiDi proposal also provides an overview of this, and then refines the behavior for some special cases.

I think that going with this traditional, well proven behavior of tracking newlines is the key to address multiple issues, e.g. rewrapping's dropping of a newline and off-by-one as reported here, proper spaces vs. newlines at copy-pasting (e.g. #2944), proper semantical behavior on double click, and to prepare for other cool features WT is about to have.

@DHowett-MSFT
Copy link
Contributor

Just dropping in to say: yeah, we don't do wrapping correctly in Windows Terminal. Since it's a client of conhost, and conhost does wrapping somewhat okay, WT isn't yet enlightened here.

There's a lot of intricacies in the whole ConPTY layer -- it should almost certainly un-wrap wrapped lines before it sends them, and let the terminal emulator on the other end wrap them, but we can't do that until #780... WT won't even wrap right now (evidenced by #2588 #3094).

Curious thing about newlines in the traditional console is that using SetConsoleMode to turn off ENABLE_WRAP_AT_EOL_HANDLING and perhaps DISBALE_NEWLINE_AUTO_RETURN will impact the behavior of at-end and off-end line writing and wrapping.

Also of interest: #2089 (related to how we measure the righthand side of the screen).

@carlos-zamora carlos-zamora added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Oct 7, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 7, 2019
@carlos-zamora carlos-zamora added this to the Terminal v1.0 milestone Oct 7, 2019
@aeiplatform
Copy link
Author

For pseudo terminal (PTY) such as screen pseudo terminal slave (PTS) works properly.
This unexpected behavior occurs only for the default teletypewriter (TTY).

@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 Oct 29, 2019
@DHowett-MSFT
Copy link
Contributor

Incidentally, this is probably the same root cause as the issue with powershell where when you type off the edge of the screen it wraps one line early but the cursor jumps to the wrong place, and then when you backspace over it it destroys the known universe.

@aeiplatform
Copy link
Author

On a new Windows build 18363.592 this issue got even weirder after increasing size of the terminal. Per every additional column of the terminal triggers duplication of the next line.
When the size is reduced, the terminal cuts off the original text, but after resizing again the terminal to the original size text is replaced by duplication of the next line.
So if you shrink the terminal, you will lose all the exceeding text.

I tried even explicit escaping, but without any differences.
printf "%*s\\n" "$(tput cols)" "" | tr " " "X"

@zadjii-msft
Copy link
Member

Woah okay this is pretty bad not just in Terminal, but in conhost too:

image

At first I thought this might be related to the disaster that is #4200/#405/#3490/#4354 but this might be lower than even all of them

@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Jan 30, 2020
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Jan 30, 2020
ghost pushed a commit that referenced this issue Feb 27, 2020
## Summary of the Pull Request

Changes how conpty emits text to preserve line-wrap state, and additionally adds rudimentary support to the Windows Terminal for wrapped lines.

## References

* Does _not_ fix (!) #3088, but that might be lower down in conhost. This makes wt behave like conhost, so at least there's that
* Still needs a proper deferred EOL wrap implementation in #780, which is left as a todo
* #4200 is the mega bucket with all this work
* MSFT:16485846 was the first attempt at this task, which caused the regression MSFT:18123777 so we backed it out.
* #4403 - I made sure this worked with that PR before I even sent #4403

## PR Checklist
* [x] Closes #405
* [x] Closes #3367 
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

I started with the following implementation:
When conpty is about to write the last column, note that we wrapped this line here. If the next character the vt renderer is told to paint get is supposed to be at the start of the following line, then we know that the previous line had wrapped, so we _won't_ emit the usual `\r\n` here, and we'll just continue emitting text.

However, this isn't _exactly_ right - if someone fills the row _exactly_ with text, the information that's available to the vt renderer isn't enough to know for sure if this line broke or not. It is possible for the client to write a full line of text, with a `\n` at the end, to manually break the line. So, I had to also add the `lineWrapped` param to the `IRenderEngine` interface, which is about half the files in this changelist.

## Validation Steps Performed
* Ran tests
* Checked how the Windows Terminal behaves with these changes
* Made sure that conhost/inception and gnome-terminal both act as you'd expect with wrapped lines from conpty
@zadjii-msft zadjii-msft self-assigned this Mar 18, 2020
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase and removed Product-Terminal The new Windows Terminal. labels Mar 26, 2020
@DHowett-MSFT DHowett-MSFT changed the title WSL Terminal Line Endings WSL Terminal Line Endings (the "exact wrap" bug) Apr 9, 2020
zadjii-msft added a commit that referenced this issue Apr 10, 2020
@ghost ghost added the In-PR This issue has a related PR label Apr 15, 2020
@zadjii-msft zadjii-msft added this to the 22H1 milestone Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.14, 22H2 Mar 10, 2022
@zadjii-msft zadjii-msft removed the In-PR This issue has a related PR label Apr 6, 2023
@zadjii-msft zadjii-msft removed their assignment Apr 6, 2023
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Apr 6, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 25, 2023
DHowett pushed a commit that referenced this issue Sep 26, 2023
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 26, 2023
DHowett pushed a commit that referenced this issue Sep 26, 2023
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅

(cherry picked from commit 7474839)
Service-Card-Id: 90642727
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) 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-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
7 participants