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-128388: pyrepl on Windows: add meta and ctrl+arrow keybindings #128389

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

Conversation

paulie4
Copy link

@paulie4 paulie4 commented Jan 1, 2025

Fix Lib/_pyrepl/windows_console.py to support more keybindings, like the Ctrl+ and Ctrl+ word-skipping keybindings and those with meta (i.e. Alt), e.g. to kill-word or backward-kill-word.

Specifics: if Ctrl is pressed, emit "ctrl left" and "ctrl right" instead of just "left" or "right," and if Meta/Alt is pressed, emit the special key code for meta before emitting the other key that was pressed.

NOTE: this is my first PR for https://github.com/python/cpython, so please tell me if I need to do something else, e.g. does this need an entry added somewhere under Misc/NEWS.d?

Copy link

cpython-cla-bot bot commented Jan 1, 2025

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Jan 1, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Jan 1, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@hugovk hugovk added OS-windows topic-repl Related to the interactive shell labels Jan 1, 2025
@hugovk
Copy link
Member

hugovk commented Jan 1, 2025

NOTE: this is my first PR for https://github.com/python/cpython, so please tell me if I need to do something else, e.g. does this need an entry added somewhere under Misc/NEWS.d?

Yes please, and also sign the CLA.

@paulie4
Copy link
Author

paulie4 commented Jan 1, 2025

@hugovk, Misc/NEWS.d is a directory, so I don't know which file(s) in there to edit, especially since it looks like the versions in there have all been released. Do I need to create a new file (or files if it's going to both 3.13.2 and to a 3.14.0a)?

UPDATE: it looks like these are the instructions for which file I should create: https://devguide.python.org/core-developers/committing/#how-to-add-a-news-entry. I think _pyrepr is part of the standard library, right?

I tried signing the CLA, but the SIGN IN WITH GITHUB TO AGREE button doesn't work with the @users.noreply.github.com email address I use for my commits. I submitted a manual CLA, so hopefully that will work. FYI, this is the error I get when I try to use https://cpython-clabot.herokuapp.com/contributor-license-agreement?state=...:

Thank you for authorizing our application, but the CLA must be signed by the users who contributed to the PR. Authors emails are: [email protected].

@hugovk
Copy link
Member

hugovk commented Jan 1, 2025

UPDATE: it looks like these are the instructions for which file I should create: https://devguide.python.org/core-developers/committing/#how-to-add-a-news-entry. I think _pyrepr is part of the standard library, right?

Yes, that's right, the Lib dir is the stdlib.

@paulie4
Copy link
Author

paulie4 commented Jan 2, 2025

@hugovk, thank you for that suggested doc change, which I approved. Is there anything else I need to do, or do we now just need to wait for someone else (e.g. @pablogsal, @lysnikolaou, or @ambv) to review the code changes?

@hugovk
Copy link
Member

hugovk commented Jan 3, 2025

Thanks for the updates. Yes, someone with Windows will need to review, hopefully it shouldn't be too long :)

Copy link
Contributor

@eendebakpt eendebakpt left a comment

Choose a reason for hiding this comment

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

I left some comments for improvement of the code, but the PR is good: I tested with Windows 11 and the meta keybindings work now

Lib/_pyrepl/windows_console.py Show resolved Hide resolved
Lib/_pyrepl/windows_console.py Outdated Show resolved Hide resolved
Lib/_pyrepl/windows_console.py Show resolved Hide resolved
Lib/_pyrepl/windows_console.py Outdated Show resolved Hide resolved
return Event(
evt="key", data=code, raw=rec.Event.KeyEvent.uChar.UnicodeChar
)
if code in ("left", "right") and (ctrlstate := key_event.dwControlKeyState) and ctrlstate & CTRL_OR_ALT_ACTIVE:
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes Alt+ and Ctrl-Alt+ work just as Ctrl+. Is that how it should be?

On Unix for me Alt+ gives me capital C (not sure why, does not really make sense to me) and Ctrl-Alt+ triggers my window manager.

In the keymap I also don't see Alt-left with a special meaning:

(r"\<up>", "up"),
(r"\<down>", "down"),
(r"\<left>", "left"),
(r"\C-\<left>", "backward-word"),
(r"\<right>", "right"),
(r"\C-\<right>", "forward-word"),
(r"\<delete>", "delete"),
(r"\x1b[3~", "delete"),
(r"\<backspace>", "backspace"),
(r"\M-\<backspace>", "backward-kill-word"),
(r"\<end>", "end-of-line"), # was 'end'
(r"\<home>", "beginning-of-line"), # was 'home'
(r"\<f1>", "help"),
(r"\<f2>", "show-history"),
(r"\<f3>", "paste-mode"),
(r"\EOF", "end"), # the entries in the terminfo database for xterms

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, I didn't see anything existing that was useful for Alt+, so I figured might as well make it useful. I can remove it if you prefer only Ctrl+ being useful. Also, it would definitely violate separation of concerns if this Lib/_pyrepl/windows_console.py file were to try to read the keymappings in Lib/_pyrepl/reader.py to check if r"\M-\<left>" existed, and I don't expect that to be added anytime soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer all implementations (unix, windows, etc.) to have the same implementation. Please revert the changes to ALT. There might be good use for the ALT modifier, but that would be for another PR.

Copy link
Author

Choose a reason for hiding this comment

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

That brings up a good question, why don't we add (r"\M-\<left>", "backward-word") and (r"\M-\<right>", "forward-word") to the default_keymap, so that way all implementations will have something useful for Alt+?

Also, I just tested that, and it made me realize that the Lib\_pyrepl\windows_console.py code also needs to # queue the key, return the meta command in the special key == "\x00" section (see 46b22d1 for the fix and removal of treating Alt+ as Ctrl+).

)
key = VK_MAP.get(key_event.wVirtualKeyCode)
if key:
if key in ("left", "right") and key_event.dwControlKeyState & CTRL_ACTIVE:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if key in ("left", "right") and key_event.dwControlKeyState & CTRL_ACTIVE:
if key_event.dwControlKeyState & CTRL_ACTIVE:

I tried this locally and it works. For example for arrow up the ctrl up is passed on as the key. Currently it is ignored, but maybe it is better to just pass whatever we get and let some platform independent part of the code decide what to do with it?

Comment on lines +431 to +432
self.event_queue.insert(0, Event(evt="key", data=key, raw=key))
return Event(evt="key", data="\033") # keymap.py uses this for meta
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the answer. But why pass the ctrl modifier as a key f'ctrl {key}`, and the alt modifier using a special event?

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

Successfully merging this pull request may close these issues.

3 participants