msg266801 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-06-06 15:13 |
Ping.
|
msg267581 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-06-26 22:00 |
Ping.
|
msg269327 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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)  |
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)  |
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) *  |
Date: 2016-08-14 18:34 |
Can this issue be closed?
|
msg272700 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
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) *  |
Date: 2016-08-25 02:16 |
Any backport must include the bugfix in #27821.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:31 | admin | set | github: 71360 |
2016-08-25 02:16:41 | terry.reedy | set | messages:
+ msg273620 |
2016-08-14 22:35:09 | terry.reedy | set | status: open -> closed versions:
- Python 2.7, Python 3.5 messages:
+ msg272700
resolution: fixed stage: commit review -> resolved |
2016-08-14 18:34:36 | serhiy.storchaka | set | messages:
+ msg272686 |
2016-07-10 21:26:46 | python-dev | set | messages:
+ msg270134 |
2016-07-10 17:46:48 | python-dev | set | nosy:
+ python-dev messages:
+ msg270108
|
2016-07-10 06:03:46 | terry.reedy | set | files:
+ modern-unix6.diff
messages:
+ msg270074 stage: test needed -> commit review |
2016-07-01 21:21:19 | serhiy.storchaka | set | messages:
+ msg269698 |
2016-07-01 19:43:09 | terry.reedy | set | dependencies:
+ IDLE tests must be able to set user configuration values. messages:
+ msg269692 stage: patch review -> test needed |
2016-06-30 12:11:52 | serhiy.storchaka | set | files:
+ idle_modern_unix_key_set5.patch
messages:
+ msg269577 |
2016-06-27 22:14:23 | terry.reedy | set | messages:
+ msg269407 |
2016-06-27 05:14:16 | serhiy.storchaka | set | messages:
+ msg269349 |
2016-06-27 00:23:20 | terry.reedy | set | messages:
+ msg269327 |
2016-06-26 22:00:37 | serhiy.storchaka | set | messages:
+ msg269321 |
2016-06-13 22:35:37 | ned.deily | set | messages:
+ msg268482 |
2016-06-13 22:32:34 | terry.reedy | link | issue27099 dependencies |
2016-06-13 22:06:58 | terry.reedy | set | nosy:
+ ned.deily messages:
+ msg268478
|
2016-06-11 04:40:05 | serhiy.storchaka | set | dependencies:
+ IDLE: Fix deletion of custom themes and key bindings |
2016-06-11 01:58:45 | terry.reedy | set | assignee: terry.reedy
nosy:
+ markroseman |
2016-06-07 06:54:03 | serhiy.storchaka | set | files:
+ idle_modern_unix_key_set4.patch
messages:
+ msg267600 |
2016-06-07 06:16:33 | serhiy.storchaka | set | messages:
+ msg267596 |
2016-06-07 05:51:01 | terry.reedy | set | messages:
+ msg267594 |
2016-06-07 04:37:41 | serhiy.storchaka | set | messages:
+ msg267589 |
2016-06-07 04:31:35 | serhiy.storchaka | set | messages:
+ msg267587 |
2016-06-07 01:51:17 | terry.reedy | set | messages:
+ msg267581 |
2016-06-06 15:13:05 | serhiy.storchaka | set | messages:
+ msg267536 |
2016-06-01 19:43:56 | serhiy.storchaka | set | files:
+ idle_modern_unix_key_set3.patch
messages:
+ msg266836 |
2016-06-01 18:11:40 | terry.reedy | set | messages:
+ msg266830 |
2016-06-01 11:30:56 | serhiy.storchaka | set | files:
+ idle_modern_unix_key_set2.patch
messages:
+ msg266815 |
2016-06-01 09:15:30 | terry.reedy | set | messages:
+ msg266809 |
2016-06-01 07:30:12 | serhiy.storchaka | set | nosy:
+ terry.reedy
|
2016-06-01 07:29:45 | serhiy.storchaka | create | |