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

Refactor to use Elektra #837

Open
wants to merge 102 commits into
base: 814-improve-config
Choose a base branch
from

Conversation

qwepoizt
Copy link

@qwepoizt qwepoizt commented Sep 1, 2021

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)Refactor to use Elektra #837

Known limitations

  • 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.

Tobias Schubert added 30 commits July 20, 2021 15:53
… which automatically picks a suitable option
Copy link

@markus2330 markus2330 left a 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])

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.

Copy link
Author

@qwepoizt qwepoizt Oct 5, 2021

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

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:

  1. fix the problem with the code generator
  2. always add the generated files (for any platform)

Copy link
Author

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.

@qwepoizt qwepoizt changed the title Improve config/elektrify Refactor Redshift to use Elektra Oct 1, 2021
@qwepoizt qwepoizt changed the title Refactor Redshift to use Elektra Refactor to use Elektra Oct 1, 2021
Copy link

@markus2330 markus2330 left a 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.

@qwepoizt
Copy link
Author

qwepoizt commented Oct 19, 2021

@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!

@zsugabubus
Copy link

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?

@markus2330
Copy link

markus2330 commented Dec 15, 2021

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 😉

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

😆 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?

@qwepoizt
Copy link
Author

@markus2330 Yes, I'll take a look!

@zsugabubus
Copy link

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?

@markus2330
Copy link

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.

@qwepoizt
Copy link
Author

qwepoizt commented Jan 9, 2022

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!

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.

None yet

3 participants