Opened 4 years ago

Closed 4 years ago

#1342 closed defect (fixed)

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

Reported by: aleksey_s Owned by: aleksey_s
Priority: major Milestone: 2011.1
Component: Core Version: master
Keywords: Cc:
Operating system: Blocked by:
Blocking:
Changes document entry (for developers):

Description

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.

Change History (9)

comment:1 Changed 4 years ago by 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?

comment:2 follow-up: Changed 4 years ago by jteh

  • Milestone set to 2011.1
  • Owner set to jteh
  • Priority changed from minor to major
  • Status changed from new to assigned

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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by 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 :-)

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by aleksey_s

  • Owner changed from jteh to aleksey_s
  • Status changed from assigned to accepted

comment:9 Changed 4 years ago by jteh

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.