Skip to content

Conversation

@iansw246
Copy link
Contributor

@iansw246 iansw246 commented Aug 10, 2025

Summary of changes

For Linux Uinput, signal the evdev thread to stop using a pipe. Currently, the thread periodically checks a variable to know when to stop.

This eliminates the slight delay between clicking the reconnect button and the reconnection actually happening.

Closes

Pull Request Checklist

  • Changes have tests
  • News fragment added in news.d. See documentation for details

mkrnr
mkrnr previously approved these changes Aug 12, 2025
Copy link
Contributor

@mkrnr mkrnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this improvement!

I tested it on Ubuntu and it works well.

GPT-5 flagged some potential lifecycle concerns, but given how this class is used, I don’t see any real issues.

Just a minor note about some unused imports.

Feel free to merge whenever you’re ready.

mkrnr
mkrnr previously approved these changes Aug 13, 2025
@mkrnr
Copy link
Contributor

mkrnr commented Aug 17, 2025

@iansw246 are you allowed to merge on your own? If yes, feel free to do so. If not, let me know when ready and I merge.

@iansw246
Copy link
Contributor Author

iansw246 commented Aug 17, 2025

I didn't see a merge button after you approved merging (before these new changes), so I assume am not able to merge myself.

I think it's ready to merge now.

mkrnr
mkrnr previously approved these changes Aug 17, 2025
Copy link
Contributor

@mkrnr mkrnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a trivial remark regarding a comment. I like the additional error handling!

log.debug("failed to ungrab device", exc_info=True)

def start(self):
# Exception note: cancel() will eventually by called when the machine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial: The part "when the machine reconnect machine is pressed" sounds a bit wrong. Maybe remove one "machine"

@mkrnr mkrnr merged commit ca89e3a into opensteno:main Aug 17, 2025
15 checks passed
@user202729
Copy link
Member

I guess the exception handling counts for defensive programming (a warning message and program carry on is better than Plover hangs/crashes)

there's a disadvantage with the extra exception handling though, it makes the code harder to read for developers, and the text says the exact same thing as the code (thus doesn't save the developer any time).

I don't know in which case can the exception be thrown at all (if it cannot be thrown the exception handling is obviously redundant), but maybe one case is race condition between the two threads? In that case it may be a better idea to communicate with queues instead, or locks if the above is insufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants