Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logind fallback support for unprivileged users #83

Merged
merged 10 commits into from
Aug 6, 2023

Conversation

geekylthyosaur
Copy link
Contributor

@geekylthyosaur geekylthyosaur commented Jul 29, 2023

elogind or systemd-logind provide a safe interface for unprivileged users to control device's brightness through dbus. So I implemented support for setting backlight brightness when user is neither running program as root nor part of video group.

Unfortunately I've never used wluma myself and currently I am not able to. So this changes is not tested. But this exact code works as expected in my own brightness control utility and I think support of logind fallback will be nice addition to wluma features.

More info

@maximbaz
Copy link
Owner

Wow, very interesting, thanks for sharing! What do you think about swapping the priorities, and first try dbus, and only then fallback to write to files, is it a bad idea? On the one hand, it sounds better to promote feature that needs less permissions, on the other hand, is it perhaps noticeably slower to change brightness via dbus, or perhaps it's hard to see via code whether it worked, whether we need to fallback to file?

And thanks for sharing a link to slight, I was just recently thinking that I should find an alternative to the now abandoned light 😁

@geekylthyosaur
Copy link
Contributor Author

I've modified this code a bit and, having limited ability to test it on my own machine, found that before these changes it was about 30x slower than writing to a file, now it's at least as fast, and possibly 2-3x faster.
Also I've made dbus method the default and writing to file as fallback.
However, now it's really hard to tell if the brightness is changed because we're just sending the message and not even checking the reply.
I'll do some research and see if there are quick ways to check the reply.

@maximbaz
Copy link
Owner

Okay, thanks! And just to clarify, I'm not opposed to use file first, checking for write assess like you also do in slight, I'm just curious to learn what pros and cons there are; in fact @cyrinux was telling me in another chat that trying file first is a better approach 🙃

@geekylthyosaur
Copy link
Contributor Author

Sorry for misunderstanding.

I think if the user already has permissions, it's really better to set the brightness via a file, since the dbus method is either slower (however, it just waits about 3ms per call for a reply to make sure it's successful) or it's faster, but we don't know if it succeeded.

Also, there is no easy way to check for write permission (I just found out that my method from slight returns false positive). The best way I could think of is to try to write the current brightness.

@maximbaz
Copy link
Owner

I agree with you on everything, let's do as you propose, using file as the first option, using the check of trying to write the current brightness, and using the slower dbus that tells us about whether it succeeded or failed. Will get it tested, and merged afterwards. Huge thanks once more, not only for the idea, but especially for an actual PR!

@geekylthyosaur
Copy link
Contributor Author

All done, Thanks!

@maximbaz
Copy link
Owner

I hope you don't mind working directly into your PR 😉

The most interesting result of a test so far is that with external displays, the time we need to wait for a dbus call response is ~10 times larger than the proposed 100ms 😐 That's why I returned back to your earlier commit of just using a non-blocking send, I don't want to deal with it...

Otherwise, since we are anyway checking for file permissions only once, I made it skip dbus initialization entirely, when we know it won't be used.

@maximbaz maximbaz merged commit 55d98fe into maximbaz:main Aug 6, 2023
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.

2 participants