jonls
Refactor to use Elektra
This PR refactors Redshift to use Elektra for configuration management.
Summary of changes
- Removed Redshift's code for parsing configuration file
- Removed Redshift's code for parsing CLI options
- Configured build process to include Elektra
- Added code to load configuration settings from Elektra (from CLI options or configuration file)#837
Known limitations
- This PR has a limitation regarding the
randrmethod of adjusting color temperature: It only affects onecrtc. This could be fixed rather easily by changing the implementation of src/gamma-randr.c.
@markus2330: As we just discussed, I have removed the files generated by Elektra from Git and included them in the autotools configuration.
The PR is now ready for review.
I'd especially appreciate a review of the file src/elektra/redshift.ini (I propose to use git diff 034b2cf2..09dc2841 src/elektra/redshift.ini because diffing against master on Github is not helpful, as improve-config/specification was not yet merged).
Since you are an expert on specification, I'd be interested in your opinion on the recent changes there (e.g. name and place in hierarchy of new keys)!
@markus2330 Thanks for your elaborate review, I'll work through your remarks!
@markus2330 I've incorporated all your remarks! IMO, after fixing https://github.com/ElektraInitiative/libelektra/issues/4041 this is ready to be merged into jonls/814-improve-config as a first version of elektrified redshift with the stated limitations.
@qwepoizt It would be important that the build server passes. Before that we cannot really ask people to have a closer look.
@jonls I have finished refactoring Redshift to use Elektra (see PR description at top).
To test, you can check out the source branch of this PR, install the libelektra dependency and follow the updated instructions in CONTRIBUTING.md .
I am looking forwarding to your thoughts and feedback!
I'm just a nobody from the Internet but does this added 4000 LOC + (random?) dependency really worth? Currently the whole C source code is about 8000 LOC. Eh? That is 50% increase for what? Not speaking about the headache for thousands of users caused by throwing out simple, plain text config files and CLI. Project had no commits for more than a year. Return from the ashes but with style. Yeah.
Elektra has more unresolved issues that stars. Not sure how good metric it is but is this really better than editing plain text config files once in a lifetime?
Otherwise I am not clear what is the issue around GTK interface, if any, but it cannot be that bad...
How does this handle multiple $DISPLAYs?
Am I insane?
Thank you for bringing some life in this PR!
I'm just a nobody from the Internet but does this added 4000 LOC + (random?) dependency really worth? Currently the whole C source code is about 8000 LOC. Eh? That is 50% increase
Which 4000 LOC are you talking about? This PR decreases the LOC of redshift. It adds some documentation and two generated files, maybe this confused you? (Then people do not need to have the code generator installed to compile Redshift.)
for what?
https://www.libelektra.org/ explains this :wink:
Not speaking about the headache for thousands of users caused by throwing out simple, plain text config files and CLI.
Please do not report misinformation. Elektra also uses plain text file configs and command-line arguments.
Where it differs: It adds additional ways to change and specify configuration settings, that were not present in redshift before, e.g., kdb set user:/sw/redshift/#0/current/mode continual allows you to change the mode in the config file.
Project had no commits for more than a year. Return from the ashes but with style. Yeah. Elektra has more unresolved issues that stars. Not sure how good metric it is
:laughing: Yes, very funny metric. Sounds like interpretation of sign of the zodiac based on the first commit.
but is this really better than editing plain text config files once in a lifetime?
Again: please do not report misinformation. Elektra does not interfere with this habbit, you can continue editing plain text config files. For scripts or configuration management tools, however, editing plain text config files is a major pain, that is why Elektra exists.
Otherwise I am not clear what is the issue around GTK interface [...] How does this handle multiple $DISPLAYs?
@qwepoizt can you maybe answer these questions?
It adds some documentation and two generated files, maybe this confused you? (Then people do not need to have the code generator installed to compile Redshift.)
Yes, that was it. Sorry. But installing code generator for developers is that painful? (Will it be easier for users to install their part?)
Please do not report misinformation. Elektra also uses plain text file configs and command-line arguments.
Sorry again, I had misunderstanding here, as well. So it modifies configuration files directly (if I understand it correctly right now). Then it means I can forget about comments and such?
Anyway, maybe this comment from #814 that made me think it will cause headache for some people:
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.
Though I still fail to see why in case of Redshift, where I have a <15 line simple, static, plain-text configuration requires an external program to manage it.
Reloading? SIGHUP, if really needed.
Please absolutely do not take it as an offend but Electra currently has 21 issue identified as bug. Some of it is from years ago. When they will be cleaned up? Probably not all of them are critical, okay, but for example "silently deletes child keys" sounds a bit terrifying. I imagine a configuration handling system should be robust. After it, it will not be enough that I have an issue with a program, now I may have to fight with the program I use to configure it.
A new moving part. So many things can go wrong.
For scripts or configuration management tools, however, editing plain text config files is a major pain, that is why Elektra exists.
Some random thoughts:
- You struggled editing a configuration file but now this new layer of abstraction makes everything magically crystal clear.
- Instead of file paths you get virtual paths to remember and custom commands to operate on them.
- Instead of text editor you get a CLI with lot's of typing boilerplate with shells**t (sorry, escaping) OR a visual interface, where I guess you get a textbox aka. a single-line stupid "text-editor".
- The unformatted description barely satisfying for the more complex cases.
- Is it expected that a user who is unable to open a text file, will "hack" inside a terminal? Providing a convenient interface for non-programmers probably requires more complex GUI elements and layouts. Otherwise a long text file with lot's of comments with one-one config line in between could serve just as well.
- Me, who does a little programming, I prefer to see the complete config file in whole, opened next to the docs and reload the program only after I have finished editing. But it's just me.
Of course, I may be totally wrong.
After all, advantages really outweigh disadvantages in case of Redshift?
Yes, that was it. Sorry. But installing code generator for developers is that painful? (Will it be easier for users to install their part?)
It normally shouldn't be a problem because by default the code generator gets installed with Elektra. Iirc the reason the generated files are checked in is only because of a problem in building Redshift in Windows.
Then it means I can forget about comments and such?
No, most plugins, e.g. TOML and hosts, do great effort to keep comments. There are some plugins that discard comments, e.g. JSON.
Though I still fail to see why in case of Redshift, where I have a <15 line simple, static, plain-text configuration requires an external program to manage it.
The need has little to do with the size of the configuration settings but if there is a reason that external applications might want to change the configuration. I think in Redshift there is a need to change configuration programmatically, e.g., when you change the location.
Please absolutely do not take it as an offend but Electra currently has 21 issue identified as bug. Some of it is from years ago. When they will be cleaned up? Probably not all of them are critical, okay, but for example "silently deletes child keys" sounds a bit terrifying. I imagine a configuration handling system should be robust. After it, it will not be enough that I have an issue with a program, now I may have to fight with the program I use to configure it.
I cannot argue with that: Elektra is not yet 1.0. We currently focus on getting the API perfect, bugs in tooling unfortunately cannot get too much of our attention. So yes of course early adapters need to struggle with a few annoyances. If you actually hit the kdb mv bug, please ping the issue and it will get our attention again. I never hit it, as doing kdb mv is something rather rare.
Instead of file paths you get virtual paths to remember and custom commands to operate on them.
As said, in Elektra you can still edit config files. kdb file gives you the path where the config file is, completely bypassing Elektra. As this is dangerous (you might destroy the configuration and Redshift might not be able to start), Elektra also supports kdb editor which starts up your favorite editor with your favorite configuration file format and will only take over configuration iff the config is syntactically and semantically okay (semantics as far as specified in the specification, see src/elektra/redshift.ni in this PR), like with visudo.
Instead of text editor you get a CLI with lot's of typing boilerplate with shells**t (sorry, escaping) OR a visual interface, where I guess you get a textbox aka. a single-line stupid "text-editor".
The Web-UI gives you buttons according to the type in the specification, e.g., a boolean will give you a checkbox.
The unformatted description barely satisfying for the more complex cases.
What do you mean?
Is it expected that a user who is unable to open a text file, will "hack" inside a terminal?
No. Elektra gives you many different UIs, users can choose from them in the same way as SANE doesn't expect users to use xsane.
Providing a convenient interface for non-programmers probably requires more complex GUI elements and layouts.
Absolutely.
Me, who does a little programming, I prefer to see the complete config file in whole, opened next to the docs and reload the program only after I have finished editing.
Elektra supports this perfectly.
Sorry for the delay, here is my answer:
How does this handle multiple $DISPLAYs?
Thanks for the question! This PR has a limitation regarding the randr method of adjusting color temperature: It only affects one crtc. This could be fixed rather easily by changing the implementation of src/gamma-randr.c.
I am not sure how $DISPLAY, screen, crtc, output are related exactly. #23, #26, #183, #242 might be helpful in finding out which of these terms refer to what or to the same concept.
I've updated the top post!