Repository navigation
gh-55646: Do not crash IDLE on an invalid key binding - #152747
terryjreedy merged 5 commits into
Conversation
Sorry, something went wrong.
A typo in a key binding, such as <Alt-Key-up> for <Alt-Key-Up>, crashed IDLE at startup. It is now ignored with a warning.
On macOS an invalid binding such as <Alt-Key-up> is not parsed into a MultiCall triplet, so it falls back to Tk's event_add(), which was left unguarded and still crashed. Guard the fallback too, and test both the parsed (bind()) and unparsed (event_add()) paths on every platform.
terryjreedy
left a comment
There was a problem hiding this comment.
The issue is an example of making the good the enemy of the adequate. Unexpected crashes/exits are bad and this is definately better than no fix.
The merge conflict was due to this being the 2nd patch to add tests at the end of test_multicall. The local interface had a button to accept both additions.
I also looked at Serwy's patch: https://bugs.python.org/file24351/issue11437.patch.
His concern about repeated processing is mostly moot for most users. We merged all the IDLE feature extensions into the main code about a decade ago, leaving only a test extension.
Main question: Why catch errors in multicall rather than configparser? Does every sequence get run through multicall? Does this catch more errors?
Sorry, something went wrong.
|
Note that "Accept both" eats a blank line between fragments. You have to add it after pressing "Accept both" while you are in the online editor. I fixed this. Yes, every configured key binding goes through MultiCall. MultiCall is the single choke point where a binding string is actually handed to Tk. Serwy's patch also works. But it pulls a Tk root plus a modal dialog into config code that also runs headless (tests, the subprocess). Naming the bad key and suggesting a reconfigure is a nice touch, which I've added to the warning. |
Sorry, something went wrong.
idlelib is private, so its helpers need no leading underscore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@terryjreedy, does my answer above address your question? Every configured key binding does go through MultiCall, so it is the single place where an invalid sequence can be caught, and it also catches sequences that the config code cannot check without a Tk root. The same kind of problem exists for other config values. An invalid color in a user theme crashes the Settings dialog (#85604). A config file that cannot be parsed at all (a syntax or encoding error) was handled in #152764. Fonts are already tolerant: a bad size falls back to the default with a warning, and Tk substitutes an unknown family. |
Sorry, something went wrong.
|
Thanks @serhiy-storchaka for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
Sorry, something went wrong.
|
GH-158149 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
Sorry, @serhiy-storchaka and @terryjreedy, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker b9546455ab0f65af65b0b69e86e7a2f65d287c16 3.13 |
Sorry, something went wrong.
|
GH-158150 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
Serhiy: Conflict stopped 3.13 backport. |
Sorry, something went wrong.
…) (#158150) gh-55646: Do not crash IDLE on an invalid key binding (GH-152747) A typo in a key binding, such as <Alt-Key-up> for <Alt-Key-Up>, crashed IDLE at startup. It is now ignored with a warning. On macOS an invalid binding such as <Alt-Key-up> is not parsed into a MultiCall triplet, so it falls back to Tk's event_add(), which was left unguarded and still crashed. Guard the fallback too, and test both the parsed (bind()) and unparsed (event_add()) paths on every platform. --------- (cherry picked from commit b954645) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
GH-158265 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…) (#158265) A typo in a key binding, such as <Alt-Key-up> for <Alt-Key-Up>, crashed IDLE at startup. It is now ignored with a warning. On macOS an invalid binding such as <Alt-Key-up> is not parsed into a MultiCall triplet, so it falls back to Tk's event_add(), which was left unguarded and still crashed. Guard the fallback too, and test both the parsed (bind()) and unparsed (event_add()) paths on every platform. --------- (cherry picked from commit b954645) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
A typo in a custom key binding entered via Advanced Key Binding (which is not validated), such as <Alt-Key-up> instead of <Alt-Key-Up>, crashed IDLE at startup:
File ".../idlelib/multicall.py", line ..., in bind self.__binders[triplet[1]].bind(triplet, func) ... _tkinter.TclError: bad event type or keysym "up"MultiCall.event_add stores the parsed sequence without validating the keysym, so the TclError is raised later, when a virtual event is bound and MultiCall performs the actual Tk bind. With no window yet, IDLE just exits.
Catch TclError at both binder call sites (MultiCall.bind and MultiCall.event_add) and ignore the invalid binding with a warning naming it. The bind path also drops the bad sequence from the virtual event, which avoids a secondary ValueError if it is later rebound or unbound.
🤖 Generated with Claude Code