Add error handling for layout update on macOS#1771
Conversation
There was a problem hiding this comment.
Thank you very much for this PR and the background on the issue!
I was able to reproduce it with Bulgarian. Interestingly, Russian works fine so it's not all non-latin layouts.
Right now I also don't know a better way to fix it but not crashing is certainly an improvement so let's go for this approach.
I have one small remark and it would be great if you could add a news file, see here.
And no need to add more info to the commit message since the commit will link to this PR anyways.
|
while programmers are not perfect and continue with a warning is better than outright crash, given that the code that crash is itself the crash handler, the crash handler is not working. I suggest the following. or whichever you prefer. I believe the second version should give less verbose error message. Also, is
if yes then there isn't an issue, if no then we should add |
|
@user202729 I'm not sure I follow. At the very least, I don't want to see this message every time I change languages. If I understand your suggestion correctly, it will be popping up all the time when I'm not doing actively Steno, but I want to keep Plover open in case I need to steno. @mkrnr Otherwise, Plover not crashing in Russian is an interesting datapoint. I'll see if I can identify something funky in Bulgarian BDS that I could actually fix in |
On macOS and with some input sources (Bulgarian BDS in particular),
keyboardlayout.py doesn't properly parse the layout, throws an error,
and makes Plover crash:
Traceback (most recent call last):
File "/Users/skanev/code/opensource/plover/plover/scripts/main.py", line 147, in main
code = gui.main(config, controller)
File "/Users/skanev/code/opensource/plover/plover/gui_qt/main.py", line 125, in main
app = Application(config, controller, use_qt_notifications)
File "/Users/skanev/code/opensource/plover/plover/gui_qt/main.py", line 61, in __init__
config, controller, KeyboardEmulation()
~~~~~~~~~~~~~~~~~^^
File "/Users/skanev/code/opensource/plover/plover/oslayer/osx/keyboardcontrol.py", line 412, in __init__
self._layout = KeyboardLayout()
~~~~~~~~~~~~~~^^
File "/Users/skanev/code/opensource/plover/plover/oslayer/osx/keyboardlayout.py", line 130, in __init__
self._update_layout()
~~~~~~~~~~~~~~~~~~~^^
File "/Users/skanev/code/opensource/plover/plover/oslayer/osx/keyboardlayout.py", line 156, in _update_layout
KeyboardLayout._get_layout()
~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/skanev/code/opensource/plover/plover/oslayer/osx/keyboardlayout.py", line 268, in _get_layout
parsed_layout = KeyboardLayout._parse_layout(layout_buffer, keyboard_type)
File "/Users/skanev/code/opensource/plover/plover/oslayer/osx/keyboardlayout.py", line 423, in _parse_layout
mod = modifier_masks[i]
~~~~~~~~~~~~~~^^^
KeyError: 9
This happens both when Plover is started while in Bulgarian, and also
when switching from English to Bulgarian.
This change suppresses the crash when the input source is changed. That
way you can start Plover in English, and if you switch to Bulgarian, it
would not crash. It will output gibberish in Cyrillic, but that's OK, as
there is no Bulgarian steno yet anyway. Plover still crashes when
started in Bulgarian, but that can be worked around by starting it in
English.
|
@mkrnr I've applied your requests. I've also rebased on latest main, and wrote a better commit message. The branch name is unfortunately still I went on a side quest to understand why the parsing of the macOS layout fails, but unfortunately, it's quite dense to figure out and it felt this is a reasonable change anyway. |
Apologies for opening the pull request this way, but I'm not quite sure how to solve this issue, and wanted to start a conversation. I'm also proposing a hacky solution that is good enough for me in case it meets the bar.
The issue
The problem happens when I'm using Plover on OS X on a computer that has multiple input sources (English and Bulgarian in my case). Plover works fine when I start it while on the English input source, but crashes when I change to the Bulgarian input source. You can reproduce it by:
This makes it effectively unusable, because every time I need to switch languages and reach for a regular keyboard, I have to restart it.
I've also opened an issue previously, #1708. I've seen a few similar issues in GitHub and Discord, but don't have the time to find them right now.
The "fix"
I've basically just wrapped
_update_layout()in atry/except. Changing to Bulgarian no longer crashes the application. If I use steno while in Bulgarian, Plover would output gibberish (in Cyrillic), but that's good enough for me. I also have to be in English when I launch it, but that's also good enough for me.There is no regression when the user has multiple latin-based layouts (e.g. Qwerty and Colemak), which I assume is the use case for this feature in the first place.
How could it be done differently
KeyboardLayouthas an optional constructor argument,watch_layoutthat's always defaulted toTrue. If it could be configured through the settings, that will probably solve my problem too, but I couldn't quite figure how to add it there, and also how to start/stop the watcher when the user changes that setting.The code could also be written with a more precise exception handling, but that would require a bigger rewrite. Right now, the exception I'm getting is:
Finally, the watcher could just be removed, but I guess this will be a regression for people actually using multiple latin layouts.
If you're actually happy with this change, I'm also happy to ammend a more detailed commit message that explains the problem so it remains in the git history, just let me know if I should.