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

Modern Unix key bindings for IDLE #71360

Closed
serhiy-storchaka opened this issue Jun 1, 2016 · 27 comments
Closed

Modern Unix key bindings for IDLE #71360

serhiy-storchaka opened this issue Jun 1, 2016 · 27 comments
Assignees
Labels
topic-IDLE type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 27173
Nosy @terryjreedy, @ned-deily, @roseman, @serhiy-storchaka
Dependencies
  • bpo-27245: IDLE: Fix deletion of custom themes and key bindings
  • bpo-27437: IDLE tests must be able to set user configuration values.
  • Files
  • idle_modern_unix_key_set.patch
  • idle_modern_unix_key_set2.patch
  • idle_modern_unix_key_set3.patch
  • idle_modern_unix_key_set4.patch
  • idle_modern_unix_key_set5.patch
  • modern-unix6.diff: refactored function and tests
  • 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 2016-08-14.22:35:09.578>
    created_at = <Date 2016-06-01.07:29:45.328>
    labels = ['expert-IDLE', 'type-feature']
    title = 'Modern Unix key bindings for IDLE'
    updated_at = <Date 2016-08-25.02:16:41.187>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-08-25.02:16:41.187>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2016-08-14.22:35:09.578>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2016-06-01.07:29:45.328>
    creator = 'serhiy.storchaka'
    dependencies = ['27245', '27437']
    files = ['43079', '43083', '43091', '43274', '43587', '43673']
    hgrepos = []
    issue_num = 27173
    keywords = ['patch']
    message_count = 27.0
    messages = ['266801', '266809', '266815', '266830', '266836', '267536', '267581', '267587', '267589', '267594', '267596', '267600', '268478', '268482', '269321', '269327', '269349', '269407', '269577', '269692', '269698', '270074', '270108', '270134', '272686', '272700', '273620']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'ned.deily', 'markroseman', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27173'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    The "IDLE Classic Unix" key set in IDLE provides Emacs-like key bindings. But modern Linux environments (KDE, Gnome, etc) use different key bindings in standard editors. Proposed patch provides new key set named "IDLE Modern Unix" that uses key bindings common in KDE (and mainly in Gnome). It is more habitual for average user.

    In additional the patch makes IDLE to use platform-depending key set by default.

    @serhiy-storchaka serhiy-storchaka added topic-IDLE type-feature A feature request or enhancement labels Jun 1, 2016
    @terryjreedy
    Copy link
    Member

    Seems reasonable.

    3 or more years ago, there was an issue about needing to define pairs like Control-Key-c and Control-Key-C in order to have crtl + c/C-key work the same regardless of the caps lock setting. I also remember that the effect of Caps lock was system dependent. But I forget the details. We should find the issue and document the system dependent rules, perhaps in config.py, perhaps in config-keys.def.

    Adding a new default key set will likely not be as simple as we wish. The problem is the same one we had when adding a new default color theme last fall. If one selects the new definition, older releases will try and fail to load what is not there. Keys have a different backup logic than themes, including the 'last resort fallback' in IdleConf.GetCoreKeys. With thems the result was errors and or an exit. Have you, or can you check what happens if you select 'Modern Unix' and run an existing release.

    If necessary, we can use the same workaround we did with theme, which is described in the docstring for IdleConf.CurrentTheme. Part the theme patch was making sure that all of the rest of IDLE accessed the current theme through the rewritten CurrentTheme instead of through GetOption('main', 'Theme', ...).

    I would like to add some unittests, but a manual test protocol is needed at least to test an existing release. Tests should be run on each of the three systems, and for each backported version. When we have a tested patch ready, we could ask New to test on Mac, or do you have access to one now?

    @serhiy-storchaka
    Copy link
    Member Author

    3 or more years ago, there was an issue about needing to define pairs like Control-Key-c and Control-Key-C in order to have crtl + c/C-key work the same regardless of the caps lock setting. I also remember that the effect of Caps lock was system dependent.

    Proposed scheme binds different actions on hotkeys with and without Shift: Ctrl-Z/Ctrl-Shift-Z, Ctrl-S/Ctrl-Shift-S. If don't touch CapsLock this is not an issue.

    Yes, there is a problem with new buildin keys set in old IDLE versions. A lot of warnings is emitted on stderr. The solution for the "IDLE Dark" theme is not complete, since after adding next new theme we will encounter the same problem and should add options "name3", "name4"...

    Updated patch uses the same solution for new keys set. But it also makes warnings be less annoying. Every warning is emitted just once, and only one warning is emitted for unknown keys set.

    @terryjreedy
    Copy link
    Member

    Original IDLE Dark patch: bpo-24820. Follow-up for cross-version issues (initially for themes, but with a eye to keys, etc): bpo-25313. I intended that 'name2' be future proof (msg252372). I believe my idea was that if it were to point to a future 4th default theme that did not exist, there would be a graceful fallback to 'name', which defaults to IDLE Classic. But I am not sure if I tested this, so I should recheck. If it does not work, a fix should go in all current versions. I believe the patch did achieve no warnings for older versions, as least on my machine.

    I would have the same goal for keys: no warning, future proof. A test of the latter before committing requires two independent repositories, both with the patch, one with the extra definitions. I currently have two 3.6 repositories and will do such a test once we have code that we think should work.

    When one selects IDLE Dark as Default Theme, active or not, a subdued red message is displayed: "New theme, see Help". (The latter part should change to a more explicit "click 'Help' below" so there is no possible confusion with the main menu Help. The Highlighting specific part of the popup suggests saving IDLE Dark as a custom theme, with a new name, This makes the theme available to older versions and solves compatibility problems. The same would apply to a new key set.

    In bpo-24781, Mark Roseman proposed revamping the preferences dialog, both externally and internally. The main idea for the Theme tab is to present users with a single list of available themes and (mostly) hide the builtin versus custom distinction. (It cannot be ignored completely as builtins cannot be deleted and editing requires saving as custom.) https://bugs.python.org/file40111/highlight3.png has an image. We might consider the same idea for key sets.

    @serhiy-storchaka
    Copy link
    Member Author

    New patch makes IDLE silently fall back to default key set and adds a note about new key set on Keys configuration tab.

    @serhiy-storchaka
    Copy link
    Member Author

    Ping.

    @terryjreedy
    Copy link
    Member

    I looked at _set3 and it looks plausible and possibly complete other than testing. I added something about testing customizations to bpo-27099. This applied cleanly to 3.6. 3.6 started, dialog opened, new key set selected, dialog closed, config-main.cfg looks good. Open dialog, same as custom, close dialog, config-keys and config main changed as expected. Open 3.5, open dialog, everything looks fine.

    Now the problems.

    1. To delete, one must select as current. After deletion, config tries to reread the keyset just deleted. This prints a warning to console for each item in the set, with message as to default used instead. The same happens with deleting custom themes. I may have noted this somewhere, but cannot find.

    In any case, the solution for both problems is to either allow deletion of non-selected sets, and only such, or to switch to *something* before re-reading. I open a new issue, bpo-27245, for this.

    2. More serious, the warnigs are followed by
    Exception in Tkinter callback
    Traceback (most recent call last):
      File "F:\Python\dev\35\lib\tkinter\__init__.py", line 1550, in __call__
        return self.func(*args)
      File "F:\Python\dev\35\lib\idlelib\configDialog.py", line 1182, in Ok
        self.Apply()
      File "F:\Python\dev\35\lib\idlelib\configDialog.py", line 1186, in Apply
        self.DeactivateCurrentConfig()
      File "F:\Python\dev\35\lib\idlelib\configDialog.py", line 1166, in DeactivateCurrentConfig
        instance.RemoveKeybindings()
      File "F:\Python\dev\35\lib\idlelib\EditorWindow.py", line 771, in RemoveKeybindings
        self.text.event_delete(event, *keylist)
      File "F:\Python\dev\35\lib\idlelib\MultiCall.py", line 391, in event_delete
        self.__binders[triplet[1]].unbind(triplet, func)
      File "F:\Python\dev\35\lib\idlelib\MultiCall.py", line 234, in unbind
        doit()
      File "F:\Python\dev\35\lib\idlelib\MultiCall.py", line 232, in <lambda>
        doit = lambda: self.bindedfuncs[triplet[2]][triplet[0]].remove(func)
    ValueError: list.remove(x): x not in list

    Hitting OK repeats warnings and traceback (the last line might have been a bit different the first time). Only Cancel would close.

    @serhiy-storchaka
    Copy link
    Member Author

    I'm aware of the bug with deleting current key set. idle_modern_unix_key_set3.patch effectively fixes it.

    @serhiy-storchaka
    Copy link
    Member Author

    After applying this patch I'm going to redesign Highlighting and Keys tabs in the Settings dialog.

    @terryjreedy
    Copy link
    Member

    I don't understand

    idle_modern_unix_key_set3.patch effectively fixes [the bug].
    That is the patch that generated the exception I posted.

    I look forward to contributions to improving the dialog. Redoing the font tab is bpo-24776. The highlight tab is bpo-24781. The latest mockups look like improvements to me, but we can discuss further. (bpo-24781 also has Mark's proposed refactoring of the dialog code, but I have not reviewed in detail.) There are numerous issues connected with key definitions.

    @serhiy-storchaka
    Copy link
    Member Author

    Interesting, I was not able to reproduce a bug if just save as custom theme and apply, but after creating and deleting the second custom theme I got a mystical behavior of IDLE -- radiobuttons are not consistent with comboboxes, OK can't be pressed, etc.

    @serhiy-storchaka
    Copy link
    Member Author

    There was a bug, saving custom key set didn't have any effect if previous key set was IDLE Modern Unix.

    @terryjreedy terryjreedy self-assigned this Jun 11, 2016
    @terryjreedy
    Copy link
    Member

    Ned, Serhiy's patch 4 solves one of your concerns in bpo-20580 with

    + @staticmethod
    + def DefaultKeys():
    + if sys.platform[:3] == 'win':
    + return 'IDLE Classic Windows'
    + elif sys.platform == 'darwin':
    + return 'IDLE Classic OSX'
    + else:
    + return 'IDLE Modern Unix'

    Please confirm that Classic OSX is the right choice over Classic Mac.

    @ned-deily
    Copy link
    Member

    "Please confirm that Classic OSX is the right choice over Classic Mac"

    It is. At least, when we install IDLE on OS X, Clsssic OS X is the default set. I don't know why Classic Mac exists; I'm guessing it is left over from Mac OS 9 days (pre OS X).

    @serhiy-storchaka
    Copy link
    Member Author

    Ping.

    @terryjreedy
    Copy link
    Member

    The change in v4 is the addition of
    self.AddChangedItem('main', 'Keys', 'name2', '')
    There is a bug in the get keys function. After applying, when I restart IDLE and go to 'Keys', my custom key set is 'Modern Unix' instead of 'Terry'. It is still 'Terry' on other, unpatched IDLEs. I shelved the patch, restarted, back to 'Terry'. Repeated unshelving and shelving restarts switched each time.

    @serhiy-storchaka
    Copy link
    Member Author

    I can't reproduce your issue (or understand it correctly). Could you please show the [Keys] section in your config-main.cfg?

    @terryjreedy
    Copy link
    Member

    I rebooted and reran build in case this was another funny heisenbug. It isn't. Before patch, start 3.6 repository IDLE. Click Options, Configure IDLE, and Keys tab. Look at 'Use a Custom Key Set'. It says 'Terry', which is correct.

    After patch: repeat. It says 'IDLE Modern Unix'. Custom Key Binding box reflects change. Repeat with 3.5 repository IDLE: it still says Terry. Ditto for installed 3.6.0a2.

    My custom [Keys] section:

    [Keys]
    default = False
    name2 = IDLE Modern Unix
    name = Terry

    The problem is that name2 should be treated only as default, never as custom, and should be used only if default=True. This is true in CurrentTheme. However, your revised CurrentKeys uses it first and unconditionally. Hence the observed behavior.

    CurrentKeys should use the same logic as CurrentTheme. In fact, the common logic should factored out into a separate function with two or three parameters and used by both.

    If, as you claim, CurrentTheme is buggy, it should be fixed before using it as the template for both. To test and claim that it was future proof, I did something like the following last fall. First set config-main.cfg [Keys] name2 to something non-existent, that might be added in the future.

    [Theme]
    default = True
    name2 = IDLE Future
    name = Custom Dark

    Then start, for instance, 3.5.2. The non-existent IDLE Future is ignored and replaced with the default default IDLE Classic, as intended in the code. No warnings. What exactly did you do to claim otherwise?

    ---
    Also wrong, even with your patch removed: '()Use a Built-in Key Set' says IDLE Classic Mac (slightly grayed) on all versions. If I select built-in, change built-in selection to IDLE Classic Windows, select custom again, click [OK] to close, and reopen, the unselected built-in has switched back to Mac. This must be a pre-existing bug that makes Classic Mac the default builtin when custom is selected, This is in spite of config-main.def having

    [Keys]
    default= 1
    name= IDLE Classic Windows

    This, of course, is also wrong on non-Windows, which is why you changed it. But it is also being ignored when custom is selected.

    The problem must be in how configdialog gets the default key set when the current keyset is custom. The widget is a DynOptionMenu, created in CreatePageKeys, line 335, with no content. The options are set in LoadKeyCfg, lines 1055 and 1069. The latter is used when the current key is custom. It sets the grayed out default option to the alphabetically first among default key sets.

                itemList = idleConf.GetSectionList('default', 'keys')
                itemList.sort()
                self.optMenuKeysBuiltin.SetMenu(itemList, itemList[0])

    itemList[0] should be replaced by your new system-sensitive idleConf.DefaultKeys().

    @serhiy-storchaka
    Copy link
    Member Author

    Updated patch addresses your comments Terry.

    Since CurrentTheme checks that the corresponding section exists, I no longer think it is buggy.

    @terryjreedy
    Copy link
    Member

    Seems to be working correctly now, so I will review the code in detail. I will look into extracting a common get_theme_keys function and editing the long docstring currently attached to CurrentTheme. I want to look at paired functions in configdialog, such as def VarChanged_builtinTheme/Keys to see if there are any unnecessary differences.

    The hardest thing is adding tests. Correct answers depend on the configuration vales, both default and user overrides. See bpo-24737 for the latter.

    @serhiy-storchaka
    Copy link
    Member Author

    After committing the patch for this issue I'm going to refactor the code related to getting/setting theme/keys settings. For now it is too complicated, the logic is scattered between two files, some code is duplicated.

    @terryjreedy
    Copy link
    Member

    The attached patch has a new function current_colors_and_keys that combines ideas and code from both CurrentTheme and CurrentKeys. The latter are now trivial wrappers. It has a new test_config that tests both this function and _warn.

    The changes to configdialog were tested 'by hand' by loading config-main.cfg into an editor, applying every theme and keyset combination I could think of, and each time re-loading the user file to see what was written. The patterns produces were used in testing current_colors_and_keys. These tests and similar configdialog tests could be automated once I revise the user confighandler so it can write to a StringIO.

    Barring a reason not to, I plan to apply tomorrow to 3.6 so this gets into alpha 3. Backports can be done later, maybe after the configdialog tests can be automated.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 10, 2016

    New changeset e6e6c71776b0 by Terry Jan Reedy in branch 'default':
    Issue bpo-27173: Add 'IDLE Modern Unix' to the built-in key sets.
    https://hg.python.org/cpython/rev/e6e6c71776b0

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 10, 2016

    New changeset 792e3294b59e by Terry Jan Reedy in branch 'default':
    Issue bpo-27173: Fix error in test_config that caused test_idle to fail.
    https://hg.python.org/cpython/rev/792e3294b59e

    @serhiy-storchaka
    Copy link
    Member Author

    Can this issue be closed?

    @terryjreedy
    Copy link
    Member

    Yes. If I decide to do a backport to either 3.5 or 2.7, I can reopen or just commit.

    @terryjreedy
    Copy link
    Member

    Any backport must include the bugfix in bpo-27821.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants