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

gh-127947: Repeat PyREPL key events on Windows when wRepeatCount > 1 #127948

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SimonvBez
Copy link

@SimonvBez SimonvBez commented Dec 14, 2024

Keys that are repeated into PyREPL were typed only once on Windows.
This has been consistently happening when typing a dead key repeatedly, such as when double-pressing the quote character on the US International keyboard layout.
This change makes those characters be typed the correct number of times.

Keys that are repeated into PyREPL were typed only once. This change makes those characters be typed the correct number of times.
Copy link

cpython-cla-bot bot commented Dec 14, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app

This comment was marked as resolved.

@bedevere-app

This comment was marked as resolved.

@bedevere-app

This comment was marked as resolved.

@ZeroIntensity ZeroIntensity added topic-repl Related to the interactive shell needs backport to 3.13 bugs and security fixes labels Jan 10, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

It's good to see new contributors :)

Could you add a test? See the devguide for how to write tests.

@SimonvBez
Copy link
Author

There was no test coverage for the WindowsConsole.get_event method yet, because it was being mocked fully.
So I have added two tests that mock WindowsConsole._read_input (used within get_event) instead, to test if the INPUT_RECORD.Event.KeyEvent.wRepeatCount is indeed properly used.

@ZeroIntensity ZeroIntensity self-requested a review January 12, 2025 17:06
@ZeroIntensity
Copy link
Member

I'll review this soon-ish. Could you deal with the merge conflicts in the meantime?

@SimonvBez
Copy link
Author

I see #128389 has caused a merge conflict. The changes there are minimal, but it isn't marked as getting backported to 3.13.
I will resolve the conflict to the current main branch. Is there anything else I should do to make backporting my changes to 3.13 easier?

@ZeroIntensity
Copy link
Member

Nah, backporting is pretty easy even if the automation fails. Just deal with the conflicts on main and you're good.

@SimonvBez
Copy link
Author

SimonvBez commented Jan 12, 2025

The one thing I'm not sure about is the current distinction between WindowsConsole.event_queue and WindowsConsole.key_repeat_queue.
The event_queue already existed and was only used for a "scroll" event (until #128389 started using it too).
But using the event_queue will put a delay of 0.01s until it will get handled, because of the wait() function.
I did not want to change the existing behavior, so I opted to create the key_repeat_queue and made the wait() return immediately if there is anything in key_repeat_queue, so that repeated keys would appear immediately instead of with a 0.01s delay.
But looking at it right now it might be better to merge the queues into just event_queue, and make it also circumvent the wait() sleep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review needs backport to 3.13 bugs and security fixes OS-windows topic-repl Related to the interactive shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants