Fix Control+Space not sent to program running in terminal #16298

lonnywong · 2023-11-12T18:02:23Z

Converts null byte to specific input event, so that it's properly delivered to the program running in the terminal.

lonnywong · 2023-11-13T14:56:55Z

This is a test program written in go:

package main

import (
	"fmt"
	"os"
	"syscall"

	"golang.org/x/sys/windows"
	"golang.org/x/term"
)

func main() {
	inHandle, err := syscall.GetStdHandle(syscall.STD_INPUT_HANDLE)
	if err != nil {
		fmt.Printf("GetStdHandle: %v\r\n", err)
		return
	}
	var inMode uint32
	if err := windows.GetConsoleMode(windows.Handle(inHandle), &inMode); err != nil {
		fmt.Printf("GetConsoleMode: %v\r\n", err)
		return
	}
	defer windows.SetConsoleMode(windows.Handle(inHandle), inMode)
	if err := windows.SetConsoleMode(windows.Handle(inHandle), inMode|windows.ENABLE_VIRTUAL_TERMINAL_INPUT); err != nil {
		fmt.Printf("SetConsoleMode: %v\r\n", err)
		return
	}

	state, err := term.MakeRaw(int(os.Stdin.Fd()))
	if err != nil {
		fmt.Printf("MakeRaw: %v\r\n", err)
		return
	}
	defer term.Restore(int(os.Stdin.Fd()), state)

	buf := make([]byte, 100)
	for {
		n, err := os.Stdin.Read(buf)
		if err != nil {
			fmt.Printf("os.Stdin.Read(buf): %v\r\n", err)
			return
		}
		if n > 0 {
			fmt.Printf("%v\r\n", buf[:n])
		} else {
			fmt.Println("nil")
		}
		if n == 1 && buf[0] == '\x03' {
			return
		}
	}
}

src/host/inputBuffer.cpp

tusharsnx

Just a tiny nit.

I think we can pull this if block out of the loop if we assume NUL always comes as a single wchar_t:

        // If the text is just a null character, synthesize a key event that
        // exactly matches the null key event.
        if (const auto wch = til::at(text, 0); wch == UNICODE_NULL)
        {
            const auto zeroKey = OneCoreSafeVkKeyScanW(0);
            const auto vkey = LOWORD(zeroKey); 

            uint32_t modifiers = 0;
            if (WI_IsFlagSet(zeroKey, 0x100))
            {
                modifiers |= SHIFT_PRESSED;
            }
            if (WI_IsFlagSet(zeroKey, 0x200))
            {
                modifiers |= CTRL_PRESSED;
            }
            if (WI_IsFlagSet(zeroKey, 0x400))
            {
                modifiers |= ALT_PRESSED;
            }

            _storage.push_back(SynthesizeKeyEvent(true, 1, vkey, 0, wch, modifiers));
            return;
        }

        for (const auto& wch : text)
        {
            _storage.push_back(SynthesizeKeyEvent(true, 1, 0, 0, wch, 0));
        }

Haven't tested the code so take it with a grain of salt 😅

lonnywong · 2023-11-18T15:24:10Z

I think we can pull this if block out of the loop if we assume NUL always comes as a single wchar_t:

@tusharsnx Do we need to do it before the for loop? Is it possible that it is an input simulated by another program?

src/host/inputBuffer.cpp

tusharsnx · 2023-11-18T16:11:22Z

@lonnywong

Do we need to do it before the for loop? Is it possible that it is an input simulated by another program?

My thought process is that we process one inEvent, get the set of characters that is generated, and with that call _HandleTerminalInputCallback().

Since a null inEvent generates a single \0 (using _makeCharOutput(0)), we can assume it'll always be one character sent to the _HandleTerminalInputCallback(). That being said, it's worth some testing before making the change 🙂

lonnywong · 2023-11-18T16:57:01Z

@tusharsnx Is it the same if we do it in the for loop? There are two more instructions ( test and jmp ) in each loop. But the performance impact should be negligible. Doing in the for loop also brings benefits: If some places are modified in the future, it will be compatible here.

j4james · 2023-11-18T17:54:10Z

Since a null inEvent generates a single \0 (using _makeCharOutput(0)), we can assume it'll always be one character sent to the _HandleTerminalInputCallback().

I'm fairly sure you could also get a string like \x1B\x00 from a Ctrl+Alt+Space keypress. I don't think you can rely on the \0 always being the first character.

lonnywong · 2023-11-18T18:02:09Z

Since a null inEvent generates a single \0 (using _makeCharOutput(0)), we can assume it'll always be one character sent to the _HandleTerminalInputCallback().

I'm fairly sure you could also get a string like \x1B\x00 from a Ctrl+Alt+Space keypress. I don't think you can rely on the \0 always being the first character.

I get \x1b\x00 from ctrl + alt + shift + 2 on my computer too.

lhecker · 2023-11-18T18:09:21Z

FWIW I personally don't think this is the right approach, as it just works around the if condition in stream.cpp:

terminal/src/host/stream.cpp

Lines 234 to 244 in cc2ba53

    
           const auto zeroKey = OneCoreSafeVkKeyScanW(0); 
        
           if (LOBYTE(zeroKey) == Event.Event.KeyEvent.wVirtualKeyCode && 
        
               WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, ALT_PRESSED) == WI_IsFlagSet(zeroKey, 0x400) && 
        
               WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, CTRL_PRESSED) == WI_IsFlagSet(zeroKey, 0x200) && 
        
               WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, SHIFT_PRESSED) == WI_IsFlagSet(zeroKey, 0x100)) 
        
           { 
        
               // This really is the character 0x0000 
        
               *pwchOut = Event.Event.KeyEvent.uChar.UnicodeChar; 
        
               return STATUS_SUCCESS; 
        
           }

Additionally, it introduces a disparity between how \0 is presented to a console application vs. any other character. Because now only \0 gets a vkey while all others don't.

Unfortunately, I can't really criticize this PR for it, because the framework for "my way" doesn't exist yet. Right now, InputBuffer is basically a wrapper around a

std::deque<INPUT_RECORD>

This is fairly limiting for us, because it requires us to translate raw strings into INPUT_RECORDs just to turn them back into strings if a function like GetChar in stream.cpp is called. Instead, I'd prefer if we had a

struct StringRecord {
    std::wstring str;
    size_t consumed = 0;
};

so that we can have a

std::deque<std::variant<INPUT_RECORD, StringRecord>>

That way we can store inputs like the above as their raw strings and extract them them as raw strings without any uncertainty over whether any given INPUT_RECORD "truly stores a null character" or not.

I'm marking this for our upcoming (internal) discussion, because I'm curious what others think about this.

lonnywong · 2023-11-18T18:22:39Z

@lhecker Consider it a temporary solution until StringRecord is released?

lhecker · 2023-11-18T18:24:37Z

Oh yeah, please don't get me wrong: I do think it's acceptable in the meantime. The only thing I find somewhat concerning is the vkey thing I mentioned. But I'm not entirely sure how important that is.

lonnywong · 2023-11-18T18:39:09Z

Oh yeah, please don't get me wrong: I do think it's acceptable in the meantime. The only thing I find somewhat concerning is the vkey thing I mentioned. But I'm not entirely sure how important that is.

@lhecker You may want to check

terminal/src/terminal/input/terminalInput.cpp

Lines 610 to 646 in cc2ba53

    
           // Pressing the control key causes all bits but the 5 least 
        
           // significant ones to be zeroed out (when using ASCII). 
        
           // This results in Ctrl+Space and Ctrl+@ being equal to a null byte. 
        
           // Normally the C0 control code set only defines Ctrl+@, 
        
           // but Ctrl+Space is also widely accepted by most terminals. 
        
           // -> Send a "null input sequence" in that case. 
        
           // We don't need to handle other kinds of Ctrl combinations, 
        
           // as we rely on the caller to pretranslate those to characters for us. 
        
           if (!WI_IsAnyFlagSet(keyEvent.dwControlKeyState, ALT_PRESSED) && WI_IsAnyFlagSet(keyEvent.dwControlKeyState, CTRL_PRESSED)) 
        
           { 
        
               const auto ch = keyEvent.uChar.UnicodeChar; 
        
               const auto vkey = keyEvent.wVirtualKeyCode; 
        
               // Currently, when we're called with Ctrl+@, ch will be 0, since Ctrl+@ equals a null byte. 
        
               // VkKeyScanW(0) in turn returns the vkey for the null character (ASCII @). 
        
               // -> Use the vkey to alternatively determine if Ctrl+@ is being pressed. 
        
               if (ch == UNICODE_SPACE || (ch == UNICODE_NULL && vkey == LOBYTE(OneCoreSafeVkKeyScanW(0)))) 
        
               { 
        
                   return _makeCharOutput(0); 
        
               } 
        
               // Not all keyboard layouts contain mappings for Ctrl-key combinations. 
        
               // For instance the US one contains a mapping of Ctrl+\ to ^\, 
        
               // but the UK extended layout doesn't, in which case ch is null. 
        
               if (ch == UNICODE_NULL) 
        
               { 
        
                   // -> Try to infer the character from the vkey. 
        
                   auto mappedChar = LOWORD(OneCoreSafeMapVirtualKeyW(keyEvent.wVirtualKeyCode, MAPVK_VK_TO_CHAR)); 
        
                   if (mappedChar) 
        
                   { 
        
                       // Pressing the control key causes all bits but the 5 least 
        
                       // significant ones to be zeroed out (when using ASCII). 
        
                       mappedChar &= 0b11111; 
        
                       return _makeCharOutput(mappedChar); 
        
                   } 
        
               } 
        
           }

tusharsnx

LGTM, and wait for the discussion to be over, I think it'll be helpful in guiding any additional changes we might require.

@lonnywong Thanks for the PR! This not only fixes the mentioned issue but also highlights a much needed change in the inputBuffer to disambiguate a VT-translated input vs an actual KeyEvent. 😃

j4james

I'm in agreement with lhecker that it would be better if we could get rid of these translations altogether, but if that's not likely to happen soon, this PR seems like a reasonable short term fix.

lonnywong · 2023-11-19T22:22:20Z

@tusharsnx @j4james Thanks for your help. Is it okay to be merged?

j4james · 2023-11-19T22:46:10Z

@lonnywong You'll need approvals from two of the core team members to get the PR merged. Approval from outside contributors doesn't usually count. And I think lhecker was saying that they were going to discuss this issue internally before making a decision.

DHowett

@lhecker brought this up with the team today, and while it does introduce another special case translation I think I'm okay with it. We should try to solve the problem all-up at some future point...

Thanks for all the discussion @j4james and the review notes @tusharsnx 😄

DHowett · 2023-11-21T03:50:28Z

@lonnywong thanks for your contribution as well, of course!

Converts null byte to specific input event, so that it's properly delivered to the program running in the terminal. Closes #15939 (cherry picked from commit 8747a39) Service-Card-Id: 91201444 Service-Version: 1.19

GitMurf · 2024-01-20T16:20:36Z

I am really bad at following PRs to their release ;) is this something that has landed? if not, any ideas when it will? I would like to be able to confirm the fix solved the problem as I have this problem using neovim in windows terminal. Thanks!

GitMurf · 2024-01-20T16:43:03Z

I found the canary branch and I installed that and the control-space issue does not seem to be solved for my case using neovim at least. Should this fix be expected to be landed in the canary branch by now? Thanks!

lonnywong · 2024-01-20T16:53:13Z

I found the canary branch and I installed that and the control-space issue does not seem to be solved for my case using neovim at least. Should this fix be expected to be landed in the canary branch by now? Thanks!

@GitMurf Which version of ssh are you using? You can try #2865 (comment).
Or, you can try https://github.com/trzsz/trzsz-ssh ( an openssh client alternative ), it works for me.

lonnywong · 2024-01-20T17:18:09Z

@GitMurf Are you using nvim locally, without ssh?
Is ctrl + space a shortcut key for switching input methods?

GitMurf · 2024-01-21T15:23:40Z

@GitMurf Are you using nvim locally, without ssh?

Is ctrl + space a shortcut key for switching input methods?

@lonnywong i am not using ssh. Just straight nvim.exe locally. For example I currently am using the nightly build using the zip source from here: https://github.com/neovim/neovim/releases/tag/nightly

What do you mean about switching input methods?

escritorio-gustavo · 2024-02-06T13:27:38Z

I am really bad at following PRs to their release ;) is this something that has landed? if not, any ideas when it will? I would like to be able to confirm the fix solved the problem as I have this problem using neovim in windows terminal. Thanks!

Seems like this is the release that includes the fix https://github.com/microsoft/terminal/releases/tag/v1.20.10293.0

BPplays · 2024-08-13T19:43:18Z

@GitMurf ctrl + space is seems to also not be getting through to neovim for me too, did you manage to fix it. my wt version is 1.21.1772.0

What do you mean about switching input methods?

i think what they were talking about was that ctrl + space is an option for switching input method,
for example i have english and japanese as input methods that i can switch between with alt + shift

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Input Related to input processing (key presses, mouse, etc.) Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Nov 12, 2023

lonnywong force-pushed the main branch 2 times, most recently from a195939 to 3b31f63 Compare November 13, 2023 14:41

lonnywong changed the title ~~Fix Control+Space not sent to program running in terminal (#15939)~~ Fix Control+Space not sent to program running in terminal Nov 13, 2023

This was referenced Nov 14, 2023

Control+Space not sent to program running in terminal #15939

Closed

响应ctrl+space问题 trzsz/trzsz-ssh#5

Closed

j4james reviewed Nov 18, 2023

View reviewed changes

src/host/inputBuffer.cpp Outdated Show resolved Hide resolved

lonnywong force-pushed the main branch from 3b31f63 to d20f36c Compare November 18, 2023 15:16

tusharsnx reviewed Nov 18, 2023

View reviewed changes

j4james reviewed Nov 18, 2023

View reviewed changes

src/host/inputBuffer.cpp Outdated Show resolved Hide resolved

Fix Control+Space not sent to program running in terminal

96e8afa

lonnywong force-pushed the main branch from d20f36c to 96e8afa Compare November 18, 2023 15:48

lhecker added the Needs-Discussion Something that requires a team discussion before we can proceed label Nov 18, 2023

lonnywong requested review from j4james and tusharsnx November 18, 2023 21:30

tusharsnx approved these changes Nov 19, 2023

View reviewed changes

j4james approved these changes Nov 19, 2023

View reviewed changes

DHowett approved these changes Nov 21, 2023

View reviewed changes

lhecker approved these changes Nov 27, 2023

View reviewed changes

zadjii-msft merged commit 8747a39 into microsoft:main Nov 27, 2023
12 of 14 checks passed

zadjii-msft added zInbox-Bug Ignore me! and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Nov 27, 2023

This was referenced Jan 20, 2024

Windows: console: <C-2>, <C-/> neovim/neovim#8435

Open

to_fuzzy_refine does not work with <C-Space> mapping in Powershell in Windows Terminal nvim-telescope/telescope.nvim#2646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Control+Space not sent to program running in terminal #16298

Fix Control+Space not sent to program running in terminal #16298

lonnywong commented Nov 12, 2023 •

edited

Loading

lonnywong commented Nov 13, 2023

tusharsnx left a comment

lonnywong commented Nov 18, 2023 •

edited

Loading

tusharsnx commented Nov 18, 2023 •

edited

Loading

lonnywong commented Nov 18, 2023

j4james commented Nov 18, 2023

lonnywong commented Nov 18, 2023

lhecker commented Nov 18, 2023 •

edited

Loading

lonnywong commented Nov 18, 2023

lhecker commented Nov 18, 2023

lonnywong commented Nov 18, 2023

tusharsnx left a comment •

edited

Loading

j4james left a comment

lonnywong commented Nov 19, 2023

j4james commented Nov 19, 2023

DHowett left a comment

DHowett commented Nov 21, 2023

GitMurf commented Jan 20, 2024

GitMurf commented Jan 20, 2024

lonnywong commented Jan 20, 2024

lonnywong commented Jan 20, 2024

GitMurf commented Jan 21, 2024 •

edited

Loading

escritorio-gustavo commented Feb 6, 2024

BPplays commented Aug 13, 2024 •

edited

Loading

Fix Control+Space not sent to program running in terminal #16298

Fix Control+Space not sent to program running in terminal #16298

Conversation

lonnywong commented Nov 12, 2023 • edited Loading

lonnywong commented Nov 13, 2023

tusharsnx left a comment

Choose a reason for hiding this comment

lonnywong commented Nov 18, 2023 • edited Loading

tusharsnx commented Nov 18, 2023 • edited Loading

lonnywong commented Nov 18, 2023

j4james commented Nov 18, 2023

lonnywong commented Nov 18, 2023

lhecker commented Nov 18, 2023 • edited Loading

lonnywong commented Nov 18, 2023

lhecker commented Nov 18, 2023

lonnywong commented Nov 18, 2023

tusharsnx left a comment • edited Loading

Choose a reason for hiding this comment

j4james left a comment

Choose a reason for hiding this comment

lonnywong commented Nov 19, 2023

j4james commented Nov 19, 2023

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Nov 21, 2023

GitMurf commented Jan 20, 2024

GitMurf commented Jan 20, 2024

lonnywong commented Jan 20, 2024

lonnywong commented Jan 20, 2024

GitMurf commented Jan 21, 2024 • edited Loading

escritorio-gustavo commented Feb 6, 2024

BPplays commented Aug 13, 2024 • edited Loading

lonnywong commented Nov 12, 2023 •

edited

Loading

lonnywong commented Nov 18, 2023 •

edited

Loading

tusharsnx commented Nov 18, 2023 •

edited

Loading

lhecker commented Nov 18, 2023 •

edited

Loading

tusharsnx left a comment •

edited

Loading

GitMurf commented Jan 21, 2024 •

edited

Loading

BPplays commented Aug 13, 2024 •

edited

Loading