-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
GH-132439: REPL on Windows swallows characters entered via AltGr #132440
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
base: main
Are you sure you want to change the base?
Conversation
# Do not swallow characters that have been entered via AltGr: | ||
# Windows internally converts AltGr to CTRL+ALT, see | ||
# https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-vkkeyscanw | ||
if not key_event.dwControlKeyState & CTRL_ACTIVE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue for me. Control-←
and Control-→
word-skipping keybindings and Alt-d
to kill-word
or Alt-Backspace
to backward-kill-word
mentioned in #128389 are still working.
if block: | ||
continue | ||
|
||
return None | ||
elif self.__vt_support: | ||
# If virtual terminal is enabled, scanning VT sequences | ||
self.event_queue.push(rec.Event.KeyEvent.uChar.UnicodeChar) | ||
self.event_queue.push(raw_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A missed opportunity when main was merged during development of #124119.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which @sergey-miryanov takes care of here as part of #131901.
So maybe I should remove this one, but I'd really like to keep the other two raw_key
changes (not only because I'd have to adapt almost all tests :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO you should merge my branch here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can undo the change - then you won't have conflicts. I don't think we should (partially) merge between our two PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you are right - we shouldn't merge our PRs. You can keep your changes - I'm OK if here will be conflict.
return Event(evt="key", data="\033") # keymap.py uses this for meta | ||
return Event(evt="key", data=key, raw=key) | ||
return Event(evt="key", data=key, raw=raw_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the diff , previously
return Event(
evt="key", data=code, raw=rec.Event.KeyEvent.uChar.UnicodeChar
)
was used, which in the new code should have been raw_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was wondering why this change did not break anything, but AFAICT the raw
member is not used in the whole code base except
cpython/Lib/_pyrepl/unix_console.py
Line 513 in 5d8e432
def getpending(self): |
where the two
getpending
implementations dutifully respect e.raw += e.raw
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When searching in the code base for getpending
I found
cpython/Lib/_pyrepl/windows_console.py
Lines 518 to 521 in 5d8e432
def getpending(self) -> Event: | |
"""Return the characters that have been typed but not yet | |
processed.""" | |
return Event("key", "", b"") |
which clearly seems to be a bug to me? Because even though WindowsConsole._read_input()
only reads one Windows INPUT_RECORD
per call, get_event
could have put something into self.event_queue
.
I've addressed this in https://github.com/python/cpython/pull/132889/files#r2059235071.
@@ -459,22 +459,26 @@ def get_event(self, block: bool = True) -> Event | None: | |||
key = f"ctrl {key}" | |||
elif key_event.dwControlKeyState & ALT_ACTIVE: | |||
# queue the key, return the meta command | |||
self.event_queue.insert(Event(evt="key", data=key, raw=key)) | |||
self.event_queue.insert(Event(evt="key", data=key, raw=raw_key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about this one, but raw_key
seems plausible?
@paulie4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that key or raw_key is OK here. key is a str here, raw_key is unicode symbol (maybe with two or more code points) - both are not a byte
that expected. Maybe we should encode raw_key
here? But I don't know how to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these code paths return a string instead of bytes either way. But as written in #132440 (comment), Event.raw
is not used anywhere.
I fully agree that it should be bytes:
cpython/Lib/_pyrepl/console.py
Lines 40 to 44 in 5d8e432
@dataclass | |
class Event: | |
evt: str | |
data: str | |
raw: bytes = b"" |
It is almost everywhere used incorrectly in
windows_console.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, raw_key
is always "\x00"
in the mentioned code block
elif key == "\x00":
# Handle special keys like arrow keys and translate them into the appropriate command
key = VK_MAP.get(key_event.wVirtualKeyCode)
if key:
if key_event.dwControlKeyState & CTRL_ACTIVE:
key = f"ctrl {key}"
elif key_event.dwControlKeyState & ALT_ACTIVE:
# queue the key, return the meta command
self.event_queue.insert(Event(evt="key", data=key, raw=raw_key))
return Event(evt="key", data="\033") # keymap.py uses this for meta
return Event(evt="key", data=key, raw=raw_key)
because
raw_key = key = key_event.uChar.UnicodeChar
a few lines above. I tend to believe that Event.raw
is / was a debug help, since it serves (to me) no obvious purpose ...
Adding @vstinner, @eendebakpt and @paulie4 , since they were involved in #128389 and already talked about AltGr. |
PS: switching to an english keyboard layout, I can use the AltGr key like right-Alt, since it is not used in this keyboard layout :) |
The failing of Ubuntu (free-threading) is definitely unrelated, since this is a Windows specific change. |
Do you think it'd be possible to add some tests for this change to ensure we don't accidentally regress again? |
Yeah, I've already thought about it, but am still unsure how to best do it. ATM, I am thinking of mocking cpython/Lib/_pyrepl/windows_console.py Line 411 in b87189d
to be able to test cpython/Lib/_pyrepl/windows_console.py Lines 426 to 432 in b87189d
AFAICT, all the existing tests just mock WDYT? |
Agreed that we shouldn't be mocking |
self.assertEqual(reader.cxy, (2, 3)) | ||
con.restore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this fix I get a garbaged terminal after running the tests.
con.restore() | ||
|
||
|
||
class WindowsConsoleGetEventTests(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've printed the INPUT_RECORD
s I get for
- a lot of keys (or combination thereof)
- and for different keyboard layouts
- with virtual terminal support disabled or enabled (by running either in a legacy console or a Windows terminal)
self.assertEqual(self.get_event([None]), None) | ||
self.assertEqual(self.mock.call_count, 1) | ||
|
||
def test_WINDOW_BUFFER_SIZE_EVENT(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not PEP 8 compliant, but IMHO here it makes more sense to use things like WINDOW_BUFFER_SIZE_EVENT
or LEFT_ALT_PRESSED
.
E.g. using lower/upper case m
/ M
instead m_key_lower
etc.
Especially, test_m_LEFT_ALT_PRESSED_and_LEFT_CTRL_PRESSED
would be much harder to write to not be misleading, e.g. test_m_key_lower_left_alt_pressed_and_left_ctr_pressed
. Or test_left_LEFT_CTRL_PRESSED
.
@sergey-miryanov since you are working on REPL issues on Windows, too: can you have a look at my PR? |
|
Also on your branch if I put altgr + m - it inputs nothing. |
Can you also verify, that for What is your keyboard layout?
What does AltGr + m do in your editor? It should do the same in the REPL. If your keyboard layout does not feature anything on AltGr + m, then it is normal, that nothing happens. See https://en.wikipedia.org/wiki/AltGr_key which keyboards do what. I've tested e.g. a French layout, too. For me, everything works well in the legacy terminal. That's your PR that fixes the VT for me 🚀 |
@sergey-miryanov: and many thanks for testing ❤️ |
Merge with main to see whether that solves the GHA hiccup. [Edit] - it did fix GHA. |
Oh, I am running out of ideas now. I am on Windows 10, but can't believe this is the culprit. Seems that not a sinlge thing that is working for me is working for you. After all, my PR should not induce any change in behaviour, except
Since the elif key == "\x00":
# Handle special keys like arrow keys and translate them into the appropriate command
key = VK_MAP.get(key_event.wVirtualKeyCode) (admittetly except for the What happens if you omit my fix? Maybe other Windows users can test, too, so we have better coverage, in case this depends on more things we do not yet know. |
Maybe I misinformed you.
With DEU German (Germany) layout in ps with vt=True I have: I hope this is helps you. Let me know if you need to test something else. |
Oh, that helps a lot - because you can reproduce my fix in legacy terminals. As said, my PR cannot help with VT mode quirks (and is not intended to fix it), that's why we need your #131901 - which looks promising, too, but we shouldn't mix them. |
VK_OEM_7 = 0xDE | ||
|
||
# State of control keys: https://learn.microsoft.com/en-us/windows/console/key-event-record-str | ||
RIGHT_ALT_PRESSED = 0x0001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
windows_console.py has ALT_ACTIVE as bitwise or of 0x01 and 0x02 - I believe it is better to move these constants to file and use them. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've placed it there in the beginning and then later removed it to keep the diff in windows_console.py
small.
I'd happily move it if other reviewers prefer that, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise for CTRL_ACTIVE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Misc/NEWS.d/next/Library/2025-04-12-16-29-42.gh-issue-132439.3twrU6.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
E.g. on my keyboard with German layout,
{
is usually entered via pressing the AltGr key and7
, i.e.AltGr+7
.Likewise,
}
,[
,]
,\
and some more can only be entered viaAltGr
.But since #128388 / #128389 these are swallowed by the REPL on Windows and can no longer be entered.
This happens in legacy Windows terminals, where the virtual terminal mode is turned off (e.g.
cmd.exe
).In virtual terminal mode there are other issues, see #131878.