Skip to content

GH-132439: Fix REPL swallowing characters entered with AltGr on cmd.exe #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

Merged
merged 13 commits into from
May 5, 2025

Conversation

chris-eibl
Copy link
Member

@chris-eibl chris-eibl commented Apr 12, 2025

E.g. on my keyboard with German layout, { is usually entered via pressing the AltGr key and 7, i.e. AltGr+7.

Likewise, }, [, ], \ and some more can only be entered via AltGr.

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.

@chris-eibl chris-eibl added OS-windows 3.13 bugs and security fixes 3.14 bugs and security fixes topic-repl Related to the interactive shell labels Apr 12, 2025
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)
Copy link
Member Author

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.

Copy link
Member Author

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 :)

Copy link
Contributor

@sergey-miryanov sergey-miryanov Apr 20, 2025

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 :)

Copy link
Member Author

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?

Copy link
Contributor

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)
Copy link
Member Author

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?

Copy link
Member Author

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

def getpending(self):

where the two getpending implementations dutifully respect e.raw += e.raw :)

Copy link
Member Author

@chris-eibl chris-eibl Apr 20, 2025

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

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.

@chris-eibl chris-eibl requested a review from vstinner April 12, 2025 14:49
@chris-eibl
Copy link
Member Author

Adding @vstinner, @eendebakpt and @paulie4 , since they were involved in #128389 and already talked about AltGr.

@chris-eibl
Copy link
Member Author

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 :)

@picnixz picnixz added needs backport to 3.13 bugs and security fixes and removed 3.13 bugs and security fixes 3.14 bugs and security fixes labels Apr 12, 2025
@chris-eibl
Copy link
Member Author

The failing of Ubuntu (free-threading) is definitely unrelated, since this is a Windows specific change.

@tomasr8
Copy link
Member

tomasr8 commented Apr 17, 2025

Do you think it'd be possible to add some tests for this change to ensure we don't accidentally regress again?

@chris-eibl
Copy link
Member Author

Yeah, I've already thought about it, but am still unsure how to best do it.

ATM, I am thinking of mocking

def _read_input(self, block: bool = True) -> INPUT_RECORD | None:

to be able to test
def get_event(self, block: bool = True) -> Event | None:
"""Return an Event instance. Returns None if |block| is false
and there is no event pending, otherwise waits for the
completion of an event."""
while self.event_queue.empty():
rec = self._read_input(block)

AFAICT, all the existing tests just mock get_event, but for this, IMHO get_event itself must be tested.
Mocking _read_input seems to be the best way forward.

WDYT?

@tomasr8
Copy link
Member

tomasr8 commented Apr 17, 2025

Agreed that we shouldn't be mocking get_event since that's where we're making changes. _read_input seems like a good option, we can capture some real inputs and then replay them with it.

@ambv ambv changed the title GH-132439: REPL on Windows swallows characters entered via AltGr GH-132439: Fix REPL swallowing characters entered with AltGr May 5, 2025
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work again! Very well thought through.

@ambv ambv merged commit 07f416a into python:main May 5, 2025
52 checks passed
@miss-islington-app
Copy link

Thanks @chris-eibl for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @chris-eibl and @ambv, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 07f416a3f063db6b91b8b99ff61a51b64b0503f1 3.13

@ambv ambv changed the title GH-132439: Fix REPL swallowing characters entered with AltGr GH-132439: Fix REPL swallowing characters entered with AltGr on cmd.exe May 5, 2025
@sergey-miryanov
Copy link
Contributor

Will repeat here too - problem with backport because VT support was not backported #130805 (comment) (for completeness)

@ambv
Copy link
Contributor

ambv commented May 5, 2025

Yeah, I am backporting the VT support too because it's the only way we can have a sensibly similar codebase for bug fixes.

ambv pushed a commit to ambv/cpython that referenced this pull request May 5, 2025
…ltGr on cmd.exe (pythonGH-132440)

(cherry picked from commit 07f416a)

Co-authored-by: Chris Eibl <[email protected]>
Co-authored-by: Stan Ulbrych <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented May 5, 2025

GH-133460 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 5, 2025
ambv added a commit that referenced this pull request May 5, 2025
…n cmd.exe (GH-132440) (GH-133460)

(cherry picked from commit 07f416a)

Co-authored-by: Chris Eibl <[email protected]>
Co-authored-by: Stan Ulbrych <[email protected]>
@chris-eibl chris-eibl deleted the fix_altgr branch May 5, 2025 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows topic-repl Related to the interactive shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants