-
Notifications
You must be signed in to change notification settings - Fork 431
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
Refactor to use Elektra #837
base: 814-improve-config
Are you sure you want to change the base?
Refactor to use Elektra #837
Conversation
…p. vidmode/screen.
…placed by my own work and feedback from @markus2330.
… which automatically picks a suitable option
…ause they are handled by Elektra
…e-config/elektrify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions about the windows build.
@@ -32,6 +32,7 @@ esac | |||
AC_CHECK_TOOL([WINDRES], [windres], []) | |||
AS_IF([test "x$build_windows" = "xyes" -a -n "x$WINDRES"], [ | |||
enable_windows_resource=yes | |||
AC_DEFINE([WINDOWS_BUILD], [1], [Add a DEFINE that this is a Windows build]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would surprise me very much if autotools do not already have a solution for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thank you!
I haven't found a more elegant way to accomplish this and I found this solution to have a big benefit: WINDOWS_BUILD
will be defined if and only if the rest of the build will also be made Windows-compatible.
E.g. a solution like https://stackoverflow.com/a/38899152 would have most of the build depend on AC_CHECK_TOOL([WINDRES], [windres], [])
while WINDOWS_BUILD
would only be defined if ${host_os}
matches mingw*
.
@@ -0,0 +1,754 @@ | |||
// clang-format off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a problem with the code generator under windows? Then it is probably better to either:
- fix the problem with the code generator
- always add the generated files (for any platform)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint! I have implemented 2. and modified the Makefile and CONTRIBUTING.md so that developers are aware they need to manually trigger the generation on changes of the specification files. I will open an issue in Elektra's repo for compatiblity of the code generator with Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to see what others are saying.
@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?) 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!
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.)
https://www.libelektra.org/ explains this 😉
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.,
😆 Yes, very funny metric. Sounds like interpretation of sign of the zodiac based on the first commit.
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.
@qwepoizt can you maybe answer these questions? |
@markus2330 Yes, I'll take a look! |
Yes, that was it. Sorry. But installing code generator for developers is that painful? (Will it be easier for users to install their part?)
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:
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.
Some random thoughts:
Of course, I may be totally wrong. After all, advantages really outweigh disadvantages in case of Redshift? |
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.
No, most plugins, e.g. TOML and hosts, do great effort to keep comments. There are some plugins that discard comments, e.g. JSON.
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.
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
As said, in Elektra you can still edit config files.
The Web-UI gives you buttons according to the type in the specification, e.g., a boolean will give you a checkbox.
What do you mean?
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.
Absolutely.
Elektra supports this perfectly. |
Sorry for the delay, here is my answer:
Thanks for the question! This PR has a limitation regarding the I am not sure how I've updated the top post! |
This PR refactors Redshift to use Elektra for configuration management.
Summary of changes
Known limitations
randr
method of adjusting color temperature: It only affects onecrtc
. This could be fixed rather easily by changing the implementation of src/gamma-randr.c.