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

Jump to bottom

IDLE : Bug in keybinding validity check #65718

Closed
SaimadhavHeblikar mannequin opened this issue May 18, 2014 · 11 comments
Closed

IDLE : Bug in keybinding validity check #65718

SaimadhavHeblikar mannequin opened this issue May 18, 2014 · 11 comments
Assignees
@terryjreedy
Labels
3.7 (EOL) end of life topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@SaimadhavHeblikar
Copy link
Mannequin

SaimadhavHeblikar mannequin commented May 18, 2014

BPO 21519
Nosy @terryjreedy
PRs
  • bpo-21519: IDLE basic custom key entry detects duplicates. #2428
  • [3.6] bpo-21519: IDLE basic custom key entry better detects duplicate… #2433
    		•
    Files
  • keybinding.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2017-06-27.06:33:21.861>
created_at = <Date 2014-05-18.05:54:15.813>
labels = ['expert-IDLE', 'type-bug', '3.7']
title = 'IDLE : Bug in keybinding validity check'
updated_at = <Date 2017-06-27.06:33:21.860>
user = 'https://bugs.python.org/SaimadhavHeblikar'

    bugs.python.org fields:

    activity = <Date 2017-06-27.06:33:21.860>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2017-06-27.06:33:21.861>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2014-05-18.05:54:15.813>
creator = 'Saimadhav.Heblikar'
dependencies = []
files = ['35275']
hgrepos = []
issue_num = 21519
keywords = ['patch']
message_count = 11.0
messages = ['218734', '218777', '220625', '228300', '296481', '296562', '296650', '296796', '297005', '297011', '297012']
nosy_count = 2.0
nosy_names = ['terry.reedy', 'Saimadhav.Heblikar']
pr_nums = ['2428', '2433']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21519'
versions = ['Python 3.6', 'Python 3.7']
    @SaimadhavHeblikar
    Copy link
    Mannequin Author

    SaimadhavHeblikar mannequin commented May 18, 2014

    Steps to reproduce the bug:

    1. IDLE > options > configure idle > keys

    2. Try to replace a keybinding for an action with that of another action which has more than one keybinding.
      For eg : Default binding of copy=<Control-Key-c> <Control-Key-C>.
      So, try to replace any other keybinding with <Control-Key-c>

    3. This change is accepted

    Now the <Control-Key-C> binding will be assigned to two actions.

    This bug only applies if the other action has more than one binding.
    In case the other action has only one binding, an error is raised(as expected).

    Attaching a patch to fix this issue.

    @SaimadhavHeblikar SaimadhavHeblikar mannequin added topic-IDLE type-bug An unexpected behavior, bug, or error labels May 18, 2014
    @terryjreedy
    Copy link
    Member

    Now the <Control-Key-C> binding will be assigned to two actions.

    I do not see this. I only see <Control-Key-c> duplicated, which is bad enough to be patched. What did you leave out ;-).

    Also see bpo-12387, which we need to finish, and Ned's link to http://wiki.tcl.tk/28331

    A patch for KeysOK should include test_bindings.py with class KeysOK_Test. As with the test of name_ok in test_configname.py (sp?), the method should be embedded in a dummy class and use a dummy message box.

    The problem with the method goes deeper. It is not called if the 'advanced' (hand entry, therefore primitive) pane is used. As near as I can tell (and if I am wrong, please tell me), multiple bindings, which are required to ignore lower-upper case issues, can only be entered with this 'advanced' method. Hence, most new entries are not checked at all. The basic pane, which is advanced in that one can only enter possibly valid combination, should be modified to allow multiple entries. (The two panes should be called 'Easy' and 'Error-prone ;-)

    As it is, 'keys' is a misnomer[note], "keys.strip()" is a no-op in that 'keys' is auto-generated, and "keySequence = keys.split()" is the same as "keySequence = [keys]"[note]. Many of the tests only test mistakes that can happen with hand-entry, which is not tested. I believe that the duplicate check is the only one needed (except possible for the modifier check, which I have not looked at enough to tell).

    [note] If the easy pane were modified to define multiple keys, and all were passed at once, then the two noted lines would become valid, but then each key, would have to be tested individually against the flat list.

    This suggests to me that the flattening should be done just once and that KeysOK should become KeyOK, or rather key_ok. It might even be replaced by "newkey in flat_list" or something nearly that simple.

    @SaimadhavHeblikar
    Copy link
    Mannequin Author

    SaimadhavHeblikar mannequin commented Jun 15, 2014

    A small bug in line 185 in keybindingDialog.py. 'F2' appears twice and 'F3' is missing. Since this is a typo, I did not create a new issue.

    @terryjreedy
    Copy link
    Member

    bpo-6739 may also be relevant

    @terryjreedy
    Copy link
    Member

    bpo-6739 also has a patch to refuse invalid key bindings.

    @terryjreedy terryjreedy added the 3.7 (EOL) end of life label Jun 20, 2017
    @terryjreedy terryjreedy self-assigned this Jun 20, 2017
    @terryjreedy
    Copy link
    Member

    I believe patch affects same area of file as patch for bpo-6739.

    @terryjreedy
    Copy link
    Member

    bpo-6739 is about rejecting *invalid* sequences, this is about rejecting a *duplicate* valid sequence. Both fixes are needed.

    @terryjreedy
    Copy link
    Member

    configdialog.ConfigDialog.getNewKeys() calls config_key.GetKeysDialog with a list of lists of one or more sequences (currentKeySequences). GetKeysDialog.KeysOK looks for keys.split() in currentKeySequences. Since KeysOK on only called for the no-space 'keys' produced by the basic dialog, keys.split is always [keys]. This can only match one of the length-1 lists in currentKeySequences.

    The patch semi-flattens the latter to a list of lists of 1 sequence each
    . But instead of looking for [keys] in the result, better and faster to omit the list wrappers and look for keys in a list or set of sequences.

    A separate improvement would be to have a reverse map of sequences to pseudoevents, so the error message can specify the exact conflict instead of just saying 'some conflict'. I have separately thought that the [keys] tab of ConfigDialog should be able to display such a mapping to help one plan a new key mapping.

    Off topic for this issue: the keys tab could have a 'load key-def file' function that would check a definition and if ok, load it.

    @terryjreedy
    Copy link
    Member

    New changeset 44913e5 by terryjreedy in branch 'master':
    bpo-21519: IDLE basic custom key entry better detects duplicates. (bpo-2428)
    44913e5

    @terryjreedy
    Copy link
    Member

    New changeset 93b88e9 by terryjreedy in branch '3.6':
    [3.6] bpo-21519: IDLE basic custom key entry better detects duplicates. (GH-2428) (bpo-2433)
    93b88e9

    @terryjreedy
    Copy link
    Member

    Thanks for the patch. The F3-missing bug has been fixed.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @terryjreedy terryjreedy mentioned this issue Dec 4, 2023
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @terryjreedy terryjreedy

    Labels
    3.7 (EOL) end of life topic-IDLE type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @terryjreedy