plover
plover copied to clipboard
Update machine state on unhandled exceptions
Summary of changes
When the machine thread dies due to an unhadled exception then indicate this to the engine (and by extension to the user) by setting the machine state to "disconnected".
This is a first step. Possible follow-ups:
- We may choose to handle
serial.serialutil.SerialException
more explicitly as a "disconnect" (say, not even print a stack trace tostderr
). - What I really want from a users perspective is that the machine re-connects automatically (I think it's feasible to do so, stay tuned for a follow-up PR).
- For
SerialStenotypeBase
specifically, we also want to close the serial port on unhandled exceptions.
Even if we do (1) and/or (2) later, I think we still want this PR as a fallback for arbitrary exceptions.
I will have a separate PR for (3), but again, I think this PR provides merit on its own. So let’s focus on this first.
Closes #1559.
Pull Request Checklist
- [x] Changes have tests
- [x] News fragment added in news.d. See documentation for details
Somewhat related to #1054, although I can't immediately tell if they're identical.
@user202729 thanks for taking the time to look at this.
Somewhat related to #1054, although I can't immediately tell if they're identical.
They are addressing the same underlying issue. One difference is that #1054 is technically a breaking change as it changes the base class of ThreadedStenotypeBase
(see https://github.com/openstenoproject/plover/issues/1559).
@sammdot thanks for merging this. I had more improvements in the pipeline, but the inertia of this project let me eventually give up on it, and on steno as a whole.
The thing is that if things are fundamentally broken, there have been bug reports for years and several PRs that address these, but they don't get merged, then from a user perspective that's not very attractive. I certainly was not up for maintaining my own fork indefinitely + upstream did not work for me.
Also, don't understand me wrong, I have patience + I completely understand if nobody has time to look at some thing. I myself suffer from RSI and I have not been able to keep up with PRs to my projects in the past. I even chose to ignore PRs that I considered not crucial, or that had severe issues, like no tests or other code issues.
I tried very hard to make it as easy as possible for an interested maintainer to merge this by keeping it as focused as possible.
It was only after I realized that people are actually looking at this, and still choose not to merge it, after I realize that other PRs get merged, but not this one, when I decide to cut my looses and move on.
I understand the frustration. 😞 It's not exactly easy not having enough bandwidth (especially after the two lead devs stepped down) to take care of this on top of all my existing commitments, and as much as I would have liked to contribute more, even the process to set up a dev environment was painful for me, and I only just got more permissions to review and merge stuff a few days ago.
I can't promise that I'll be especially active moving forward, but I'm doing my best in my current situation.
(For what it's worth, having ~~had this project dropped on me~~ officially picked this project up just recently, I didn't exactly have a proper system for triaging PRs; the main perceived difficulty with this one was deciding between this and #1054, and being unable to test either locally I had to rely on CI-built artifacts. This then broke the build which I had to patch in #1634, etc. It was a whole thing 🙃)
@sammdot to be clear, that criticism was not directed towards you.