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

Add config file and implement config file parsing #79

Merged
merged 3 commits into from
Aug 25, 2024

Conversation

Ferdi265
Copy link
Contributor

@Ferdi265 Ferdi265 commented Feb 18, 2024

This PR adds a TOML config file for the server, client, and libinput backend.

There are two config files

  • <SYSTEM_CONFIG_DIRS>/swayosd/backend.toml for the backend components that run with elevated privileges. This file should never be read from a user configuration folder.
  • <SYSTEM_CONFIG_DIRS>/swayosd/config.toml or <USER_CONFIG_DIR>/swayosd/config.toml for the server and client that run as the user. This file should be read from the user configuration folder if it exists and fall back to the system configuration directories otherwise

The config files are structured like this:

Backend configuration

[input]
# libinput backend configuration options (none so far)

# ... sections for future backend components that need elevated privileges ...
# [foo]
# bar = "baz"

Server and Client configuration

[server]
style = "/path/to/my/style.css"
top_margin = 0.85
# show_percentage = true (once #69 is merged)

[client]
# client configuration options (none so far)

Implementation Design

The parsing uses serde and toml to parse the TOML file directly into a struct.

Sections and options in the config file are optional, but misspelling them is an error.

Fixes #42.

@ErikReider
Copy link
Owner

Thanks a ton for working on this! I'll have time to review early next week. If I forget, don't be afraid to ping me here :)

@Ferdi265
Copy link
Contributor Author

Any updates on this?

I understand if you don't have much time due to other commitments, but just wanted to check in.

@ErikReider
Copy link
Owner

Any updates on this?

I understand if you don't have much time due to other commitments, but just wanted to check in.

I'll get to this next week after my finals :)

@Ferdi265
Copy link
Contributor Author

Good luck on your finals then!

Copy link
Owner

@ErikReider ErikReider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than the single comment. The implementation is really clean and works like a charm!

Thanks for your much appreciated patience! :)

@@ -30,6 +30,11 @@ pub fn get_proxy() -> zbus::Result<ServerProxyBlocking<'static>> {
}

fn main() -> Result<(), glib::Error> {
// Parse Config
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some config dir information in the client, server, and backends --help print information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea.

Though, I can't find a way to add text to --help without adding another option with gtk-rs. 😅

I think one way to workaround that would be to add a --config option that allows specifying a custom config path, and explain the config stuff in the help text for that option.

I'll look at this at some point in the next few days, when I have a bit more time.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea.

Though, I can't find a way to add text to --help without adding another option with gtk-rs. 😅

I think one way to workaround that would be to add a --config option that allows specifying a custom config path, and explain the config stuff in the help text for that option.

I'll look at this at some point in the next few days, when I have a bit more time.

Oh yeah, true. Sounds like a good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the --config option today. Sorry it took so long. The libinput backend doesn't have any command line parsing at the moment, so I left it out there, but I can add similar parsing to it as with client/server.

The help message isn't final yet, I still need to think about how to best communicate the location without hardcoding paths or making the message too long.

Let me know if you like the basic implementation outline or if you'd like it done differently.

@Ferdi265
Copy link
Contributor Author

Ferdi265 commented May 6, 2024

(fixed rustfmt difference in latest push)

@CelticBoozer
Copy link

Hi, any updates on this PR?

Copy link
Owner

@ErikReider ErikReider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Again, thanks for your immense patience :)

@ErikReider ErikReider merged commit d304967 into ErikReider:main Aug 25, 2024
1 check passed
@ErikReider
Copy link
Owner

The next step would be to clean out some of the cmdline args and move them into the config

@Ferdi265 Ferdi265 deleted the config-parsing branch August 25, 2024 15:02
@Ferdi265 Ferdi265 restored the config-parsing branch August 25, 2024 15:02
@Ferdi265 Ferdi265 deleted the config-parsing branch August 25, 2024 15:24
@Ferdi265
Copy link
Contributor Author

The next step would be to clean out some of the cmdline args and move them into the config

--top-margin, --style, and max volume already have config settings, so those options could potentially be removed, but some people might have scripts that depend on them.

The "show percentage" PR could also remove its CLI option if you want to move away from settings via CLI options.

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.

Config
3 participants