replace 'folder' with 'path' in icon functionality

[gangelop]

This bothered me.
I replaced all the instances of 'folder' with 'path'.
I tested the icon functionality and it worked, including the relevant configuration option; but please somebody else also look at it because I'm unfamiliar with the code.

[knopwob]

I agree with your point but this would break running configs. I'm still considering merging this because the configuration option hastn't been in the last release so anyone using this is using a development version anyway.

I'll make a decision on this before I make the next release.

[giodamelio]

👍 Path is much better

[nifr]

👍 for path ... this minor BC break would just bump the minor version.

Merge and tag with 1.2.0 according to semver.

[gangelop]

rebased on master

[tsipinakis]

This will probably be merged for the next release. @lathan can you resolve the merge conflicts?

[bebehei]

Could you also please use path instead of paths.

Please use it in singular form, as most unices name such a variable type in singular form.

[gangelop]

oops, sorry for disappearing for 3 years.

I went back to master and ran:

find -type f -exec sed -i 's/icon_folders/icon_path/g' {} +

Beyond my mad sed skillz, maybe it would be useful to retain icon_folders for backwards compatibility.

@tsipinakis your thoughs?

[tsipinakis]

oops, sorry for disappearing for 3 years.

Nice timing, I was just trying to decide how long I want to hold off before closing PRs due to inactivity.

maybe it would be useful to retain icon_folders for backwards compatibility.

We obviously should since this change is probably going to break many configs that rely heavily on icons. There are some examples in settings.c on how to handle backwards compatibility.

[gangelop]

I think 095b310 should cover the backwards compatibility. This is probably the first real C code I've ever written, so if someone can review it, that'd be nice.

Also, I noticed that other existing if/else backwards compatibility code is enclosed in curly braces { }, changing the variable scope, iiuc. I'm not exactly sure what purpose this serves and I didn't think it was needed so I didn't do it.

Finally, I think there is some issue with the help output generation when running dunst -h. I'll follow up on this with more details.

[gangelop]

Regarding the output of dunst -h, if icon_path or icon_folders appear neither as command line arguments or config file options, then they also do not appear in help output, which of course should not happen.

This is also the case for markup and its older form allow_markup. Not surprising since I based my code on this earlier code. Both have the same issue. So, it would be nice to fix this before pulling it in. Not sure if I'll be able to but I'll try.

[bebehei]

when you call -help, -icon_path is not set. So, the line if (ini_is_set("global", "icon_path") || cmdline_is_set("-icon_path")) { is not triggered.

The function cmdline_usage_append is not called (which gets called in options_get_string.

[gangelop]

sec... I think I got it. testing stuff...

[bebehei]

Just put the correct call of options_get_string out of the if-clause and check for the old option first.

[gangelop]

@bebehei how about 14b270b ?

[bebehei]

scheme:

// check if icon_folders is set
// do not show up in --help
if(isset(icon_folders)){
    option_get_string(... icon folders ...);
}

//check if icon_path is set
//show up in --help
// override icon_folders and use icon_path if both set.
option_get_string( ... icon_path ...);

I think this might give all neccessary hints to rearrange the code.

[gangelop]

@bebehei I'm afraid I don't really understand where you were going with that.

How about 2c917cb?

It's basically this:

if icon_path {
    // save the setting and generate icon_path help
} else if icon_folders {
    // save the setting and generate icon_folders help
    // with deprecation warning
} else {
    // use default setting and generate icon_path help
}

The logic is straightforward. icon_path takes precedence.

There is only one thing which bothers me. Help output differs depending on which combination of options is used. I don't see how I can control help output generation independently of setting the value for settings.icon_path. Sure, I can call option_get_string by itself which will just eventually call cmdline_usage_append with whatever value I give it. That's not a problem.

The problem is that I cannot not call cmdline_usage_append when I get call option_get_string to read the value for icon_folders.

I'll note again, that this issue is not unique to icon_folders/icon_path. The allow_markup/markup option has a similar issue and I don't see any better solution in the code at the moment.

[bebehei]

That's the code, what should be in there:

        if (ini_is_set("global", "icon_folders") || cmdline_is_set("-icon_folders")) {
                settings.icon_path = option_get_string(
                        "global",
                        "icon_folders", "-icon_folders", icon_path,
                        "folders to default icons (deprecated, please use 'icon_path' instead"
                );
                fprintf(stderr, "Warning: 'icon_folders' is deprecated, please use 'icon_path' instead.\n");
        }
        settings.icon_path = option_get_string(
                "global",
                "icon_path", "-icon_path", icon_path,
                "paths to default icons"
        );

• no code duplication
• icon_path shows up in help, but icon_folders shows up only, if it's set
• icon_path takes precedence
• Warnings are printed correctly

Copy it. You're allowed to do it.

I'll note again, that this issue is not unique to icon_folders/icon_path. The allow_markup/markup option has a similar issue and I don't see any better solution in the code at the moment.

Yes, I've already prepared a branch for this. I will create a separate PR for this.

[gangelop]

Ok, that's one of the many approaches I tried but it doesn't work.

The reason it doesn't work is because the icon_path code always overwrites the icon_folders code. The icon_folder option will never be applied and an existing user who runs this will have their icon_folders option ignored and get the compile-time default for icon_path instead.

Also, code duplication isn't always bad, especially when it makes the code easier to comprehend. :)

[bebehei]

The reason it doesn't work is because the icon_path code always overwrites the icon_folders code. The icon_folder option will never be applied and an existing user who runs this will have their icon_folders option ignored and get the compile-time default for icon_path instead.

Ah, yes. That's a point. What about using a ternary operator?

        settings.icon_path = option_get_string(
                "global",
                "icon_path", "-icon_path",
                settings.icon_path ? settings.icon_path : icon_path,
                "paths to default icons"
        );

[bebehei]

Also, code duplication isn't always bad, especially when it makes the code easier to comprehend. :)

Yes, but we have to execute option_get_string unconditionally with icon_path one time to show it up in -help. Packing everything in if-clauses will prevent this.

[gangelop]

Yes, but we have to execute option_get_string unconditionally with icon_path one time to show it up in -help. Packing everything in if-clauses will prevent this.

My proposal was "if, else if, else". One of these three always runs.

I like the ternary operator approach though. Commit incoming...

[gangelop]

The last four commits could be squashed into one. Let me know if you want me to do that or keep all of them.

[bebehei]

My proposal was "if, else if, else". One of these three always runs.

🤦‍♂️ 🙈 Yes, you're right.

The last four commits could be squashed into one. Let me know if you want me to do that or keep all of them.

I'd guess you should squash them.

[gangelop]

Squashed and force pushed. Old branch backed up as not_windoze-backup.

[tsipinakis]

LGTM 👍

Thanks a lot to both of you for working on this!

[gangelop]

ty @tsipinakis

thanks for the tips @bebehei !