bpo-6739: IDLE: Check for valid keybinding in config_keys#2377
bpo-6739: IDLE: Check for valid keybinding in config_keys#2377terryjreedy merged 5 commits intopython:masterpython/cpython:masterfrom
Conversation
terryjreedy
left a comment
There was a problem hiding this comment.
I will submit a patch that addresses all my comments.
Lib/idlelib/config_key.py
Outdated
| key.strip() | ||
| if not key: | ||
| tkMessageBox.showerror(title=self.keyerror_title, parent=self, | ||
| message="No keys specified.") |
There was a problem hiding this comment.
I changed line 6 way above from
import tkinter.messagebox as tkMessageBox # to
from tkinter.messagebox import showerror
and deleted tkMessageBox. everywhere.
The same deletion will be needed in any other existing patches.
There was a problem hiding this comment.
I wasn't sure when you would want the tkMessageBox and tkFont to be updated when changes are made. I can make a PR for just that change across all source or change it as we come across it, like this. I saw it on the roadmap and just wasn't sure how you wanted to handle it.
There was a problem hiding this comment.
Three possible updates:
from tkinter import messagebox
from tkinter.messagebox import showerror (for instance)
<replace showerror with custom function, #30751
Lib/idlelib/config_key.py
Outdated
| if self.advanced or self.KeysOK(): # doesn't check advanced string yet | ||
| self.result=self.keyString.get() | ||
| key = self.keyString.get() | ||
| key.strip() |
There was a problem hiding this comment.
This is a bug. See why? I noticed while writing a test for showerror() below and the new test fails without the fix.
There was a problem hiding this comment.
key.strip() isn't inplace, so without assignment, it's not doing anything?
If that's the bug, it existed before, but wouldn't have been reached because the only way to enter whitespace would have been on the 'advanced' tab and this check was in KeysOK, which was never run when the advanced flag was set.
Thank you for giving me time to look at this and figure it out.
There was a problem hiding this comment.
The check for missing '>' is odd because that also can only happen with 'advanced' entry. So either these are a holdover from when 'advanced' entries were run through this function, or they constitute a check on the components to string code that belongs in the test module. There is still improvement possible.
| dia.Cancel() | ||
|
|
||
|
|
||
| class SequenceOKTest(unittest.TestCase): |
There was a problem hiding this comment.
I merged this into GetKeysTest and am adding more tests.
Lib/idlelib/config_key.py
Outdated
| self.unbind(keys, binding) | ||
| accepted = True | ||
|
|
||
| return accepted |
There was a problem hiding this comment.
I simplified the function code a bit.
| dia = config_key.GetKeysDialog( | ||
| self.root, 'test', '<<Test>>', ['<Key-F12>'], _utest=True) | ||
| dia.Cancel() | ||
|
|
There was a problem hiding this comment.
I am adding a few tests to try to cover the code changed.
There was a problem hiding this comment.
I was thinking of adding more tests and looks like I should have because of the bug. Sorry that the PR wasn't complete.
|
I didn't see your commit until now. Your simplification of the new function was what I was thinking -- I guess the old discussion of one return vs many. And I didn't know about this -> |
|
With the patch, the function is a bit odd in the argument is replaced by modifiers and key for some of the checks. More revision is needed in a further issue. |
…onGH-2377) Verify user-entered key sequences by trying to bind them with tk. Add tests for all 3 validation functions. Original patch by G Polo. Tests added by Cheryl Sabella. (cherry picked from commit 8c78aa7)
Based on patch by Guilherme Polo.