jonls / redshift

Redshift adjusts the color temperature of your screen according to your surroundings. This may help your eyes hurt less if you are working in front of the screen at night.

http://jonls.dk/redshift

GNU General Public License v3.0

5.91k stars 429 forks source link

Improving configuration management (bachelor thesis) #814

Open qwepoizt opened 3 years ago

qwepoizt commented 3 years ago

Dear redshift community and @jonls!

In the course of my CS bachelor thesis (advised by @markus2330), I am working on several features/improvements for redshift's configuration management. I will document my plan and progress in this issue.

Planned features

Add configuration UI to redshift-gtk.
Configuration changes have immediate effect, no restart of redshift required.

These changes will:

improve UX greatly, especially for users with no CLI skills.
fix https://github.com/jonls/redshift/issues/194
fix https://github.com/jonls/redshift/issues/324
render the planned, but never merged, D-Bus integration obsolete: https://github.com/jonls/redshift/issues/54 and https://github.com/dkondor/redshift/tree/dbus_new

Roadmap

Replace redshift's custom config parsing (config-ini.c/h) with libelektra (https://www.libelektra.org and https://github.com/ElektraInitiative/libelektra/).
Add UI controls for configuration to redshift-gtk.
Extend redshift and redshift-gtk to read/write to the config file using libelektra.
Extend redshift to react to changes to the config file during runtime using libelektra.

qwepoizt commented 3 years ago

@jonls has raised several interesting interesting discussion points (thanks!), which I am addressing here:

Merging back to upstream

I would be happy for my changes being merged to upstream, so they are available to all redshift users! (However, this is not a requirement for the success of my thesis.)

Avoiding code that does a lot of experimental stuff to reduce maintenance burden

I agree! Simplifying maintainability is part of my goals (e.g. by reducing project size by replacing custom config parsing with libelektra)

Don't break 1. current usage of signals in redshift and 2. compatiblity with Windows/macOS build

Great remark, I will be mindful of that.
Signals will not be an issue, especially since libelektra will only be used to transfer configuration changes between redshift and redshift-gtk (e.g. location coordinates). Other data transfer (e.g. current color temperature) will not be transfered via libelektra.

Why prefer libelektra over D-Bus?

I've discussed this with @markus2330 who has pointed out that:

libelektra solves some similar problems as D-Bus (e.g. enabling configuration changes without application restart). Additionally, libelektra offers many features that simplify/enable configuration management: e.g.
- config file parsing
- writing and validation
- changing configuration from several backends (e.g. redshift-gtk, CLI, Elektra Web UI).
- for further reading: https://www.libelektra.org/docgettingstarted/why-elektra
libelektra is also available on Windows and macOS (although Windows support is still WIP).

jonls commented 3 years ago

HI @qwepoizt, thanks for working on this. A few quick comments:

I'm not sure I completely understand how this could obsolete the D-Bus work in terms of signals. I would like to get rid of the current signal handling in favor of something like D-Bus signals which would allow the UI to do more things, like turn redshift on/off, manually override temperature, etc. Is there a way that libelektra can provide signaling like that?
I'm assuming from your description that libelektra can synchronize configuration state between redshift and UIs (I haven't had a chance to look deeper into it). Is it also appropriate for synchronizing other application state that is not specifically configuration? in particular I'm thinking something like the current location which may come from geoclue. It's not a thing that's explicitly configured, though the UI will probably want to show it. Or maybe consider something like "current elevation of the sun". It's something that the UI might want to show but should definitely not be modifiable by the user.

markus2330 commented 3 years ago

Yes, exactly: libelektra synchronizes configuration state (also between applications). This could be misused as IPC for any state but there might be functionality where D-Bus is better suited, especially if it is not about configuration at all.

Is it also appropriate for synchronizing other application state that is not specifically configuration?

It is actually a quite often done, e.g. to store window&panel sizes in configuration files. For such values I would definitely not recommend to store them together with the "normal" configuration settings, but instead using a separate mountpoint in Elektra (i.e. a separate file on the disc).

E.g. if we would store redshift config in /sw/redshift/service/#0/current (see also https://www.libelektra.org/tutorials/integration-of-your-c-application about how this path is recommended to look like), the redshift UI-specific config could be in /sw/redshift/ui/#0/current and the more state related config could be in /sw/redshift/state/#0/current.

Probably it would be a good idea to start with designing how the overall config settings should look like? (Elektra's spec language should be suited to do this. :rocket:) Then we would also see if there are left-overs where D-Bus is much better suited.

YuriGor commented 3 years ago

..ah sorry, just found in readme there is no way for wayland, comment removed

qwepoizt commented 3 years ago

Thanks to both of you for your remarks!

I believe there's no harm in having redshift communicate with external applications via D-Bus and with redshift-ui via Elektra. The mentioned benefits of using Elektra for configuration instead of D-Bus still remain.

I appreciate @markus2330 's idea of starting with writing an Elektra specification for redshift's config. I'll put that on my agenda.

qwepoizt commented 3 years ago

Hi!

@jonls Could you please create a branch 814-improve-config in jonls/redshift? I'd then target my PRs to that instead of master. This way, you can later decide if you want to merge improve-config into master.

I have some updates I wanted to share:

Adjusted scope

@markus2330 and I decided that the initial scope was too large for a bachelor's thesis and have narrowed it down accordingly:

Planned features

~~Add configuration UI to redshift-gtk.~~
Configuration changes have immediate effect, no restart of redshift required.

Roadmap

Replace redshift's custom config parsing (config-ini.c/h) with libelektra (https://www.libelektra.org and https://github.com/ElektraInitiative/libelektra/).
~~Add UI controls for configuration to redshift-gtk.~~
Extend redshift ~~and redshift-gtk~~ to read/write to the config file using libelektra.
Extend redshift to react to changes to the config file during runtime using libelektra.

Specification file

I have written up an Elektra specification file for redshift (https://github.com/jonls/redshift/pull/829). CLI arguments are currently missing.

backwards compatiblity: I have decided to break backwards compatibility in favor of a revised hierarchy, naming and typing of the values. That makes the file easier to read, use and less error-prone for human editors. As redshift's config file is quite small it should be easy for users to recreate theirs (based on an example) during an upgrade.

@markus2330 Can you please take a look, if you have any improvement suggestions?

Best regards Tobias

qwepoizt commented 3 years ago

Hi @jonls !

I have finished the specification file and incorporated @markus2330's very helpful review comments.

Feel free to take a look at #829. There is also one open question regarding the gamma settings where your input would be appreciated.

jonls commented 3 years ago

@qwepoizt :+1: I have created the branch 814-improve-config