bpo-33610: validate non-negative integer inputs in IDLE's config #14822
Conversation
This forces the entry widgets to always only contain digits or be empty.
There was a problem hiding this comment.
Without the patch, I entered '15aa' for max context lines. When I closed and reopened config dialog, it only showed 15. 'OK, no damage done.' When I closed and restarted IDLE,
" Warning: config.py - IdleConf.GetOption -
invalid 'int' value for configuration option 'maxlines'
from section 'CodeContext': '15aa'
So I highly approve of adding something to prevent this in the next week (but see name comment).
It would be nice to check that an entry is in an allowed range, with an error message. I suspect that 0 for some entries could disable IDLE or at least have negative effects. But I have not tested my suggestion, limits would need some discussion, and it might be a good idea to put them in a global dict.
I mentioned not restricting validation to the General tab because I want to replace the indent slider with an Entry box. I would like to eventually move this to the first section of the General tab, but perhaps only when splitting that tab into two.
Lib/idlelib/configdialog.py
Outdated
| def is_digits_or_empty(s): | ||
| "Return 's is blank or contains only digits'" | ||
| return digits_or_empty_re.fullmatch(s) is not None | ||
| self.digits_or_empty_vldcmd = ( |
There was a problem hiding this comment.
I would strongly prefer a simpler name, such as 'digits_only', or 'check_digits', or 'non_negative'.
|
The trouble with range checks is that we shouldn't run them on every change, rejecting invalid changes, as we do with the "empty or digits only" check. Not allowing zero would be fine, because there's never a good reason to write a number with leading zeros in such a config dialog. But anything beyond that would make editing the value a hassle. For example, if I wanted to change the default window width from 80 to 100, I couldn't delete the "80" and type "100" nor could I delete the "8" and add "10". I would have to do something like type "100" after the "80", resulting in "80100", and then delete the "80". Assuming we agree that that is horrible UX, we'd need to validate the value on focus out and/or hitting the "apply" or "ok" buttons, and somehow inform the user which value(s) they've set are invalid and why. That's orders of magnitude more complex than what's needed for my suggestion above. I suggest staying with the current method, making a few entries non-zero, and leaving the range checks for a separate issue. |
|
You are right. A range validation should wait for a later event than key press in the widget. That includes checking for initial 0, as changing 80 to 0 to 90 would temporarily have a leading 0. (I guess this is why I put off range checks for 'later'.) So keep the current new check. |
|
Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
…honGH-14822) (cherry picked from commit 1ebee37) Co-authored-by: Tal Einat <taleinat@gmail.com>
|
GH-14913 is a backport of this pull request to the 3.8 branch. |
…honGH-14822) (cherry picked from commit 1ebee37) Co-authored-by: Tal Einat <taleinat@gmail.com>
|
GH-14914 is a backport of this pull request to the 3.7 branch. |
This forces the entry widgets to always only contain digits or be empty.
https://bugs.python.org/issue33610