smartpause: use X API instead of xprintidle subprocess #358
Conversation
|
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 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! |
|
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:
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? |
|
Another thing worth talking about here is the library choice. I replaced 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
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.
|
| return None | ||
| if not Utility.command_exist(command): | ||
| return _("Please install the command-line tool '%s'") % command | ||
|
|
There was a problem hiding this comment.
Should we try to make the xcffib dependency optional, by putting an import check here?
Or keep it simple and not have maybe-dependencies for bundled components?
There was a problem hiding this comment.
Can Smart Pause plugin work without the xcffib library? If it cannot please add a check here. This check will only disable the plugin (not Safe Eyes) if the dependency is missing.
because they may require their own resources
which is why we refactored these to objects.
trying to make it clearer that this is part of the event system and not a configuration parameter or something.
| __set_active(True) | ||
|
|
||
| # FIXME: need to make sure that this gets updated if the waiting_time config changes | ||
| _timer_event_id = GLib.timeout_add_seconds(waiting_time, __idle_monitor) |
There was a problem hiding this comment.
From reading init(), it looks like waiting_time is almost always 2, except in the cases where plugin_config.idle_time is less than two seconds.
Is there still a use case for that requirement? I'm having trouble imagining what that might be for.
There was a problem hiding this comment.
Users have the option to specify the minimum idle time to pause Safe Eyes (by default 5 sec). Executing xprintidle every 5 seconds may pause after seconds. That's why I made it less than seconds which is hard coded to 2. However if the user reduces the minimum idle time to 1 sec, waiting_time also will be 1 sec.
Waiting time is used to add a delay to the while loop so that it doesn't eat the CPU. If your solution does not need busy waiting, you can skip this.
There was a problem hiding this comment.
It does still need a waiting_time. The part I am unclear on is this
However if the user reduces the minimum idle time to 1 sec, waiting_time also will be 1 sec.
What sort of user is it that wants a minimum idle time of only one second? What kind of workflow do they have? How would they be troubled if their waiting_time never went below two seconds?
It must be very different than how I have considered using this, because I've set my minimum idle_time much higher than that.
There was a problem hiding this comment.
I do not have any specific user requirements. Personally I set it to 5 seconds but in I didn't want to deal with issues complaining people about smart pause not respecting/not allowing a minimal waiting time. That's why I try my best to keep all settings configurable and let people to choose.
However your argument makes sense and we can set the minimum bound to be 5. Just checked the config.json it is already set to a minimum of 5. So we can go with a minimum of 5 seconds idle time and a waiting time slightly less than that.
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.
I also prefer pure Python based solution but looks like it requires more effort. Therefore I agree with your plan. Lets go with |
|
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 ;) |
|
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. |
The
xprintidleutility 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.