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

dunst: improve configurability #2113

Merged
merged 2 commits into from
Aug 8, 2021
Merged

Conversation

ctem
Copy link
Contributor

@ctem ctem commented Jun 16, 2021

Description

This PR adds options to provide workarounds for shortcomings in dunst’s configuration format. All defaults remain unchanged.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

@ctem ctem requested a review from rycee as a code owner June 16, 2021 13:17
modules/services/dunst.nix Outdated Show resolved Hide resolved
modules/services/dunst.nix Outdated Show resolved Hide resolved
@ctem
Copy link
Contributor Author

ctem commented Jun 17, 2021

Thank you for your reviews! The added complexity likely seems unjustified at a glance, so I think this PR warrants a bit more explanation of my use case. The two new options provide me the ability to programmatically concatenate an immutable general configuration (generated by the home-manager module) with a mutable color configuration (generated by a utility like wpgtk). This is trivial for programs with configuration formats that allow importing external configuration files (e.g., Alacritty’s import, Polybar’s include-file, Rofi’s theme, Xresources’ #include paired with i3’s set_from_resource and so on), but poor Dunst was left out in the cold, leaving users to divine their own solutions against all odds. This PR reflects how I addressed the issue with home-manager.

My configuration looks something like this:

# Read (complete, mutable) configuration file generated by script specified in wpgtk configuration
commandOptions = "-config ${config.xdg.configHome}/dunst/dunstrc-combined";

# Write (incomplete, immutable) configuration file to be read by script specified in wpgtk configuration
configPath = "${config.xdg.configHome}/dunst/dunstrc-homeManagerPart";

When wpgtk is called, it generates the color configuration file and runs the script that ultimately produces the combined configuration file.

If all of this sounds obnoxious, you are not wrong. However, I have been using this solution without issue for a number of months and imagine it would be appreciated by the ricing community.

@ncfavier
Copy link
Member

@ctem can't you just extend xdg.configFile."dunst/dunstrc".text (which has type lines) and add the mutable configuration there?

@ctem
Copy link
Contributor Author

ctem commented Jun 19, 2021

@ncfavier Thank you for your suggestion! Unless I'm misunderstanding, this would cause the mutable configuration file to be evaluated, resulting in an immutable concatenated rc file. This implies that a rebuild would be required every time the colors change, which for pywal/wpgtk users means that they would have to rebuild every time they wanted to change their wallpaper.

By contrast, this PR enables the user to specify a mutable concatenated rc file such that regenerating the color scheme triggers an immediate update to the UI.

@ncfavier
Copy link
Member

Indeed.

I'm fine with the commandOptions addition then, but do you really need to change the immutable path from dunstrc/dunstrc? It seems like you can just replace dunstrc-homeManagerPart with dunstrc.

I would actually be more in favour of a configFile option specifying the file to read from (replacing commandOptions, defaulting to ${xdg.configHome}/dunst/dunstrc), and just writing the configuration to dunst/dunstrc. The other command line options are not useful to us anyway.

This seems compatible with your workflow and doesn't add too many options whose usefulness is hard to justify.

@ctem
Copy link
Contributor Author

ctem commented Jun 20, 2021

That’s a nice idea and minimizes the impact on this module. I agree that the PR currently offers more customizability than is needed. I was initially concerned that writing the immutable config to the default path would be misleading if the service was reading a different file, but shipping the new configFile option with a sufficient description should be enough to allay that concern.

I’ll revise the implementation based on this and force push.

modules/services/dunst.nix Outdated Show resolved Hide resolved
modules/services/dunst.nix Outdated Show resolved Hide resolved
modules/services/dunst.nix Outdated Show resolved Hide resolved
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

TBH, I'm not convinced of the utility of this additional option. Doesn't live reloading still work with the existing module after you run home-manager switch. Also, are there other examples of modules doing this?

@ctem
Copy link
Contributor Author

ctem commented Jun 29, 2021

@sumnerevans Thank you for your review!

Doesn't live reloading still work with the existing module after you run home-manager switch.

As previously discussed, building the configuration (e.g., with home-manager switch) results in an immutable configuration file, which by definition precludes live reloading. Currently, if you want a mutable Dunst rc, you must forgo use of this module. The goal of this PR is to provide the best of both worlds.

are there other examples of modules doing this?

As I previously explained, most applicable programs do not require such a workaround because their configuration formats are flexible enough to pull in additional files. Indeed, it is my hope that there are not many other examples of this!

@ncfavier
Copy link
Member

Live reloading is done by this module on activation, using an onChange hook.

As far as I understand, this is mostly an issue of timing: live reloading with wpgtk is near instant, whereas live reloading with home-manager requires evaluating, building and activating the whole configuration.

@ctem
Copy link
Contributor Author

ctem commented Jul 1, 2021

This is correct. @ncfavier Thank you for clarifying; it appears I misinterpreted @sumnerevans’ use of the term.

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

The change is simple and costs us nothing to maintain.

modules/services/dunst.nix Outdated Show resolved Hide resolved
modules/services/dunst.nix Outdated Show resolved Hide resolved
@ctem ctem force-pushed the feature/dunst branch from 1542ea1 to 907eac3 Compare July 6, 2021 18:19
@ctem
Copy link
Contributor Author

ctem commented Jul 6, 2021

@berbiche Thank you for returning to this PR. I have updated with your changes.

@ctem
Copy link
Contributor Author

ctem commented Jul 21, 2021

This is my first contribution to this repo, so I'm not entirely sure what sort of turnaround to expect. In this situation, is it appropriate to request reviews from other maintainers, or should I continue to await further action? Thanks!

@sumnerevans sumnerevans requested a review from berbiche July 21, 2021 14:40
@ctem
Copy link
Contributor Author

ctem commented Aug 4, 2021

@sumnerevans Thank you kindly for your prompt assistance. As @berbiche is unresponsive, is there perhaps another fine maintainer willing to give this small PR attention?

@berbiche berbiche merged commit 59be1f4 into nix-community:master Aug 8, 2021
@berbiche
Copy link
Member

berbiche commented Aug 8, 2021

As @berbiche is unresponsive, is there perhaps another fine maintainer willing to give this small PR attention?

Sorry, I am currently in my finals week in uni.

@ctem
Copy link
Contributor Author

ctem commented Aug 8, 2021

@berbiche No worries. Thank you and best of luck with your finals!

@ctem ctem deleted the feature/dunst branch August 8, 2021 17:43
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.

5 participants