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.

Sign up for GitHub

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

Jump to bottom

SSH then running a reset caused blank space #6545

Closed
NathanR1225 opened this issue Jun 17, 2020 · 8 comments · Fixed by #6763
Closed

SSH then running a reset caused blank space #6545

NathanR1225 opened this issue Jun 17, 2020 · 8 comments · Fixed by #6763
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Windows vNext

Comments

@NathanR1225
Copy link

NathanR1225 commented Jun 17, 2020 •
edited
Loading

Environment

Win 10
Windows Terminal Version: 1.0.1401.0

Any other software?
SSH?

Steps to reproduce

  1. SSH into Linux using power shell or Ubuntu terminal
  2. run some commands (to full console )
  3. run reset to clear console
    Show example in images

Expected behavior

When running reset the console should clear like it does but put the next line at the top of the screen.

Actual behavior

Used SSH to log into a piece of hardware running linux. When I ran the reset command it clears the screen but adds a blank new line above the terminal line. resizing the terminal from full screen to partial screen then back to full screen does remove some new lines. Seen in Power shell and Ubuntu terminal.
image
image
@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 Jun 17, 2020
@NathanR1225 NathanR1225 changed the title SSH then running a rest caused blank space SSH then running a reset caused blank space Jun 17, 2020
@DHowett
Copy link
Member

DHowett commented Jun 17, 2020

Would you mind sharing the output of...

infocmp | grep "rs[123]"

in a session where reset is acting up?

@DHowett DHowett added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 17, 2020
@NathanR1225
Copy link
Author

NathanR1225 commented Jun 17, 2020 •
edited by DHowett
Loading

Command was run in Ubuntu terminal

rs1=\Ec\E]104\007, rs2=\E[!p\E[?3;4l\E[4l\E>, sc=\E7,

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 17, 2020
@DHowett DHowett mentioned this issue Jun 17, 2020
Closed
@j4james
Copy link
Collaborator

j4james commented Jun 17, 2020

This is because AdaptDispatch::EraseInDisplay returns false in conpty mode, which prevents the remaining steps of the AdaptDispatch::HardReset method being executed. That include the cursor position not being reset to 1,1 so the conhost cursor ends up out of sync with the conpty client.

I can't help thinking this passthrough approach is wrong - I mean idea of "failing" in conpty mode as a way to trigger a passthrough. I'm also not convinced that we should be aborting command sequences (like HardReset) on the first failure. I'd much rather we just continued executing everything, and accumulated the failure status.

So instead of:

if (success)
{
    success = _EraseScrollback();
}

we could just have:

success = _EraseScrollback() && success;

That at least should fix this particular problem.

@DHowett
Copy link
Member

DHowett commented Jun 18, 2020

I also find the failure=passthrough behavior somewhat suspect. 😄

@DHowett
Copy link
Member

DHowett commented Jun 18, 2020

Thanks for digging into this!

@DHowett DHowett added Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 19, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 19, 2020
@DHowett DHowett removed the Needs-Attention The core contributors need to come back around and look at this ASAP. label Jun 19, 2020
@DHowett DHowett added this to the 21H1 milestone Jun 19, 2020
@j4james j4james mentioned this issue Jul 2, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Jul 2, 2020
@ghost ghost closed this as completed in #6763 Jul 6, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 6, 2020
ghost pushed a commit that referenced this issue Jul 6, 2020
@j4james
Don't abort early in VT reset operations if one of the steps fails (#…
0651fcf 
…6763)

The VT reset operations `RIS` and `DECSTR` are implemented as a series
of steps, each of which could potentially fail. Currently these
operations abort as soon as an error is detected, which is particularly
problematic in conpty mode, where some steps deliberately "fail" to
indicate that they need to be "passed through" to the conpty client. As
a result, the reset won't be fully executed. This PR changes that
behaviour, so the error state is recorded for any failures, but the
subsequent steps are still run.

Originally the structure of these operations was of the form:

    bool success = DoSomething();
    if (success)
    {
        success = DoSomethingElse();
    }

But I've now changed the code so it looks more like this:

    bool success = DoSomething();
    success = DoSomethingElse() && success;

This means that every one of the steps should execute, regardless of
whether previous steps were successful, but the final _success_ state
will only be true if none of the steps has failed.

While this is only really an issue in the conhost code, I've updated
both the `AdaptDispatch` and `TerminalDispatch` classes, since I thought
it would be best to have them in sync, and in general this seems like a
better way to handle multi-step operations anyway.

VALIDATION

I've manually tested the `RIS` escape sequence (`\ec`) in the Windows
Terminal, and confirmed that it now correctly resets the cursor
position, which it wasn't doing before.

Closes #6545
DHowett pushed a commit that referenced this issue Jul 7, 2020
@j4james @DHowett
Don't abort early in VT reset operations if one of the steps fails (#…
950fac8 
…6763)

The VT reset operations `RIS` and `DECSTR` are implemented as a series
of steps, each of which could potentially fail. Currently these
operations abort as soon as an error is detected, which is particularly
problematic in conpty mode, where some steps deliberately "fail" to
indicate that they need to be "passed through" to the conpty client. As
a result, the reset won't be fully executed. This PR changes that
behaviour, so the error state is recorded for any failures, but the
subsequent steps are still run.

Originally the structure of these operations was of the form:

    bool success = DoSomething();
    if (success)
    {
        success = DoSomethingElse();
    }

But I've now changed the code so it looks more like this:

    bool success = DoSomething();
    success = DoSomethingElse() && success;

This means that every one of the steps should execute, regardless of
whether previous steps were successful, but the final _success_ state
will only be true if none of the steps has failed.

While this is only really an issue in the conhost code, I've updated
both the `AdaptDispatch` and `TerminalDispatch` classes, since I thought
it would be best to have them in sync, and in general this seems like a
better way to handle multi-step operations anyway.

VALIDATION

I've manually tested the `RIS` escape sequence (`\ec`) in the Windows
Terminal, and confirmed that it now correctly resets the cursor
position, which it wasn't doing before.

Closes #6545

(cherry picked from commit 0651fcf)
@DHowett DHowett mentioned this issue Jul 14, 2020
Closed
@ghost
Copy link

ghost commented Jul 21, 2020

🎉This issue was addressed in #6763, which has now been successfully released as Windows Terminal v1.1.2021.0.:tada:

Handy links:

1 similar comment
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6763, which has now been successfully released as Windows Terminal v1.1.2021.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6763, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Windows vNext
Development

Successfully merging a pull request may close this issue.

Don't abort early in VT reset operations if one of the steps fails j4james/terminal
3 participants
@DHowett @j4james @NathanR1225