SafeEyes icon indicating copy to clipboard operation
SafeEyes copied to clipboard

smartpause: use X API instead of xprintidle subprocess

Open keturn opened this issue 4 years ago • 7 comments

The xprintidle utility is a command-line wrapper around an X11 API call. SafeEyes can make the same call itself.

This likely reduces resource usage by avoiding spawning a new subprocess every few seconds, and potentially simplifies installation dependencies.

keturn avatar Feb 23 '20 05:02 keturn

PR in draft mode because there are some things I could yet clean up. Let me know if it looks like the sort of thing you'd be interested in.

keturn avatar Feb 23 '20 05:02 keturn

@keturn it is definitely a good direction to go. I used xprintidle because I didn't know too much about X API. I am looking forward to merge your PR.

Thank you very much for your contribution!

slgobinath avatar Feb 23 '20 16:02 slgobinath

One detail worth mentioning: xprintidle has some extra code in it to work around a bug where the idle time resets when the monitor drops in to power-saving standby mode.

However:

  1. that bug is marked as fixed: https://bugs.freedesktop.org/show_bug.cgi?id=6439
  2. even if that does happen, it might not be such a problem for this use case, because the times smartpause is interested in are shorter than the time it takes the monitor to go to sleep.

Do you think it's okay to skip that workaround code, or do we need to do more testing to see if it's a problem?

keturn avatar Feb 23 '20 17:02 keturn

Another thing worth talking about here is the library choice. I replaced xprintidle with usage of the xcffib library. I think that's a good trade, as it gets rid of the repeated subprocess and it's also available in Debian.

The snag is that makes this package depend on both xcffib and python-xlib (in BreakScreen), which would be a silly thing to do, because they're both X client protocol libraries.

The advantage of python-xlib is that it's pure python, which can be easier to work with and safer from memory errors. The disadvantage is that it's a new implementation and doesn't yet cover the XScreenSaver extension we need.

I think the options are

  1. implement XScreenSaver QueryInfo for python-xlib.
  2. accept this PR's use of xcffib, and then potentially address what to do with BreakScreen in another ticket.

While I like pure-python implementations in theory, I think the approach xcffib uses is pretty good as far as things depending on C go.

  • [x] xcffib?

keturn avatar Feb 23 '20 20:02 keturn

One detail worth mentioning: xprintidle has some extra code in it to work around a bug where the idle time resets when the monitor drops in to power-saving standby mode.

Since the bug is fixed long time ago, I think we don't need that fix. However it is better to test it with few distributions to make sure that we are not affected. In case if we are affected, I prefer to have the fix in place.

Usage of xcffib

I also prefer pure Python based solution but looks like it requires more effort. Therefore I agree with your plan. Lets go with xcffib and later we can consider switching to python-xlib.

slgobinath avatar Feb 24 '20 03:02 slgobinath

Btw, both Stretchly and Workrave already have a similar detection, which, unlike SafeEyes with xprintidle, actually work well for me - maybe there's something you could use from their implementations ;)

xeruf avatar Dec 17 '20 16:12 xeruf

Yep, I just returned to my computer after starting it half an hour ago - and within few minutes was prompted for a long break. Wut?

I really like the idea, but I guess I gotta go back to Stretchly for now.

xeruf avatar Dec 18 '20 09:12 xeruf