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

Understand why the PTY might delete and then re-add spaces on certain lines #5430

Open
DHowett-MSFT opened this issue Apr 20, 2020 · 0 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Apr 20, 2020

Notes from #5398

SPACES REMOVED WHEN:
* line was not wrapped

AND

* It is optimal to prefer ECH/EL
* We are not on a new bottom line
* We did not clear all this frame
OR
* We cleared all this frame
OR
* It is a new bottom line and we are on it

SPACES RE-ADDED WHEN:
* It is not optimal to prefer ECH/EL
OR
* It is a new bottom line
OR
* We cleared all this frame

AND
* It is a new bottom line and we are on it
* The line had spaces at the end.
* We deleted the spaces previously <<< NEW IN #5398

Originally posted by @DHowett-MSFT in #5398 (comment)

While we're looking at this, we'll want to investigate how to consolidate the space add/remove logic so that we can make maximal use of the buffer already passed to us (like: write it out in full if it has spaces we want to keep, etc.)

Notes from @zadjii-msft

I can't remember tbh. This screams organic growth to me - I think we first just decided to do ECH and removing spaces, when we didn't have wrapping (and spaces didn't matter). Then we did some deferring the cursor movement & not erasing optimizations in 20H1, which added cases where we wouldn't ECH. Now we're adding cases where we still need the spaces. I suppose if we'd written it from scratch now, accounting for wrapping, we'd do it better.

I'd TODO it tbh.

I bet we were trying to optimize out the cursor positioning that we'd do after a new line is printed. If we did optimize that scenario, and used ECH to erase some spaces, we also used CUF to move the cursor. However, if we couldn't do that, we'd still want to leave the cursor in the right place...

yea we probably should have just not removed them in the first place.

@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 Apr 20, 2020
@DHowett-MSFT DHowett-MSFT added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Apr 20, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 20, 2020
@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 Apr 20, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.x milestone Apr 20, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

No branches or pull requests

3 participants