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

Automatically triggered Replace mode using vim in OpenSSH on cmd / powershell though Windows Terminal #1637

Closed
SuzunaeHoumi opened this issue Jun 26, 2019 · 42 comments · Fixed by #7583
Assignees
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@SuzunaeHoumi
Copy link

SuzunaeHoumi commented Jun 26, 2019

Replace input mode will triggered automatically when using vim though OpenSSH Client in cmd.

Environment

Windows build number: [Version 10.0.18362.175]
Windows Terminal version (if applicable): 0.2.1715.0

Remote Linux PC: Ubuntu LTS 1604

OpenSSH Client: Stock version in Windows 10 1903 Build 18362.175

Steps to reproduce

  1. Open Windows Terminal
  2. Open new tab running cmd / powershell / powershell 6
  3. ssh to a Ubuntu 1604 LTS PC
  4. Open any document using vim

Expected behavior

Not trigger any input mode, the status bar at the bottom displays the path of the file that vim opened.

Actual behavior

Triggered Replace input mode.

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

None of us can reproduce this. Is there anything else unusual about your machine, about your vim configuration, or about your keyboard layout?

@DHowett-MSFT DHowett-MSFT added Area-Input Related to input processing (key presses, mouse, etc.) Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. Product-Terminal The new Windows Terminal. Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 26, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 26, 2019
@DHowett-MSFT
Copy link
Contributor

Also, does it still happen if you use wsl ssh?

@SuzunaeHoumi
Copy link
Author

Also, does it still happen if you use wsl ssh?

No, it would not happened when I use wsl ssh

@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 27, 2019
@SuzunaeHoumi
Copy link
Author

None of us can reproduce this. Is there anything else unusual about your machine, about your vim configuration, or about your keyboard layout?

Keyboard Layout is simple Standard US Layout

@SuzunaeHoumi SuzunaeHoumi changed the title Automatically triggered Replace mode using vim in OpenSSH on cmd / Powershell though Windows Terminal Automatically triggered Replace mode using vim in OpenSSH on cmd though Windows Terminal Jun 27, 2019
@SuzunaeHoumi SuzunaeHoumi changed the title Automatically triggered Replace mode using vim in OpenSSH on cmd though Windows Terminal Automatically triggered Replace mode using vim in OpenSSH on cmd / powershell though Windows Terminal Jun 27, 2019
@jgeorgeson
Copy link

[Un]Fortunately I don't think this is a Windows Terminal issue. I see the same behavior using ConEmu on my work machine (not updated to W10 1903 so can't install Windows Terminal there), but not in either Windows Terminal or ConEmu on my home machine (W10 1903). Both machines have Standard US Layout and both English US and English US International input languages. I see no change in this behavior on either machine based on selected input langauge.

@DHowett-MSFT
Copy link
Contributor

If it doesn't happen in wsl ssh, but it does in win32-openssh, I'd go to their repository and complain there. Thanks!

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jun 28, 2019
@DHowett-MSFT DHowett-MSFT added Resolution-External For issues that are outside this codebase and removed Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. labels Jun 28, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 28, 2019
@linuxcyberlabs
Copy link

Seems to be an issue with utf-8 ambiguous characters and Windows cmd console. Flag t_u7 is set by default and so vim will request cursor position and get a bad reply from the ssh client.

Workaround: Adding set t_u7= or set ambw=double to your vimrc should fix the problem. set t_u7= will disable requesting cursor position and ambw=double will set the ambiguous characters mode to double.

For more info see vim reference manual: https://vimhelp.org/term.txt.html

Credits: https://superuser.com/a/1525060

set ambw=double appears to have conflict with airline/powerline characters. I see extra spaces after the <| arrows. set t_u7= works though! – David Woods Jul 9 at 17:12

@rsynnest
Copy link

Windows build number: 10.0.19041.0
Windows Terminal version: 1.2.2022.0 (Preview Build)
WSL2 distribution: Ubuntu 18.04.4 LTS and Debian 10 (both affected)

This bug happens in WSL for local sessions as well as remote SSH sessions when running the Terminal Preview build v1.2.2022.0. The latest non-preview release of Terminal v1.1.2021.0 does not inflict this bug on WSL sessions. A note on reproducibility - this happens at random, around 1 in 5 times I open vim it opens in Replace mode.

This comment fixed the issue for me, but is needed in my local .vimrc as well as any remote servers I'm working on. Another fix is to install and use Neovim which is unaffected by this, presumably because it uses utf-8 internally for everything.

@vladsol
Copy link

vladsol commented Aug 12, 2020

Windows 10.

Windows Terminal Preview
Version: 1.2.2022.0

Remote SSH (using windows ssh) -> vim -> starting in Replace mode :(

@DHowett
Copy link
Member

DHowett commented Sep 8, 2020

@FremdSprach #1637 (comment)

This bug was only reopened a few days ago, after it was initially closed thanks to (what appeared to be) Win32-OpenSSH having a similar issue.

Since then, it’s been a weekend followed by a US holiday, which has occupied most of our US-based team’s time 😄

(Edit: whoa, it has been a week since I reopened this.)

@DHowett
Copy link
Member

DHowett commented Sep 8, 2020

Now, that might seem like a weird diagnosis. Until recently, win32-openssh implemented its own terminal emulator! It also implemented its own VT input generator.

@pujfei
Copy link

pujfei commented Sep 8, 2020

Ah, Labor Day, that makes it. It must be a sound and good break without labor~~

Suppose God the savior himself take a same holiday, people across the world would get nobody to pray to. You guys are Savior to the WT-addicted die-hard fans. Hope God's got no holidays and wish you be blessed Thor's hammer 🔨 and hammer it hard 😁

@SuzunaeHoumi
Copy link
Author

Just surprised this issue has reopened after a year ago.
Since was told it was an issue in Win32-OpenSSH, then just ignored this issue when it happens.
Hum.

@pujfei
Copy link

pujfei commented Sep 8, 2020

Historical bug awakening the first dragon-rider @SuzunaeHoumi 😀

@DHowett DHowett removed the Resolution-External For issues that are outside this codebase label Sep 8, 2020
@DHowett DHowett added this to the Terminal v2.0 milestone Sep 8, 2020
@DHowett
Copy link
Member

DHowett commented Sep 8, 2020

Okay, here's what's happening.

When we receive a DSR(CPR), the console host attempts to prepend the CPR to the input buffer here:

const auto response = wil::str_printf<std::wstring>(L"\x1b[%d;%dR", coordCursorPos.Y, coordCursorPos.X);
success = _WriteResponse(response);

size_t eventsWritten;
success = _pConApi->PrivatePrependConsoleInput(inEvents, eventsWritten);

Because of the design of the input queue and how it has to remain compatible with ReadConsoleInput, the CPR is transformed into a series of KEY_EVENT_RECORDs -- one down and one up each for the entire sequence. Just like a user had pressed each of those keys.

Now, this poses a problem for two CPRs being received back-to-back: the second CPR will cause the later position to be prepended to the input queue before the earlier position.

If an application has read input between the two CPRs -- even a single keypress, it will certainly encounter this issue.

In my testing, I saw that the input queue at the time of the second prepend contained...

ESC up
[ DOWN
[ UP
2 DOWN
2 UP
...

indicating that only the first keypress (esc down) had been read and popped out of the queue.

The resultant sequence was...

^[ ^[  [  [  3  3  ;  ;  3  3  R  R ^[  [  [  2  2  ;  ;  2  2  R  R
DN UP DN UP DN UP DN UP DN UP DN UP UP DN UP DN UP DN UP DN UP DN UP
                                    ^^ frame-shifted start of prior event

Because VT input events are usually read on the keydown event, the first ESC has already been totally spooled into the application.

I'll need to wait for my team to be back from vacation/leave to figure out why we went with a prepend, as that decision predates me. 😄

@DHowett
Copy link
Member

DHowett commented Sep 9, 2020

Reader's notes:

Prepend seems like it was an intentional choice -- we do it in a number of places (every DSR, DA, DA2, DA3, VT52 Identify, DSR-OS, CPR) -- but I do not believe that it is ever correct.

It was introduced on 2017-10-17 in a pull request (!997738) that introduced support for window manipulation sequences over the pty, which we've never documented and quickly replaced with the signal channel. (This will be of particular interest to the folks having the purity discussion about sending an ED down the input handle 😁).

commit [REDACTED]
Author: @zadjii-msft
Date:   Thu Oct 12 13:03:30 2017 -0700

    We need seperate Append and Perpend WriteInputs

      The InteractDispatch wants to append to the buffer,
      but the adapter needs to prepend to the buffer.

      Tests still don't build.

This comment suggests that we had to add a separate prepend so that InteractDispatch could append. Because this pull request mainly dealt with InteractDispatch, the implication is that the "adapter" was already prepending and we needed to add Prepend...Buffer to disentangle the prepending behavior that "adapter" was using.

However, the code change is fairly obvious in the "adapter". It explicitly goes from appending to prepending with this diff:

diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index xxxxxxxxx..yyyyyyyyy 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -932,7 +932,7 @@ bool AdaptDispatch::_WriteResponse(_In_reads_(cReply) PCWSTR pwszReply, _In_ siz
     if (cInputBuffer > 0)
     {
         DWORD dwWritten = 0;
-        fSuccess = !!_pConApi->WriteConsoleInputW(rgInput, (DWORD)cInputBuffer, &dwWritten);
+        fSuccess = !!_pConApi->PrivatePrependConsoleInput(rgInput, (DWORD)cInputBuffer, &dwWritten);
     }

Mike's on paternity leave (yay!), so I can't exactly ask 😄

DHowett added a commit that referenced this issue Sep 9, 2020
This fixes an issue where two CPRs could end up corrupted in the input
buffer. An application that sent two CPRs back-to-back could
accidentally read the first few characters of the first prepended CPR
before handing us another CPR. We would dutifully prepend it to the
buffer, causing them to overlap.

```
^[^[2;2R[1;1R
^^      ^^^^^ First CPR
  ^^^^^^ Second CPR
```

Response prepending was implemented in !997738 without much comment.
There's very little in the way of audit trail as to why we switched.
Michael believes that we wanted to make sure that applications got DSR
responses immediately.

I discussed our options with him, and he suggested that we could
implement a priority queue in InputBuffer and make sure that "response"
input was dispatched to a client application before any application- or
user-generated input. This was deemed to be too much work.

We decided that DSR responses getting top billing was likely to be a
stronger guarantee than most terminals are capable of giving, and that
we should be fine if we just switch it back to append.

Fixes #1637.
@ghost ghost added the In-PR This issue has a related PR label Sep 9, 2020
@ghost ghost closed this as completed in #7583 Sep 9, 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 Sep 9, 2020
ghost pushed a commit that referenced this issue Sep 9, 2020
This fixes an issue where two CPRs could end up corrupted in the input
buffer. An application that sent two CPRs back-to-back could
end up reading the first few characters of the first prepended CPR
before handing us another CPR. We would dutifully prepend it to the
buffer, causing them to overlap.

```
^[^[2;2R[1;1R
^^      ^^^^^ First CPR
  ^^^^^^ Second CPR
```

The end result of this corruption is that a requesting application
would receive an unbidden `R` on stdin; for vim, this would trigger
replace mode immediately on startup.

Response prepending was implemented in !997738 without much comment.
There's very little in the way of audit trail as to why we switched.
Michael believes that we wanted to make sure that applications got DSR
responses immediately. It had the unfortunate side effect of causing
subsequence CPRs across cursor moves to come out in the wrong order.

I discussed our options with him, and he suggested that we could
implement a priority queue in InputBuffer and make sure that "response"
input was dispatched to a client application before any application- or
user-generated input. This was deemed to be too much work.

We decided that DSR responses getting top billing was likely to be a
stronger guarantee than most terminals are capable of giving, and that
we should be fine if we just switch it back to append.

Thanks to @k-takata, @Tekki and @brammool for the investigation on the
vim side.

Fixes #1637.
DHowett added a commit that referenced this issue Sep 18, 2020
This fixes an issue where two CPRs could end up corrupted in the input
buffer. An application that sent two CPRs back-to-back could
end up reading the first few characters of the first prepended CPR
before handing us another CPR. We would dutifully prepend it to the
buffer, causing them to overlap.

```
^[^[2;2R[1;1R
^^      ^^^^^ First CPR
  ^^^^^^ Second CPR
```

The end result of this corruption is that a requesting application
would receive an unbidden `R` on stdin; for vim, this would trigger
replace mode immediately on startup.

Response prepending was implemented in !997738 without much comment.
There's very little in the way of audit trail as to why we switched.
Michael believes that we wanted to make sure that applications got DSR
responses immediately. It had the unfortunate side effect of causing
subsequence CPRs across cursor moves to come out in the wrong order.

I discussed our options with him, and he suggested that we could
implement a priority queue in InputBuffer and make sure that "response"
input was dispatched to a client application before any application- or
user-generated input. This was deemed to be too much work.

We decided that DSR responses getting top billing was likely to be a
stronger guarantee than most terminals are capable of giving, and that
we should be fine if we just switch it back to append.

Thanks to @k-takata, @Tekki and @brammool for the investigation on the
vim side.

Fixes #1637.

(cherry picked from commit cb037f3)
@ghost
Copy link

ghost commented Sep 22, 2020

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

Handy links:

@ghost
Copy link

ghost commented Sep 22, 2020

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

Handy links:

@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

This fix has also been ingested into Windows for a future release.

@electrofloat
Copy link

🎉This issue was addressed in #7583, which has now been successfully released as Windows Terminal v1.3.2651.0.🎉

Handy links:

* [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.3.2651.0)

* [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)

The "Handy links" here takes you to the Preview app, not to the non-preview app.

@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

http://aka.ms/terminal enjoy

@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

(sorry, I flippantly gave you the preview link anyway ;P)

@pujfei
Copy link

pujfei commented Sep 23, 2020

@DHowett Great work! Finally, and Wonderfully! Thanks so much!
Clouds fade away upon the sky and sunshine shines on earth again!

@plutonji
Copy link

Thanks!

@WSLUser
Copy link
Contributor

WSLUser commented Oct 2, 2020

This still sometimes happens where VIM reports being in replace mode over SSH. Othertimes it correctly goes into Insert Mode in v1.4

@DHowett
Copy link
Member

DHowett commented Oct 2, 2020

@WSLUser please file a separate bug following the bug template and note the versions of all software involved.

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 2, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.