maximbaz / wluma

Automatic brightness adjustment based on screen contents and ALS
ISC License
625 stars 26 forks source link

Possibily modify target of wluma.service #80

Closed hvitoi closed 1 year ago

hvitoi commented 1 year ago

Please describe your feature request

The unit file wluma.service relies on graphical-session.target in order to get started at boot. However, not all display managers start this target, for instance greetd.

➜  ~ systemctl list-units --user --type=target
  UNIT             LOAD   ACTIVE SUB    DESCRIPTION     
  basic.target     loaded active active Basic System
  bluetooth.target loaded active active Bluetooth
  default.target   loaded active active Main User Target
  paths.target     loaded active active Paths
  sockets.target   loaded active active Sockets
  timers.target    loaded active active Timers

Which causes wluma service not to start at all and remain in an inactive (dead) state. Changing the target to default.target does the job and it is started at boot as expected (and benefits from the auto-restart on crash)

[Unit]
Description=Adjusting screen brightness based on screen contents and amount of ambient light
PartOf=default.target
After=default.target

[Service]
ExecStart=/usr/bin/wluma
Restart=always
EnvironmentFile=-%E/wluma/service.conf
PrivateNetwork=true

[Install]
WantedBy=default.target

I guess some open questions would be:

  1. Would it be fine commit these changes? Would there be any other side effect from starting the wluma service before the compositor has started?
  2. Does wluma actually requires a wlroots compositor to be up and running in order to adjust the screen brightness or would it be something optional (to also adjust brightness based on the screen content)? A use case would be in ttys (/dev/tty1) without a DE running, adjusting the brightness based on the light sensor only would already be a great benefit.
maximbaz commented 1 year ago

Hello!

Thanks for bringing this topic up!

First of all, you are totally right in saying that not all display or window managers start graphical-session.target. A typical reason for this that I've seen is that systemd is just one of many init systems, and it doesn't feel right to hardcode systemd-specific things into a display or window manager that does not depend on systemd specifically in any way. Heck, I couldn't even get sway to put a 6-line file in the contrib directory, that would help people start the graphical-session.target! Today I use Hyprland, and it is the same story. Believe me, I am very familiar with the problem :grin:

Having said that, in my mind, not starting the graphical-session.target on a system that does use systemd is a violation of a contract, established when you agreed to use systemd. It's a way to manage dependencies, and for any GUI app (not just wluma) that wants to only start on a graphical system, depending on graphical-session.target is the right thing to do.

That is to say, when you and I use systemd, and a display/window manager that doesn't start graphical-session.target, it is our responsibility to do it. It's not actually that hard, you need to start the target when DM or WM launches (example) and stop it when it exits (example). This will help with all other GUI apps that you start with systemd (common example could be a bar like waybar), and would bring all the same benefits you saw when you changed to default.target (stop on crash, auto-restart, etc).

You will see in the links I provided, that we create <app>-session.target (sway-session.target / hyprland-session.target), that binds to graphical-session.target - this is optional but recommended, in case you ever have a systemd dependency that can only run in your particular WM and not just any display session.

Now, does wluma actually require a wlroots compositor to be up and running on launch? Yes, to take screen contents into consideration when picking the best brightness value to set. While this feature can be disabled in config, it is enabled by default, and I consider it a key differentiator of this particular app compared to anything else on the market, that can at most take ALS into account. And when you have the feature enabled, wluma will actually crash on startup, as it fails to find a display session, so it will crash in the default config, and it will crash for the majority of people.

I don't disagree with the benefits of using wluma on a TTY, or on a non-wlroots display manager for that matter - it was the whole reason to make the feature toggleable! It's just that the config is static today, and you can't switch to TTY mode without also adjusting the default config (and so the default systemd service unit).

So, would the default wluma.service benefit from switching to base.target? Not at the moment. If we can make the setting dynamic, such that wluma can start on TTY and use only ALS, later detect that wlroots-based display manager was started and begin to take screen contents into account, and similarly switch back and continue working when display session was closed, then it would totally make sense to define only the minimally necessary dependency in the systemd unit! Until then, I believe the default unit should continue to match the default config.

It was a wonderful question, thanks again for asking, I hope I managed to give you at least somewhat of an answer than it deserves! Don't hesitate to ask if you have further comments :wink:

CorvetteCole commented 1 year ago

Just want to chime in and say I agree with @maximbaz , particularly with regards to describing graphical-session.target as a contract

hvitoi commented 1 year ago

That makes total sense, thanks for your detailed response. And thanks for the reference to the dotfiles.

Unfortunately for wlroots compositors and display managers it's not very clear that this target must be handled by the user itself (creation and startup).

I even tested it with sddm thinking that it would automagically start up this target but also not. So this "issue" might something common for wluma users.

maximbaz commented 1 year ago

If you think it's valuable I'd be happy to accept a PR adding a short note about this caveat in README, but since it's not really specific to wluma, but to all GUI apps that you want to start with systemd (e.g. waybar), perhaps it would be more beneficial to document it on a place like arch wiki for your desktop or window manager.

maximbaz commented 1 year ago

Closing as answered and non-actionable, but any further comments, questions or PRs for README improvements are always welcome :slightly_smiling_face: