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

ConPTY doesn't set KeyEvent::_virtualScanCode for certain characters like : (which bothers GoW less) #2873

Closed
perlmaster opened this issue Sep 24, 2019 · 20 comments · Fixed by #3199
Labels
Area-Input Related to input processing (key presses, mouse, etc.) 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-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

Comments

@perlmaster
Copy link

Environment

Windows 10
Any other software?


# Steps to reproduce

1. Run cmd.exe under windows terminal preview
2. navigate to a directory containing text files
3. Run the command "less -MN *.txt"
4. When you reach the end of the 1st file enter the "less" command ":n" to go to the next file

# Expected behavior
"less" should start displaying the next file.

# Actual behavior
I get the following error message "no previous regular expression". This does not happen when I run less under a cmd.exe session the old fashioned way.
@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 Sep 24, 2019
@DHowett-MSFT
Copy link
Contributor

less doesn't ship with Windows, so you're going to need to share exactly which less you are using. Otherwise, we can't exactly debug the issue.

@DHowett-MSFT DHowett-MSFT added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 24, 2019
@perlmaster
Copy link
Author

according to "less --version" I am running less version 378

@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 Sep 24, 2019
@zadjii-msft
Copy link
Member

I think @DHowett-MSFT meant more of "are you using wsl's less, cygwin's less, some unknown less.exe?"

@zadjii-msft zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Sep 24, 2019
@perlmaster
Copy link
Author

I downloaded "less" a long time ago. I don't remember exactly where I got it.

The command "less --version" produces the following

less 378
Copyright (C) 2002 Mark Nudelman

less comes with NO WARRANTY, to the extent permitted by law.
For information about the terms of redistribution,
see the file named README in the less distribution.
Homepage: http://www.greenwoodsoftware.com/less

In the documentation I got at the time, there is the following paragraph:

   Copyright (C) 2002  Mark Nudelman

   less is part of the GNU project and is free software.  You
   can redistribute it and/or modify it under  the  terms  of
   either  (1) the GNU General Public License as published by
   the Free Software Foundation; or  (2)  the  Less  License.

Although I have cygwin, I do not use it.

@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 Sep 24, 2019
@zadjii-msft
Copy link
Member

zadjii-msft commented Sep 26, 2019

So the less I have on WSL is just about 100 versions more updated - could you maybe try a newer less version?

zadjii@migrie-slaptop:~$ less --version
less 487 (GNU regular expressions)
Copyright (C) 1984-2016  Mark Nudelman

I can only find posts about sed creating that "no previous regular expression" error, so I'm not really sure how that's getting into less, or how the terminal might be feeding bad characters into it.

What keyboard layout are you using?

@zadjii-msft zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Sep 26, 2019
@perlmaster
Copy link
Author

I must be having a slow day. Who or what is WSL ?

@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 Sep 26, 2019
@perlmaster
Copy link
Author

Sorry, it just occured to me that WSL == Windows Subsystem for Linux

@perlmaster
Copy link
Author

Does anyone know where I can find a compiled binary version of less ? So far the only thing I have found is source code and I have always had issues trying to compile anything that requires the bourne shell to run a script named "configure" which starts the whole build process. On my windows 10 64 bit system I can't get sh.exe to run properly

@DHowett-MSFT
Copy link
Contributor

That's a really good question, honestly. It looks like the GoW project produces binaries of these tools with reasonable frequency, but their terminal emulator bridge leaves a little bit to be desired. Perhaps their less works slightly better than the one you've got?

Sorry, I know it's not much. 😄

@DHowett-MSFT DHowett-MSFT added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Oct 7, 2019
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 12, 2019
@ghost
Copy link

ghost commented Oct 12, 2019

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@perlmaster
Copy link
Author

The GoW version of less did not solve the problem. I am continuing my search for some kind of solution.

@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 No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Oct 12, 2019
@DHowett-MSFT
Copy link
Contributor

That is very good news for us, because now we have learned of a distribution of less that we can use for a repro. Thank you!

What keyboard layout are you using? Standard en-US PC104?

@perlmaster
Copy link
Author

I am not sure about the exact specs for my keyboard, but it is definitely en-US. I am unable to determine if it is PC104 , I have checked in the control panels and nothing speaks of PC104

@perlmaster
Copy link
Author

just for the record I have the same issue wether I use my laptop keyboard or my wired USB keyboard

@0xd4d
Copy link

0xd4d commented Oct 12, 2019

Try less that's part of Git for Windows

@perlmaster
Copy link
Author

I already had git for windows 2.15.0 on my system. I was not aware that it had all those extra executables. It had less version 481. That version of less seems to be working just fine !

That solves my specific issue. Thanks to all who helped !

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Oct 12, 2019

And we've got a root cause, too!

It looks like some versions of less for windows are checking the scancode of the keyboard key that's pressed, and when that key comes in off a pty (which Windows Terminal, VSCode, Visual Studio, etc. use), that scancode isn't set.

pseudoconsole

↓ wch:0 '\0' mod:Shift (16) repeat:1 vk:16 vsc:42
↓ wch:0 '\0' mod:Shift (16) repeat:1 vk:16 vsc:42
↓ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:0 <--- bad (0)
↓ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:0 <---
↑ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:0 <---
↑ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:0 <---
↑ wch:0 '\0' mod:None (0) repeat:1 vk:16 vsc:42
↑ wch:0 '\0' mod:None (0) repeat:1 vk:16 vsc:42

conhost

↓ wch:0 '\0' mod:Shift (16) repeat:1 vk:16 vsc:42
↓ wch:0 '\0' mod:Shift (16) repeat:1 vk:16 vsc:42
↓ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:39 <--- good (39)
↓ wch:58 ':' mod:Shift (16) repeat:1 vk:186 vsc:39 <--- good (39)
↑ wch:0 '\0' mod:None (0) repeat:1 vk:16 vsc:42
↑ wch:0 '\0' mod:None (0) repeat:1 vk:16 vsc:42

@DHowett-MSFT DHowett-MSFT changed the title Bug keyboard mapping problem with windows terminal preview ConPTY doesn't set KeyEvent::_virtualScanCode for certain characters like : (which bothers GoW less) Oct 12, 2019
@DHowett-MSFT DHowett-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. Product-Conpty For console issues specifically related to conpty and removed Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 12, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 12, 2019
@DHowett-MSFT DHowett-MSFT added this to the 20H1 milestone Oct 12, 2019
@DHowett-MSFT DHowett-MSFT added the Priority-2 A description (P2) label Oct 12, 2019
@DHowett-MSFT
Copy link
Contributor

And as a final followup, it's because somewhere deep in the console host, we're doing a "vkey to vsc" translation, but we're providing it a character instead of a vkey. 🤦‍♂

@DHowett-MSFT
Copy link
Contributor

diff --git a/src/types/convert.cpp b/src/types/convert.cpp
index c2049ded9..bc87bab18 100644
--- a/src/types/convert.cpp
+++ b/src/types/convert.cpp
@@ -206,7 +206,8 @@ std::deque<std::unique_ptr<KeyEvent>> SynthesizeKeyboardEvents(const wchar_t wch
                                                        SHIFT_PRESSED));
     }

-    const WORD virtualScanCode = gsl::narrow<WORD>(MapVirtualKeyW(wch, MAPVK_VK_TO_VSC));
+    auto vk = keyState & 0xFF;
+    const WORD virtualScanCode = gsl::narrow<WORD>(MapVirtualKeyW(vk, MAPVK_VK_TO_VSC));
     KeyEvent keyEvent{ true, 1, LOBYTE(keyState), virtualScanCode, wch, 0 };

     // add modifier flags if necessary

@ghost ghost added the In-PR This issue has a related PR label Oct 15, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements 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 Oct 17, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 17, 2019
@ghost
Copy link

ghost commented Oct 23, 2019

🎉This issue was addressed in #3199, which has now been successfully released as Windows Terminal Preview v0.6.2951.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) 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-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
Development

Successfully merging a pull request may close this issue.

3 participants