wavexx / screenkey

A screencast tool to display your keys inspired by Screenflick
https://www.thregr.org/~wavexx/software/screenkey/
GNU General Public License v3.0
758 stars 65 forks source link

Fix exit on Ctrl+C from the terminal where app was started #53

Closed mk-fg closed 6 years ago

mk-fg commented 7 years ago

When starting screenkey from an open terminal, python creates KeyboardInterrupt-raising SIGINT handler, which does not work well with GTK. As there's no value in raising KeyboardInterrupt here, simple solution is to reset handler to its default before entering gtk eventloop.

To test, run e.g. screenkey --no-detach ... in the terminal and try to Ctrl+C it before/after the patch.

Not sure if starting thing from terminal is that common use-case, but that's definitely most convenient way to set/repeat various parameters for me.

Thanks!

wavexx commented 6 years ago

I concur. I do run screenkey from the terminal. For this, I generally run the main loop wrapped and trap the interrupt manually:

try:
    gtk.main()
except KeyboardInterrupt:
    return 0

any advantage in resetting the signal instead?

mk-fg commented 6 years ago

Pretty sure your solution shouldn't always work - main "does not work well with GTK" issue is not that you get KeyboardInterrupt traceback instead of silent exit, but that it doesn't exit at all.

Chain of events should go something like this:

I.e. until control gets back to python, nothing happens, exception doesn't get raised. And in fact, pretty sure it might get cleared somewhere along the way and won't ever be raised.

So from the outside, Ctrl+C only works as expected in cases when python code happens to be running right after signal handler gets called.

Thus removing this signal handler seem to be the only clean solution.

mk-fg commented 6 years ago

Though I wonder if try: ... except: ... you've proposed above indeed works around this "GTK event loop just keeps running" issue, i.e. python signal handler never returns control there, even though that sounds kinda bogus - it'd still need to call clean stop on the thing, let it run until it stops, free() it all, etc, not just os._exit(0) on it. It shouldn't afaik, but I don't know that much either :)

mk-fg commented 6 years ago

removing this signal handler seem to be the only clean solution.

Should've said "...out of the two" there, sorry.

Actual "clean" solution would be to glib.add_signal_handler(sigint, loop.stop()) or such.

wavexx commented 6 years ago

Ok, technically glib supports unix_signal_add, which would properly fix the issue as we could call main_stop() from within the main loop.

But guess what? It's only through the "gi" interface, which conflicts with pygtk. Since we don't do any shutdown, calling _exit() is still the best option so far.

Closing the PR in favor of a new issue to be properly fixed later.

mk-fg commented 6 years ago

Doubt it's really worth it for this issue, where indeed _exit or whatever other method would work, but I think you can work around that with python's signal.signal + io_add_watch, emulating something like signal.set_wakeup_fd but only for signals you want.

For example, create pipe (os.pipe(), they're always buffered), set signal handler to write a byte to it, register read fd via io_add_watch, and when signal comes glib will run that io handler code as it'll see new byte there. More glib-friendly than running whatever code in async signal handler directly, though suspect that that'd work just fine as well.