classification
Title: IDLE crash on startup with typo in config-keys.cfg
Type: behavior Stage: patch review
Components: IDLE, Tkinter Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: asvetlov Nosy List: George.Dhoore, Saimadhav.Heblikar, asvetlov, ned.deily, roger.serwy, taleinat, terry.reedy
Priority: normal Keywords: patch

Created on 2011-03-08 00:13 by George.Dhoore, last changed 2016-11-13 11:07 by BreamoreBoy.

Files
File name Uploaded Description Edit
config-keys.cfg George.Dhoore, 2011-03-08 00:13 File that crashes IDLE in interpreter mode
issue11437.patch roger.serwy, 2012-01-28 20:48 review
Messages (17)
msg130301 - (view) Author: George Dhoore (George.Dhoore) Date: 2011-03-08 00:13
If the user makes a typo when setting a custom keybind (in this case "<Alt-Key-up>" instead of "<Alt-Key-Up>") IDLE will silently crash.  From the command-line the error shows as:

Traceback (most recent call last):
  File "C:\Python32\Lib\idlelib\idle.py", line 11, in <module>
    idlelib.PyShell.main()
  File "C:\Python32\Lib\idlelib\PyShell.py", line 1388, in main
    shell = flist.open_shell()
  File "C:\Python32\Lib\idlelib\PyShell.py", line 277, in open_shell
    self.pyshell = PyShell(self)
  File "C:\Python32\Lib\idlelib\PyShell.py", line 856, in __init__
    self.history = self.History(self.text)
  File "C:\Python32\Lib\idlelib\IdleHistory.py", line 12, in __init__
    text.bind("<<history-previous>>", self.history_prev)
  File "C:\Python32\Lib\idlelib\MultiCall.py", line 332, in bind
    self.__binders[triplet[1]].bind(triplet, func)
  File "C:\Python32\Lib\idlelib\MultiCall.py", line 213, in bind
    seq, handler)))
  File "C:\Python32\Lib\tkinter\__init__.py", line 977, in bind
    return self._bind(('bind', self._w), sequence, func, add)
  File "C:\Python32\Lib\tkinter\__init__.py", line 932, in _bind
    self.tk.call(what + (sequence, cmd))
_tkinter.TclError: bad event type or keysym "up"

This is doubly confusing as case sensitivity seems to be applied at random in config-keys.cfg.  

Expected behavior:
Ideally case sensitivity shouldn't matter in config-keys.cfg and if there is an error in the config syntax, that particular line should be ignored and the rest of the file tried.  If IDLE is still able to start, some user friendly error should be displayed indicating the problem line.
msg130387 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-03-09 00:07
I presume 'silently' means when starting with an icon or shortcut, so that there is no window left to contain the traceback.

The standard key customization is by clicking, which makes errors impossible. How did you get the error? By using the "Advanced Key Binding Entry" button, which warns "These bindings will not be checked for validity!"? Or by directly editing the a file in its undocumented location (which is unknown to me)? [Question: why is either alternative
needed?]

Case sensitivity is a feature of string comparison. In this case, it is done by tcl and beyond our control. Adding validity checking where now disclaimed would be a possible feature request.

Assuming the binding is done within a loop, it would be reasonable to wrap the failing call with try...except TclError: display message "When binding key '...' to action '...', tcl reports error <emsg>".

Can you submit a patch?
msg151993 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-01-26 04:27
#13864 is a duplicate, where I mentioned the remedy of deleting the bad file.

This is similar to #5707, but I am not sure if exactly the same.
The patch there fixed one problem but not the bad key binding problem.
msg151994 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-01-26 04:33
#13071 is another duplicate closed in favor of this.
msg152187 - (view) Author: Roger Serwy (roger.serwy) * (Python committer) Date: 2012-01-28 20:48
Attached is a patch against 3.3a0 to validate the active key set in the configuration handler. It validates each key binding and then presents a dialog with a list of incorrect keys if there are any. 

In developing this patch I found that IDLE calls GetCurrentKeySet multiple times during initialization of an EditorWindow or a PyShell, once for each extension loaded. This inefficiency leads to long start times for IDLE.
msg157095 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-03-29 20:36
Roger, can you help me?

What steps should I do to reproduce the issue? Pushing config from George.Dhoore into ~/.idlerc does nothing. IDLE starts fine as usual.
msg157098 - (view) Author: Roger Serwy (roger.serwy) * (Python committer) Date: 2012-03-29 20:51
Andrew, after placing config-keys.cfg into .idlerc, launch IDLE and change the key map to Xip's keyset. You'll get a traceback on the terminal when you click ok.
msg157206 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-03-31 16:57
I reproduced the bug.

Roger Serwy, your patch looks working, but can you describe:
1. Why do you need `self.known_invalid` list?
2. Why do you always extend that list (`known_invalid` variable is 
just shared alias for `self.known_invalid` member, not list copy).

I'm a bit confusing with that logic.
msg157208 - (view) Author: Roger Serwy (roger.serwy) * (Python committer) Date: 2012-03-31 17:10
Without the known_invalid list, each call to GetCurrentKeySet would cause an error message to appear. The effect of keeping track of these bad key bindings is that the user receives a notification only once about a particular bad key binding. (The present architecture in IDLE calls GetCurrentKeySet too many times during initialization.)

The known_invalid list gets extended with any newly found invalid key bindings. Also, these newly found invalid keys generates an error message. This can occur again if the user modifies the key map with the configuration dialog.

Perhaps using a set instead of a list would make the code clearer.
msg157211 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-03-31 17:20
Ok
msg157213 - (view) Author: Roger Serwy (roger.serwy) * (Python committer) Date: 2012-03-31 17:35
I just realized that if the user, for whatever reason, fixes a bad key binding and then rebinds it to the previously bad one, no error message would appear. This can occur if the user is switching between key sets where one key set contains invalid bindings.
msg157215 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-03-31 17:39
You right. That was my concern.
msg220093 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-09 14:08
@Roger, Andrew could you pick this up again?
msg220107 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-06-09 18:29
Mark, they are both inactive at the moment.

There are a number of issue around configuration, especially key bindings. I think the whole process needs review and redoing. For instance, I understand Roger as saying that the problematical known-invalid list (set) is needed because GetCurrentKeySet is called repeatedly, once for each extension. I want to look into fixing the repeated calls, which will increase with more extensions, and possibly eliminate the need for known-invalid.

Another redesign: I think Control-Key-x should automatically mean Control-Key-X, and ditto for Alt-Key-x, and that we should not need both in config-keys even if tk requires separate bind calls.

Saimadhav, we should try to do some validation of the key bindings in the unittest for configurations. The pattern seems to be
  optional Control-, Alt-, or Meta-
  optional Shift-
  required Key-
  required ascii key, bracketleft/right, or Uppercase-key-name,
msg220391 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-06-12 21:49
I don't think the patch should currently be committed.

I agree with Terry: we should first fix the issue whereby the key config is read repeatedly. Given such a fix, the problematic "known_invalid" workaround in the patch would no longer be necessary.

As a side note, I think the method used in the patch to check if a binding is good. Ideally it would be the final check done after a simpler syntax check, since a syntax check could give more informative error messages.

Also, there are two other more technical issues with the patch; I mentioned them in the patch review system.
msg226890 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-09-14 23:47
https://stackoverflow.com/questions/25750477/python-2-7-8-idle-will-not-open-on-mavericks -- because one of the two files has 'shift' instead of Shift.  When I make a change in either file (with custom keys being used), the traceback ends slightly differently until the final message.

  File "C:\Programs\Python34\lib\idlelib\EditorWindow.py", line 1100, in apply_bindings
    text.event_add(event, *keylist)
  File "C:\Programs\Python34\lib\idlelib\MultiCall.py", line 379, in event_add
    widget.event_add(self, virtual, seq)
  File "C:\Programs\Python34\lib\tkinter\__init__.py", line 1441, in event_add
    self.tk.call(args)

#21519 is about checking bindings entered in the dialog with either the basic or advanced methods.  Bindings also need to be checked when read so we know which file is bad.  Exception tracebacks should be replaced by more helpful messages that include fix-up instructions and which do not disappear into the void when running on pythonw.
msg280685 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-11-13 04:10
I closed #25662 in favor of this.
History
Date User Action Args
2016-11-13 11:07:34BreamoreBoysetnosy: - BreamoreBoy
2016-11-13 04:10:14terry.reedysetmessages: + msg280685
2016-11-13 04:08:18terry.reedylinkissue25662 superseder
2014-09-14 23:47:28terry.reedysetmessages: + msg226890
2014-06-12 21:49:36taleinatsetnosy: + taleinat
messages: + msg220391
2014-06-09 21:46:23terry.reedysetstage: needs patch -> patch review
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2014-06-09 18:29:13terry.reedysetmessages: + msg220107
2014-06-09 14:11:13Saimadhav.Heblikarsetnosy: + Saimadhav.Heblikar
2014-06-09 14:08:57BreamoreBoysetnosy: + BreamoreBoy
messages: + msg220093
2012-03-31 17:39:58asvetlovsetmessages: + msg157215
2012-03-31 17:35:15roger.serwysetmessages: + msg157213
2012-03-31 17:20:52asvetlovsetmessages: + msg157211
2012-03-31 17:10:55roger.serwysetmessages: + msg157208
2012-03-31 16:57:17asvetlovsetmessages: + msg157206
2012-03-29 20:51:40roger.serwysetmessages: + msg157098
2012-03-29 20:36:16asvetlovsetmessages: + msg157095
2012-03-14 21:53:11asvetlovsetassignee: asvetlov

nosy: + asvetlov
2012-01-28 20:48:34roger.serwysetfiles: + issue11437.patch
keywords: + patch
messages: + msg152187
2012-01-26 04:39:54terry.reedylinkissue13864 superseder
2012-01-26 04:35:35terry.reedylinkissue13071 superseder
2012-01-26 04:33:22terry.reedysetmessages: + msg151994
2012-01-26 04:27:24terry.reedysetnosy: + roger.serwy

messages: + msg151993
versions: - Python 3.1
2011-03-09 00:07:40terry.reedysetnosy: + terry.reedy
messages: + msg130387
2011-03-08 01:48:15ned.deilysetnosy: + ned.deily
stage: needs patch
type: crash -> behavior

versions: + Python 3.1, Python 2.7, Python 3.3
2011-03-08 00:13:57George.Dhoorecreate