muniter / halinuxcompanion

HomeAssistant Linux Companion
MIT License
54 stars 6 forks source link

also listen for org.gnome.ScreenSaver to update 'idle' #14

Closed evgeni closed 1 year ago

evgeni commented 1 year ago

org.freedesktop.ScreenSaver.ActiveChanged seems to be a KDE invention[1], while org.gnome.ScreenSaver.ActiveChanged is the API used by other implementations [2].

Tested on Fedora 37 with Gnome 43.2 on Wayland

[1] https://people.freedesktop.org/~hadess/idle-inhibition-spec/re01.html [2] https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/data/dbus-interfaces/org.gnome.ScreenSaver.xml

evgeni commented 1 year ago

cc @oranja

oranja commented 1 year ago

@evgeni, thanks for adding Gnome support :)

The freedesktop project is not exactly a KDE invention. It is a collection of specs and libraries that are supposed to enable interoperability between desktop environments (such as KDE and Gnome). I have both org.freedesktop.ScreenSaver and org.kde.screensaver on my machine, but I preferred the freedesktop service over the KDE-specific one and hoped that it would work out-of-the-box for modern Gnome-based desktops too.

So first, does the current implementation not work for you on Gnome? Is it not reporting any events? (I tested by locking and unlocking the session and saw that the signal was activated) or is halinuxcompanion not even starting because there is no such service?

Second, I don't think @muniter would mind if there are multiple potential DBus services listed for getting a hook on some events, as long as: (a) they don't overwrite each other and (b) halinuxcompanion doesn't fail to start when one of the options is not available. In testing this PR on my KDE desktop I ran into the second problem. halinuxcompanion doesn't start, because there is no org.gnome.ScreenSaver service available here (getting: dbus_next.errors.DBusError: The name is not activatable).

If you could update this PR to address this issue that would be awesome, or we can discuss possible solutions here.

evgeni commented 1 year ago

Oh I didn't mean to say the freedesktop project is an KDE invention. Just the addition of the "ActiveChanged" signal on the org.freedesktop.screensaver to be one (there is no such signal defined in the official spec at https://people.freedesktop.org/~hadess/idle-inhibition-spec/re01.html)

The app starts just fine for me without the patch, but there is no event fired when I lock/unlock the screen.

I'll look if I can make the gnome stuff optional.

muniter commented 1 year ago

Hello! Thanks for the contribution. Indeed wouldn't mind if a sensor listens to multiple signals, as long as it doesn't misbehave.

evgeni commented 1 year ago

I've updated the code not to fail if a certain DBus interface could not be found. Not sure about the exact logging (levels) of the "failures", but I guess it's a start? :)

What do y'all think?

oranja commented 1 year ago

btw, relevant Gnome issue: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/722

muniter commented 1 year ago

Looks good! Can't really test on my setup since I'm out of town for a while. But let me know when you think is ready. And @oranja opinions and I'll merge.

oranja commented 1 year ago

I'll also be away from home until next week, and won't be able to test, but the code changes look good & safe to me.

evgeni commented 1 year ago

I've been running the code since I've opened the PR and it's been updating the state as expected.

What I didn't test:

So I would consider the PR ready for merging

muniter commented 1 year ago

Thank you both! LGTM