classification
Title: Modern Unix key bindings for IDLE
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 27245 27437 Superseder:
Assigned To: terry.reedy Nosy List: markroseman, ned.deily, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2016-06-01 07:29 by serhiy.storchaka, last changed 2016-08-25 02:16 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
idle_modern_unix_key_set.patch serhiy.storchaka, 2016-06-01 07:29 review
idle_modern_unix_key_set2.patch serhiy.storchaka, 2016-06-01 11:30 review
idle_modern_unix_key_set3.patch serhiy.storchaka, 2016-06-01 19:43 review
idle_modern_unix_key_set4.patch serhiy.storchaka, 2016-06-07 06:54 review
idle_modern_unix_key_set5.patch serhiy.storchaka, 2016-06-30 12:11 review
modern-unix6.diff terry.reedy, 2016-07-10 06:03 refactored function and tests review
Messages (27)
msg266801 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-01 07:29
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.
msg266809 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-01 09:15
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?
msg266815 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-01 11:30
> 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.
msg266830 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-01 18:11
Original IDLE Dark patch: #24820. Follow-up for cross-version issues (initially for themes, but with a eye to keys, etc): #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 #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.
msg266836 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-01 19:43
New patch makes IDLE silently fall back to default key set and adds a note about new key set on Keys configuration tab.
msg267536 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-06 15:13
Ping.
msg267581 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-07 01:51
I looked at _set3 and it looks plausible and possibly complete other than testing.  I added something about testing customizations to #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, #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.
msg267587 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-07 04:31
I'm aware of the bug with deleting current key set. idle_modern_unix_key_set3.patch effectively fixes it.
msg267589 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-07 04:37
After applying this patch I'm going to redesign Highlighting and Keys tabs in the Settings dialog.
msg267594 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-07 05:51
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 #24776.  The highlight tab is #24781.  The latest mockups look like improvements to me, but we can discuss further.  (#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.
msg267596 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-07 06:16
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.
msg267600 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-07 06:54
There was a bug, saving custom key set didn't have any effect if previous key set was IDLE Modern Unix.
msg268478 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-13 22:06
Ned, Serhiy's patch 4 solves one of your concerns in #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.
msg268482 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-06-13 22:35
"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).
msg269321 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-26 22:00
Ping.
msg269327 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-27 00:23
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.
msg269349 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-27 05:14
I can't reproduce your issue (or understand it correctly). Could you please show the [Keys] section in your config-main.cfg?
msg269407 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-27 22:14
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().
msg269577 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-30 12:11
Updated patch addresses your comments Terry.

Since CurrentTheme checks that the corresponding section exists, I no longer think it is buggy.
msg269692 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-07-01 19:43
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 #24737 for the latter.
msg269698 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-01 21:21
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.
msg270074 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-07-10 06:03
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.
msg270108 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-10 17:46
New changeset e6e6c71776b0 by Terry Jan Reedy in branch 'default':
Issue #27173: Add 'IDLE Modern Unix' to the built-in key sets.
https://hg.python.org/cpython/rev/e6e6c71776b0
msg270134 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-10 21:26
New changeset 792e3294b59e by Terry Jan Reedy in branch 'default':
Issue #27173: Fix error in test_config that caused test_idle to fail.
https://hg.python.org/cpython/rev/792e3294b59e
msg272686 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-14 18:34
Can this issue be closed?
msg272700 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-08-14 22:35
Yes.  If I decide to do a backport to either 3.5 or 2.7, I can reopen or just commit.
msg273620 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-08-25 02:16
Any backport must include the bugfix in  #27821.
History
Date User Action Args
2016-08-25 02:16:41terry.reedysetmessages: + msg273620
2016-08-14 22:35:09terry.reedysetstatus: open -> closed
versions: - Python 2.7, Python 3.5
messages: + msg272700

resolution: fixed
stage: commit review -> resolved
2016-08-14 18:34:36serhiy.storchakasetmessages: + msg272686
2016-07-10 21:26:46python-devsetmessages: + msg270134
2016-07-10 17:46:48python-devsetnosy: + python-dev
messages: + msg270108
2016-07-10 06:03:46terry.reedysetfiles: + modern-unix6.diff

messages: + msg270074
stage: test needed -> commit review
2016-07-01 21:21:19serhiy.storchakasetmessages: + msg269698
2016-07-01 19:43:09terry.reedysetdependencies: + IDLE tests must be able to set user configuration values.
messages: + msg269692
stage: patch review -> test needed
2016-06-30 12:11:52serhiy.storchakasetfiles: + idle_modern_unix_key_set5.patch

messages: + msg269577
2016-06-27 22:14:23terry.reedysetmessages: + msg269407
2016-06-27 05:14:16serhiy.storchakasetmessages: + msg269349
2016-06-27 00:23:20terry.reedysetmessages: + msg269327
2016-06-26 22:00:37serhiy.storchakasetmessages: + msg269321
2016-06-13 22:35:37ned.deilysetmessages: + msg268482
2016-06-13 22:32:34terry.reedylinkissue27099 dependencies
2016-06-13 22:06:58terry.reedysetnosy: + ned.deily
messages: + msg268478
2016-06-11 04:40:05serhiy.storchakasetdependencies: + IDLE: Fix deletion of custom themes and key bindings
2016-06-11 01:58:45terry.reedysetassignee: terry.reedy

nosy: + markroseman
2016-06-07 06:54:03serhiy.storchakasetfiles: + idle_modern_unix_key_set4.patch

messages: + msg267600
2016-06-07 06:16:33serhiy.storchakasetmessages: + msg267596
2016-06-07 05:51:01terry.reedysetmessages: + msg267594
2016-06-07 04:37:41serhiy.storchakasetmessages: + msg267589
2016-06-07 04:31:35serhiy.storchakasetmessages: + msg267587
2016-06-07 01:51:17terry.reedysetmessages: + msg267581
2016-06-06 15:13:05serhiy.storchakasetmessages: + msg267536
2016-06-01 19:43:56serhiy.storchakasetfiles: + idle_modern_unix_key_set3.patch

messages: + msg266836
2016-06-01 18:11:40terry.reedysetmessages: + msg266830
2016-06-01 11:30:56serhiy.storchakasetfiles: + idle_modern_unix_key_set2.patch

messages: + msg266815
2016-06-01 09:15:30terry.reedysetmessages: + msg266809
2016-06-01 07:30:12serhiy.storchakasetnosy: + terry.reedy
2016-06-01 07:29:45serhiy.storchakacreate