NVDA doesn't handle syntax errors in user-defined gestures

[aleksey_s]

Curently, NVDA misbehaves when there is an error in user gestures.ini file. It simply does not start with a critical error.
STR:

• create userConfig/gestures.ini with some weird text in it
• start NVDA

Current results:

CRITICAL - nvda (22:56:59):
core failure
Traceback (most recent call last):
  File "D:\nvda\main\source\nvda.pyw", line 139, in <module>
    core.main()
  File "D:\nvda\main\source\core.py", line 132, in main
    import speechDictHandler
  File "D:\nvda\main\source\speechDictHandler.py", line 13, in <module>
    import api
  File "D:\nvda\main\source\api.py", line 13, in <module>
    import ui
  File "D:\nvda\main\source\ui.py", line 13, in <module>
    import braille
  File "D:\nvda\main\source\braille.py", line 12, in <module>
    import keyboardHandler
  File "D:\nvda\main\source\keyboardHandler.py", line 21, in <module>
    import inputCore
  File "D:\nvda\main\source\inputCore.py", line 359, in <module>
    manager = InputManager()
  File "D:\nvda\main\source\inputCore.py", line 241, in __init__
    self.loadUserGestureMap()
  File "D:\nvda\main\source\inputCore.py", line 318, in loadUserGestureMap
    self.userGestureMap.load(os.path.join(globalVars.appArgs.configPath, "gestures.ini"))
  File "D:\nvda\main\source\inputCore.py", line 161, in load
    conf = ConfigObj(filename, file_error=True, encoding="UTF-8")
  File "c:\python27\lib\site-packages\configobj.py", line 1219, in __init__
    self._load(infile, configspec)
  File "c:\python27\lib\site-packages\configobj.py", line 1302, in _load
    raise error
ParseError: Invalid line at line "1".

Expected results:
NVDA should inform user about errors in the gestures.ini file and continue loading. Similar to error handling for the config file.

[aleksey_s]

The code path that causes user gesture map to be loaded seems a bit strange to me. Having a global object that initializes on import time with lots of side effects isn't good for example when you want to generate module doc, or am I not right?

[jteh]

Thanks for reporting.

InputManager.loadUserGestureMap and InputManager.loadLocaleGestureMap just need to catch configobj.ConfigObjError and log an appropriate error message. Constructing InputManager must never fail.

Having a global object initialised at import time isn't really a problem for module doc, but it certainly is a bit strange. When I originally wrote that code, there weren't many side effects, but there are now. Therefore, it probably makes sense to have an initialize function in the module. However, this isn't actually needed to fix this bug.

[aleksey_s]

Replying to jteh:

Thanks for reporting.

That was originally on NVDA-support list.

InputManager.loadUserGestureMap and InputManager.loadLocaleGestureMap just need to catch configobj.ConfigObjError and log an appropriate error message.

I think that behavior should be similar to that for config errors: show an alert with list of errors. This is more user-friendly and consistent.

Constructing InputManager must never fail.

May be consider moving loading of maps into standalone function? It will be nice to reload user map when reverting to saved configuration, for example.

Having a global object initialised at import time isn't really a problem for module doc, but it certainly is a bit strange. When I originally wrote that code, there weren't many side effects, but there are now. Therefore, it probably makes sense to have an initialize function in the module. However, this isn't actually needed to fix this bug.

Sure. But it's worth-doing anyway :-)

[jteh]

Replying to aleksey_s:

InputManager.loadUserGestureMap and InputManager.loadLocaleGestureMap just need to catch configobj.ConfigObjError and log an appropriate error message.

I think that behavior should be similar to that for config errors: show an alert with list of errors. This is more user-friendly and consistent.

The errors shown by config are only validation errors. We don't validate gesture maps (there's no need; they're just strings) and there can only be one parse error. The validation part is handled separately, and currently, it just logs error messages. I guess we could make them add to a list on the gesture map or something, but that's pretty ugly.

Also, even if we do show an alert for user maps, we most definitely shouldn't do it for locale maps. Those should just be logged.

May be consider moving loading of maps into standalone function? It will be nice to reload user map when reverting to saved configuration, for example.

They're already in standalone functions: InputManager.loadUserGestureMap and InputManager.loadLocaleGestureMap.

[aleksey_s]

Replying to jteh:

The errors shown by config are only validation errors. We don't validate gesture maps (there's no need; they're just strings) and there can only be one parse error. The validation part is handled separately, and currently, it just logs error messages. I guess we could make them add to a list on the gesture map or something, but that's pretty ugly.

We definitely must inform user about bad formed gesture map or errors (such as unrecognizable gesture string) as we don't provide UI for assign gestures, and error sound is turned off in final releases, so there isn't any notice for users to know that they has problems with their gesture map.

Also, even if we do show an alert for user maps, we most definitely shouldn't do it for locale maps. Those should just be logged.

Agreed.

[jteh]

Do you have time to have a go at this in the next couple of days? If so, please take it and branch. Otherwise, I'll do it, as we need it to be done before the next pre-release.

[aleksey_s]

see bzr branch: ​http://bzr.nvaccess.org/nvda/ticket1342

[jteh]

changeset:23df2e300577ded5df3c6118be2389325eafe002