Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
nvaccessAuto opened this issue Feb 1, 2011 · 8 comments
Closed

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

nvaccessAuto opened this issue Feb 1, 2011 · 8 comments
Labels
bug
Milestone
2011.1

Comments

@nvaccessAuto
Copy link

Reported by aleksey_s on 2011-02-01 21:11
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.
@nvaccessAuto
Copy link
Author

Comment 1 by aleksey_s on 2011-02-01 21:11
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?

@nvaccessAuto
Copy link
Author

Comment 2 by jteh on 2011-02-01 21:40
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.
Changes:
Milestone changed from None to 2011.1

@nvaccessAuto
Copy link
Author

Comment 3 by aleksey_s (in reply to comment 2) on 2011-02-01 21:52
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 :-)

@nvaccessAuto
Copy link
Author

Comment 4 by jteh (in reply to comment 3) on 2011-02-01 22:06
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.

@nvaccessAuto
Copy link
Author

Comment 5 by aleksey_s (in reply to comment 4) on 2011-02-01 22:24
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.

@nvaccessAuto
Copy link
Author

Comment 6 by jteh on 2011-02-01 22:30
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.

@nvaccessAuto
Copy link
Author

Comment 8 by aleksey_s on 2011-02-02 17:15
see bzr branch: http://bzr.nvaccess.org/nvda/ticket1342

@nvaccessAuto
Copy link
Author

Comment 9 by jteh on 2011-02-04 01:59
23df2e3
Changes:
State: closed

@nvaccessAuto nvaccessAuto added this to the 2011.1 milestone Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
2011.1
Development

No branches or pull requests
1 participant
@nvaccessAuto