[Idle-dev] KeyConfig, KeyBinding and other related issues.

[Terry Reedy]

On 6/11/2014 7:21 AM, Tal Einat wrote:
> On Wed, Jun 11, 2014 at 10:31 AM, Terry Reedy <tjreedy at udel.edu> wrote:
>>
>> On 6/11/2014 2:39 AM, Saimadhav Heblikar wrote:
>>>
>>> The following are the issues related to shortcuts, keybinding dialog,
>>> where the shortcuts are set and config-keys.def where the shortcuts
>>> are stored.
>>
>>
>> Great: I have been collecting a list of configuration and especially keybinding issues, with the thought of suggesting this as a coherent group of issues you could work on.
>>
>> 3068: extension CONFIG dialog, Tal new PATCH, review
>> 12274: CONFIG syntax error crash
>> 12387: CONFIG KEYboard shortcuts
>> 4765: CONFIG custom KEYset deletion, patch fixes (Mark)
>> 11437: CONFIG-KEY type causes crash; startup is inefficient. Serwy patch
>> 1517993: CONFIG KEY and Macs: (me) Alt to Meta (closed)
>> 20580: CONFIG KEY and and Macs
>> 694339: dedenting with shift-tab CONFIG KEY
>>
>> I just added 21519. Related ideas I have not opened issues for:
>>
>> Save config-KEYs sorted on disk instead of when present in CONFIG DIALOG. Would be much easier to read, just a matter of when sort.
>>
>> Make CONFIG DIALOG wide enough for all KEY definitions. Also lengthen.
>>
>> Change some names so they sort better: possible example region-dedent, region-indent instead of dedent-region, indent-region.
>>
>> Since I have a backlog of new test files to review, + Tal's config-extensions patch (+ a patch by Lita), I was about to suggest that you find something like this (configuration and keybinding) to work on that Tal knows better than me, and could help you with.
>>
>> It would be great to fix some of these issues that have dragged on for years. If either of you think that some fixes need deeper changes than the typical surface patches, please say so so we can discuss. The better we make the tests, the more confidently we can consider refactorings.
>>
>> I will look at the specifics below tomorrow. I would like to see Tal's comments either by email or on the tracker.
>>
>> Terry
>>
>>
>>> 1) Issue12387: Very simple way to reproduce the bug:

Your example below misses the point of 
http://bugs.python.org/issue12387. An example of the bug is given in the 
first message: "IDLE [on Windows] fails to save using the ctrl-s 
keyboard shortcut when caps-lock is enabled". This is because for 
CapsLock to have no effect on Windows,
   save-window=<Control-Key-s>
needs to be, as changed in the patch,
   save-window=<Control-Key-s> <Control-Key-S>
In general, this issue and patch is about adding missing members of such 
pairs. The only reason I have not applied the patch yet because I think 
there is a better solution, which is to make it unnecessary to have the 
other case version in config-keys.def. However, since a deeper change is 
in the future, if ever, and since the current inconsistent and buggy 
state of the Windows section of config-keys.def makes proper testing 
impossible without failures, I am reviewing the patch now and will 
probably commit it today. Then test_configuration can and should test 
that in the Windows section, both prefix-Key-x, for x alpha, and 
prefix-Key-X,  are present in the set of bindings. The test can change 
if the duplication is made unnecessary.

--
After writing this, I discovered that 
ConfigHandler.IdleConf.GetCurrentKeySet adjusts definitions on 'Darwin' 
by changing 'Alt' to 'Option'. On Windows and Linux, instead of 
replacing, uppercased key definitions could be added, as appropriate.
---

>>> With/without CAPS, Ctrl + x key, performs Cut action(windows keyset).
>>> This is agreeable because both those keybindings are set. The bindings
>>> are
>>> <Control-Key-x> and <Control-Key-X>
>>> But, with/without CAPS, Ctrl + Shift + x key also performs Cut action.

Correct, and this should continue as is except where there is an explict 
Control-Shift binding. The current examples of such explicit bindings 
for Windows are
   redo=<Control-Shift-Key-Z>
   save-window-as-file=<Control-Shift-Key-S>
The patch adds the lowercase versions needed for these binding to work 
with CapsLock on. I have already confirmed that with the patch, ^s and 
^shift-s each work as expected without and with caps-lock on. Ditto for 
^x and ^shift-z. So the patch fixes the bugs it is aimed at.

>>> The bindings are <Control-Shift-Key-x> and <Control-Shift-Key-X>
>>> *Workaround*:
>>> Bind the redundant <Control-Shift-Key-x> and <Control-Shifrt-Key-X> to
>>> <<do-nothing>>.(already exists).
>>>
>>> This has to be done for all existing <Ctrl+[A-Z][a-z]> key combinations.
>>> For sake of completeness, If the user wants Control + Shift + *x key*,
>>> we have to remove both from <<do-nothing>> keybinding and add it to
>>> whatever binding that the user wants.
>>> This has to be done in the current validity checking method or a new
>>> parsing method.
>>> I have tested this solution, and am convinced it would work.

Disabling ^shift-alpha would be new issue. It might break current habits 
and should probably be rejected on that basis. In the absence of users 
claiming that they are confused, it would be a waste of time.


>>> 2) Issue11437: Its hard to explain this issue in short.

In long: 12387 is about the config-keys.def that we deliver. 21696 is 
about testing all the default confix-xyz.defs we deliver. However, that 
is not enough because users can edit these files (but should not) and 
more so because they can easily make bad user .defs either with the key 
dialog or by direct editing. (Indeed, the Basic method prevents making 
the alpha pairs, discussed above, needed for Windows. Fixing this is a 
separate issue.) 11437 is about what Idle should do other than closing 
when it encounters a bad key binding.

Serwy's patch validate keys when read. There are two sub-issues.

The patch validates by binding to a temporary Tk() instance and catching 
the TclException. An alternative is to to developed a parser sufficient 
to own needs that also gives more specific errors and which can be used 
with the key dialog. I agree that we should attempt this.

To work around the fact that Idle "calls GetCurrentKeySet for each 
loaded extension" and to avoid repeated error messages, the patch tries 
to save errors that have been reported. The comments indicate that this 
hack has glitches. I would strongly prefer avoiding the repeated calls.

 >>> Please read the issue
>>> at http://bugs.python.org/issue11437
>>> Workaround:
>>> This is easy to solve, if we use the solution from issue12387.

Completing windows binding pairs in config-keys.def has little to do 
with this issue.

>>> With the parser method, both "simple" and "advanced" dialogs
 >>> will be parsed, and we will have a 1-to-1 mapping.

I am not exactly sure what you mean by 1-to-1 mapping in this context. 
Between what and what?

>>> For the case when someone tries to directly "hand-edit" the config
>>> files, with http://bugs.python.org/issue21696 and tests for the parser
>>> method in place, we should able to raise an earlier, ideally before
>>> IDLE starts.

I don't see how we can do anything between when Idle leaves the factory 
and when the user starts Idle -- unless the user happens to run the tests.

 >>> We should also be able to pinpoint where the error occurs.
>>>
>>>
>>> 3) Issue20580: This does not involve coding, but is about providing
>>> platform specific default config. This can only be done, once we agree
>>> on 1 and 2.

This presents somewhat vague problems and little solution. I added a few 
comments. This is an example of where we need specific (failing) tests 
to provide specific targets for improvement.

>>> 4) Issue21519: Again dependent on 1 and 2. Not too sure, if it is
>>> going to be an issue with 1 and 2. If we go ahead with 2, and have a
>>> parser method which is testable, we can also validate the "advanced"
>>> dialog.
>>>
>>>
>>> For other issues which are caused by typo in config,
>>> http://bugs.python.org/issue21696 along with testable parser method
>>> should catch them.
>>> -----------------
>>>
>>> I plan to work on these issues in test driven style as suggested by
>>> Tal Einat, especially keeping in mind the above issues, their current
>>> outcome and the required outcome.

> Saimadhav and I discussed this a bit on IRC yesterday. We started by
> discussing caps-lock's effect on key bindings. My conclusions are:
>
> 1) We don't want caps-lock to affect IDLE's key bindings.

Right. As I reported

> 2) Tk reacts to caps-lock in different ways on different platforms. I
> think we should ignore it on all platforms, which means non-trivial
> work to get that working right with Tk.

As I reported on 12387. http://wiki.tcl.tk/28331 appears to say that it 
Shift undoes ShiftLock except on Mac, where it does nothing. Hence the 
issue applied to both Windows and Unix. On the other hand, Roger, who 
reported a problem on Linux, only patch the Windows defs. Perhaps that 
is because the Unix defs are consistent in never having the uppercase 
version, and he wanted a test on Windows first where the file is 
inconsistent and a majority already have the uppercase version.

> 3) Specifically, Tk sometimes changes the case of the letter in the
> event it generates depending on whether caps-lock and/or shift are
> active. For example, we could get Ctrl-S instead of Ctrl-s if caps
> lock is on. How exactly this behaves differs by platform (!). For
> example, caps-lock + shift can result on either an upper- or
> lower-case letter in the event string, depending on platform. This is
> causing the issues with keyboard shortcuts not working with caps-lock
> active on some platforms.
> 4) A possible solution is to treat all keyboard shortcuts which are
> bound to a letter in a case-insensitive manner. This would seem to
> require parsing Tk event definition strings in order to normalize
> them, and binding to both the upper- and lower-case letter Tk events.

We agree. When this is done, the duplicates would be stripped from the 
Windows definitions.

> 5) Parsing the event strings has additional benefits, such as proper
> validation of custom events defined in the "advanced" section of the
> key binding config. (Validations include checking if there are already
> bindings which would catch that event).

I agree here too.

> This is basically what Terry suggests in his "1)" above, and he says

I believe you meant Saimadhav. I read him as also suggesting a 
complicated fix to disable non-specific Alt-Shift, which I think should 
not be done.

> he's already tested the approach and is convinced it would work. So it
> seems we are all agreed on the suggested implementation method :)

Except for the above, I think so.

> Additionally, I suggested that if we go down the route of such a
> "deep" change, it could be useful to write at least some tests in
> advance for things that do and don't currently work. This will help
> make sure that we have a good definition of what currently doesn't
> work properly, and that we get it working properly at the end.
> However, how good idea this is depends on how difficult it is to write
> such tests for the current implementation.
>
> Finally, there is some existing relevant code which parses and
> normalizes TK event strings in idlelib/MultiCall.py. It would perhaps
> be better to write simpler, more specifically appropriate code, but
> that could be a starting point. Terry, perhaps you have something
> better that you used in the test you mentioned?

/Terry/Saimadhav/?

-- 
Terry Jan Reedy