Settings: Be more compliant with XDG BD spec. #917

WhitePeter · 2021-09-21T20:16:36Z

Rollback some of the changes in commit 694c596 to be compliant with the
Base Directory Spec, when we can be, and provide an override. By default
there will be no fallback value set, if and only if XDG_CONFIG_DIRS is
unset. This is a deliberate digression from the spec. If one sets
XDG_CONFIG_DIRS to any value, including an empty string, dunst behaves
like the spec mandates, with all side effects that entails. This is
mostly only relevant if one installs to a local prefix, i.e. /usr/local,
or, more precisely, if the system-wide dunstrc is expected to be in a
different location than the spec default, i.e. /usr/local/etc/xdg.
Distro packages should not be affected by this, because dunstrc will be
expected in /etc/xdg, anyway, which happens to be the default if
XDG_CONFIG_DIRS is unset or empty.

See the commit and PR messages attached to above commit for more info.

Also renamed the variable systemdirs to xdgconfdirs, because those need
not necessarily be system-wide, since the user can also choose to use an
additional hierarchy in their $HOME.

Plus some minor cosmetics.

fwsmit · 2021-09-22T07:42:23Z

I realized that we forgot to update the man page. Could you write something for the man page as well?

WhitePeter · 2021-09-22T09:45:20Z

I realized that we forgot to update the man page. Could you write something for the man page as well?

I'm on it.

WhitePeter · 2021-09-22T11:10:31Z

I realized that we forgot to update the man page. Could you write something for the man page as well?

I'm on it.

This might take some time, because I want to get ${SYSCONFDIR} into the man page in a flexible way, so it gets updated to the correct location at build time. It looks like pod2man is building the man pages. Is doxygen in any way also involved in that--doesn't look that way at first glance?
Since I know virtually nothing about both pod2man and doxygen, can you give me some pointers to get a head start?

Currently I am thinking of moving docs/dunst.1.pod to 'docs/dunst.1.pod.in' and preprocessing it with something similar to the $(SED) scripts for the *.service files in Makefile. But that seems hackish to me, since doxygen provides expansion of parameters--I have snooped in the source code for the unbound DNS resolver, which makes use of that quite heavily. I couldn't find anything yet on how to do that with pod2man, though. Any hint is very welcome.

fwsmit · 2021-09-22T14:03:27Z

I realized that we forgot to update the man page. Could you write something for the man page as well?

I'm on it.

This might take some time, because I want to get ${SYSCONFDIR} into the man page in a flexible way, so it gets updated to the correct location at build time. It looks like pod2man is building the man pages. Is doxygen in any way also involved in that--doesn't look that way at first glance?
Since I know virtually nothing about both pod2man and doxygen, can you give me some pointers to get a head start?

Currently I am thinking of moving docs/dunst.1.pod to 'docs/dunst.1.pod.in' and preprocessing it with something similar to the $(SED) scripts for the *.service files in Makefile. But that seems hackish to me, since doxygen provides expansion of parameters--I have snooped in the source code for the unbound DNS resolver, which makes use of that quite heavily. I couldn't find anything yet on how to do that with pod2man, though. Any hint is very welcome.

Yeah try to not create an intermediate file. You could probably pipe the file to grep and pipe it to pod2man.

Just a note on this PR: I still haven't decided if this is settings lookup scheme is the best, so you might need to change some things.

WhitePeter · 2021-09-22T14:38:39Z

Yeah try to not create an intermediate file. You could probably pipe the file to grep and pipe it to pod2man.

I will look into that.

Just a note on this PR: I still haven't decided if this is settings lookup scheme is the best, so you might need to change some things.

All right, let me know. Basically it is almost the same as prior to #910. The only change is the override for the default of g_get_system_config_dirs(). But, since I just found out about the internal documentation, I am thinking of ditching that function altogether and build an array from the concatenation of $XDG_CONFIG_HOME:$XDG_CONFIG_DIRS:$SYSCONFDIR.

That way there is full control of the default and it should pave the way to not stopping at the first found file. My midterm goal is to take all files into account and walk through said array from least important to most important, overwriting values as they come. That is essentially what shells do when they initialize themselves and also what the BD spec seems to expect as well, since it talks about "information" instead of files.

WhitePeter · 2021-09-26T14:13:22Z

I was forced offline for a while, because of an outage in my area, so I got bored and went ahead to achieve said 'midterm goal'. This works as expected on my end. Now one only needs to put settings in the more important config files that are actually different from those in less important ones or the internal defaults. In my limited preliminary tests this works quite nicely, i.e. change a setting in ${SYSCONFDIR}/dunst/dunstrc, uncomment out the same setting in ${XDG_CONFIG_HOME}/dunst/dunstrc and you should see that the former setting gets used. But, since I mostly stick to the default config anyway (no need for bells and whistles) I have only checked some random changes and it worked.

Have a look at apply_settings() which I split out to overwrite settings over and over. From my limited understanding of the code it should just work.

P.S.: Curiously, compilation never failed and there were no warnings about delimiter being unused in string_to_array().

fwsmit

This PR looks good. The use of GQueue seems like a good solution. I don't think it works as intended in it's current state. Specifically, I think only the changes from the last file will actually end up in the settings struct.
I think this PR will also be a good step towards #796

src/utils.c

docs/dunst.1.pod

src/settings.c

README.md

WhitePeter · 2021-09-29T17:54:03Z

I don't think it works as intended in it's current state. Specifically, I think only the changes from the last file will actually end up in the settings struct.

It looked differently in my tests, if you can call them that. I have /usr/local/etc/xdg/dunst/dunstrc, which differs from the shipped one on only one line:

--- dunstrc     2021-09-26 23:32:21.687693760 +0200
+++ /usr/local/etc/xdg/dunst/dunstrc    2021-09-23 15:24:58.343225816 +0200
@@ -326,7 +326,7 @@

 [urgency_critical]
     background = "#900000"
-    foreground = "#ffffff"
+    foreground = "#0fffff"
     frame_color = "#ff0000"
     timeout = 0
     # Icon for notifications with critical urgency, uncomment to enable

and ~/.config/dunst/dunstrc:

--- /usr/local/etc/xdg/dunst/dunstrc    2021-09-23 15:24:58.343225816 +0200
+++ /home/peter/.config/dunst/dunstrc  2021-09-29 19:33:57.746505639 +0200
@@ -325,8 +325,8 @@
     #icon = /path/to/icon

 [urgency_critical]
-    background = "#900000"
-    foreground = "#0fffff"
+    background = "#400000"
+    #foreground = "#ffffff"
     frame_color = "#ff0000"
     timeout = 0
     # Icon for notifications with critical urgency, uncomment to enable
1

When I do notify-send -c critical Test I get the background set in my user config but the foreground set in the system-wide config.

I think this PR will also be a good step towards #796

Nice, I was thinking of something like that, too. But maybe one does not need include for that. A proper setup of XDG_CONFIG_DIRS could achieve the same or similar, i.e. (staying in the given example, and in prefix /usr/local) put the themed dunstrc in ~/.config/manjaro/dunst/dunstrc and run env XDG_CONFIG_DIRS=~/.config/manjaro:/usr/local/etc/xdg, or something like that. Settings in ~/.config/manjaro/dunst/dunstrc will override those in /usr/local/etc/xdg/dunst/dunstrc and, since it is the most important one $XDG_CONFIG_HOME/dunst/dunstrc settings would override the manjaro-themed ones, if any. At least, that is the idea and how it should work. But if you say it might not, then I will have to change the code until it does.

Settings: Overhaul handling of rc-files. Dunst now searches all possible locations of config files and applies all settings from least important to most important file (see dunst(1) for more details). README.md: Added information about changed behaviour dunst.{1,5}: Information about new settings behaviour and deduplication dunst.1 explains in more detail how settings get found and applied. SYSCONFDIR is now in sync with the compile time parameter. dunst.5 got reduced a bit because of deduplication, so as to have only one location for the respective relevant information and not have to edit two or have them go out of sync. utils.c: Minor fix, 'delimiter' in string_to_array() was never used.

fwsmit

When I do notify-send -c critical Test I get the background set in my user config but the foreground set in the system-wide config.

That seems good. I'll do some testing as well when you have updated your PR with my comments.

Nice, I was thinking of something like that, too. But maybe one does not need include for that. A proper setup of XDG_CONFIG_DIRS could achieve the same or similar, i.e. (staying in the given example, and in prefix /usr/local) put the themed dunstrc in ~/.config/manjaro/dunst/dunstrc and run env XDG_CONFIG_DIRS=~/.config/manjaro:/usr/local/etc/xdg, or something like that. Settings in ~/.config/manjaro/dunst/dunstrc will override those in /usr/local/etc/xdg/dunst/dunstrc and, since it is the most important one $XDG_CONFIG_HOME/dunst/dunstrc settings would override the manjaro-themed ones, if any. At least, that is the idea and how it should work. But if you say it might not, then I will have to change the code until it does.

Sure that would be a possibility, but there are two things that make me like include better:

You'll have to set a separate XDG_CONFIG_DIRS for dunst, which is not exactly the purpose of that environment variable.
You cannot easily change dunst's environment during runtime. include can be combined with Reload config by SIGUSR1 #719 to dynamically change themes.

README.md

src/settings.c

src/utils.c

settings_data.h: Drive-by fix The default background of urgency_critical was same as foreground. Now it is in sync with default dunstrc. config.mk: Drive-by fix According to the gcc man page '--std' does not exist but it seems to be tolerated. Make that '-std' then. log.h: Tweak some log messages If the preprocessor allows it, prefix messages of levels 'critical', 'error', 'debug' with [<source path>:<function name>:<line number>]. Also some unified messages for logging fopen success/failure. option_parser.c: Change handling of sections/entries. Renamed `new_section()` to `get_or_create_section()` to be more transparent about its inner workings. An already existing section should not be fatal, just add/append entries to the *existing* one. Also, directly calling `new_section()` seemed pointless, since `add_entry()` would implicitly create sections as needed, or add to existing ones anyway. Not insisting on dying when there is a duplicate section in a config file opens a lot of possibilities. Now one can simply concatenate config files in arbitrary order and feed them to dunst on stdin, i.e. `cat $XDG_CONFIG_HOME/dunst/dunstrc \ $XDG_CONFIG_HOME/dunst/dunstrc.d/*.conf \ | dunst -conf - Internally 'option_parser' now adds/appends *all* entries to their respective sections, reusing existing ones. Empty sections will never be created, at least not by `load_ini_file()`. The most important entry in a section is the last one, because that will be the last to be committed to settings. So there should never be a problem with duplicates: last occurrence of a key dictates the final value. settings.c: Fixed issues mentioned in PR dunst-project#917 `settings_init()` and `set_defaults()` are called only once now. Tweaked settings_init(). `open_config_files()` now takes care of opening all config files, including the one given as command line parameter. doc: Update documentation Added some additional information to address possible misunderstandings about SYSCONFFILE and the runtime parameter -conf.

codecov-commenter · 2021-10-02T15:23:13Z

Codecov Report

Merging #917 (edb1f0b) into master (e973cf8) will increase coverage by 0.06%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
+ Coverage   60.96%   61.02%   +0.06%     
==========================================
  Files          39       39              
  Lines        6350     6365      +15     
==========================================
+ Hits         3871     3884      +13     
- Misses       2479     2481       +2

Flag	Coverage Δ
unittests	`61.02% <70.96%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/settings.c	`63.75% <60.86%> (+0.11%)`	⬆️
src/option_parser.c	`80.00% <100.00%> (+0.69%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e973cf8...edb1f0b. Read the comment docs.

src/option_parser.c

WhitePeter · 2021-10-02T15:46:52Z

src/option_parser.c

@@ -606,7 +607,7 @@ int load_ini_file(FILE *fp)

                        g_free(current_section);
                        current_section = (g_strdup(start + 1));
-                        new_section(current_section);
+                        LOG_D("[%s]", current_section);


Pretty print the section to make it look like in the ini file.

Also, do not call new_section() here, because the later call to add_entry() will simply create it, if it does not exist already.

WhitePeter

OK, I have it ready for review, finally--lost myself in tweaking. ;)

Running dunst -verbosity debug should make it obvious, what is happening, i hope. Try

cat ~/.config/dunst/dunstrc /usr/local/etc/xdg/dunst/dunstrc | dunst -verbosity debug -conf -

which was most likely fatal previously, because of section duplication. This example reverses the priority of dunstrc files, because the system-wide one is read last (possibly) overriding settings in the first one. I hope this makes the new power more visible that comes with this.

P.S.: Or, thinking of my sugesstion about drop-ins in #796:

cat ~/.config/dunst/dunstrc ~/.config/dunst/dunstrc.d/*.conf | dunst -verbosity debug -conf -

src/settings.c

WhitePeter · 2021-10-02T23:58:42Z

You cannot easily change dunst's environment during runtime. include can be combined with #719 to dynamically change themes.

I am working on implementing the drop-in idea as a demo of sorts. combined with #719 you can keep a static XDG_CONFIG_DIRS layout and just replace the file which controls the theme or whatever aspect it controls.

fwsmit

Sorry for the delay, but the changes look good to me.
I have a small note on your way of committing for next time (you don't have to change it now). Please try to keep your commits as small as possible and don't include multiple separate changes in one commit. It seems like you squashed multiple commits in one. Your commit messages are very clear though.

fwsmit · 2021-10-13T15:34:25Z

Thanks for working on this. It's merged now

WhitePeter · 2021-10-15T15:26:08Z

Sorry for my delay as well.

Please try to keep your commits as small as possible and don't include multiple separate changes in one commit.

I will.

Thanks for working on this. It's merged now

Thanks for letting me. ;)

There is more coming, just need to clean up the code for drop-ins which builds on this. I have some rudimentary support for includes in there as well but, to be perfectly honest, I don't like it because it allows for endless loops if 'a.conf' includes 'b.conf' which in turn includes 'a.conf' again. If I want to make that impossible it gets quite involved, unless I am missing the easy and obvious solution.

My aim is to get the PR ready today or tomorrow at the latest, so we can discuss it. I am not expecting a quick reply but will keep an eye on my notifications, just in case there is a reply with relevance to this.

fwsmit · 2021-10-15T15:33:57Z

more coming, just need to clean up the code for drop-ins which builds on this. I have some rudimentary support for includes in there as well but, to be perfectly honest, I don't like it because it allows for endless loops if 'a.conf' includes 'b.conf' which in turn includes 'a.conf' again. If I want to make that impossible it gets quite involved, unless I am missing the easy and obvious solution.

The easy way to avoid this is to only allow each file to be included once. I took inspiration for this feature from sway. From their man page:

include <path>
     Includes another file from path. path can be either a full path or a path relative to the parent config, and expands shell syntax (see wordexp(3) for details). The same include file can only be included once; sub‐
     sequent attempts will be ignored.

See https://github.com/swaywm/sway/blob/555cd96e0529539df5036bfcf7c6ffe0ee23931c/sway/config.c#L637 and https://github.com/swaywm/sway/blob/555cd96e0529539df5036bfcf7c6ffe0ee23931c/sway/config.c#L580 for the implementation.

WhitePeter · 2021-10-15T16:09:45Z

The easy way to avoid this is to only allow each file to be included once.

Thanks for the hint.

I took inspiration for this feature from sway. From their man page:
include <path>
     Includes another file from path. path can be either a full path or a path relative to the parent config, and expands shell syntax (see wordexp(3) for details). The same include file can only be included once; sub‐
     sequent attempts will be ignored.
See https://github.com/swaywm/sway/blob/555cd96e0529539df5036bfcf7c6ffe0ee23931c/sway/config.c#L637 and https://github.com/swaywm/sway/blob/555cd96e0529539df5036bfcf7c6ffe0ee23931c/sway/config.c#L580 for the implementation.

I will have a look.

fwsmit · 2021-10-15T16:19:15Z

@WhitePeter Just a warning, I'll be refactoring the ini code to make it usable for icon lookup as well. This may cause some conflicts with your code, so make sure to rebase early when this code comes.

fwsmit · 2021-10-19T16:10:22Z

@WhitePeter I added a draft version of the release notes here. Would you mind checking if I included everything that's important for maintainers trying to maintain a dunst package?

WhitePeter marked this pull request as ready for review September 21, 2021 20:25

WhitePeter force-pushed the dunstrc branch from d65ffb3 to ff8c451 Compare September 26, 2021 14:00

fwsmit reviewed Sep 29, 2021

View reviewed changes

fwsmit reviewed Oct 1, 2021

View reviewed changes

README.md Show resolved Hide resolved

src/settings.c Outdated Show resolved Hide resolved

src/settings.c Outdated Show resolved Hide resolved

src/utils.c Outdated Show resolved Hide resolved

WhitePeter mentioned this pull request Oct 2, 2021

Feature: include keyword in configuration #796

Open

WhitePeter force-pushed the dunstrc branch from ff8c451 to edb1f0b Compare October 2, 2021 15:22

WhitePeter commented Oct 2, 2021

View reviewed changes

src/option_parser.c Show resolved Hide resolved

WhitePeter commented Oct 2, 2021

View reviewed changes

src/option_parser.c Show resolved Hide resolved

WhitePeter commented Oct 2, 2021

View reviewed changes

src/settings.c Show resolved Hide resolved

fwsmit approved these changes Oct 13, 2021

View reviewed changes

fwsmit merged commit 38e5492 into dunst-project:master Oct 13, 2021

WhitePeter mentioned this pull request Oct 16, 2021

Add support for config drop-ins (rudimentary includes as a bonus) #933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings: Be more compliant with XDG BD spec. #917

Settings: Be more compliant with XDG BD spec. #917

WhitePeter commented Sep 21, 2021 •

edited

Loading

fwsmit commented Sep 22, 2021

WhitePeter commented Sep 22, 2021

WhitePeter commented Sep 22, 2021 •

edited

Loading

fwsmit commented Sep 22, 2021

WhitePeter commented Sep 22, 2021 •

edited

Loading

WhitePeter commented Sep 26, 2021 •

edited

Loading

fwsmit left a comment

WhitePeter commented Sep 29, 2021

fwsmit left a comment

codecov-commenter commented Oct 2, 2021 •

edited

Loading

WhitePeter Oct 2, 2021

WhitePeter Oct 2, 2021

fwsmit Oct 13, 2021

WhitePeter left a comment •

edited

Loading

WhitePeter commented Oct 2, 2021 •

edited

Loading

fwsmit left a comment

fwsmit commented Oct 13, 2021

WhitePeter commented Oct 15, 2021

fwsmit commented Oct 15, 2021

WhitePeter commented Oct 15, 2021

fwsmit commented Oct 15, 2021

fwsmit commented Oct 19, 2021

Settings: Be more compliant with XDG BD spec. #917

Settings: Be more compliant with XDG BD spec. #917

Conversation

WhitePeter commented Sep 21, 2021 • edited Loading

fwsmit commented Sep 22, 2021

WhitePeter commented Sep 22, 2021

WhitePeter commented Sep 22, 2021 • edited Loading

fwsmit commented Sep 22, 2021

WhitePeter commented Sep 22, 2021 • edited Loading

WhitePeter commented Sep 26, 2021 • edited Loading

fwsmit left a comment

Choose a reason for hiding this comment

WhitePeter commented Sep 29, 2021

fwsmit left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 2, 2021 • edited Loading

Codecov Report

WhitePeter Oct 2, 2021

Choose a reason for hiding this comment

WhitePeter Oct 2, 2021

Choose a reason for hiding this comment

fwsmit Oct 13, 2021

Choose a reason for hiding this comment

WhitePeter left a comment • edited Loading

Choose a reason for hiding this comment

WhitePeter commented Oct 2, 2021 • edited Loading

fwsmit left a comment

Choose a reason for hiding this comment

fwsmit commented Oct 13, 2021

WhitePeter commented Oct 15, 2021

fwsmit commented Oct 15, 2021

WhitePeter commented Oct 15, 2021

fwsmit commented Oct 15, 2021

fwsmit commented Oct 19, 2021

WhitePeter commented Sep 21, 2021 •

edited

Loading

WhitePeter commented Sep 22, 2021 •

edited

Loading

WhitePeter commented Sep 22, 2021 •

edited

Loading

WhitePeter commented Sep 26, 2021 •

edited

Loading

codecov-commenter commented Oct 2, 2021 •

edited

Loading

WhitePeter left a comment •

edited

Loading

WhitePeter commented Oct 2, 2021 •

edited

Loading