On 6/11/2014 7:21 AM, Tal Einat wrote:
On Wed, Jun 11, 2014 at 10:31 AM, Terry Reedy <tjre...@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
_______________________________________________
IDLE-dev mailing list
IDLE-dev@python.org
https://mail.python.org/mailman/listinfo/idle-dev
