Feature: Install completions by default

[zappolowski]

This resolves #1262.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.51%. Comparing base (660e8ba) to head (56ca621).
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1263   +/-   ##
=======================================
  Coverage   65.51%   65.51%           
=======================================
  Files          48       48           
  Lines        7997     7997           
=======================================
  Hits         5239     5239           
  Misses       2758     2758           

Flag	Coverage Δ
unittests	65.51% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fwsmit]

LGTM. If we ship completions by default, they have to be removed from the contrib directory, though. That contains only community additions.

[zappolowski]

If we ship completions by default, they have to be removed from the contrib directory, though.

Do you have a preferred directory for them to be moved to?

I just quickly went through some projects (distilled from installed bash completions on my system):

project	directory
mako	contrib
ninja	misc
podman	completions
systemd	shell-completions

[fwsmit]

I like "completions" the best. So if you don't have a strong preference you can go for that

[zappolowski]

@fwsmit I just realized that when completions are installed by default, we should really make jq an official dependency (even more necessary with the changes to dunstctl pending in #1212).

If you agree on this, I would:

1. add jq to the README as dependency (this PR)
2. rework the existing completions to (in another PR)
   1. get rid of the fallback to awk in case jq is not installed (dunstify.fishcomp, dunstctl.fishcomp)
   2. add jq based parsing to dunstctl.bashcomp

Do you agree on this approach?

[fwsmit]

Jq is a little bit of a weird dependency to have for dunst. Are there simple alternatives? Maybe the output of dunstctl history and rules could be tweaked?
Also, awk seems to be required for at least the zsh completions.

[zappolowski]

Jq is a little bit of a weird dependency to have for dunst.

I partially agree here, but I currently don't see any (time and code) efficient way of getting the needed information out of the DBus data while keeping dunstctl a simple shell script as it currently is. One way of getting forward would be to rewrite dunstctl in C.

Maybe the output of dunstctl history and rules could be tweaked?

Do you have any proposals here? I don't see how it can be made more simple than an array of array of key-values pairs (the inner one basically forming a dictionary). If the output of DBus needs to be parsed (which is required to provide the actual data), it will be really ugly in any kind of shell if there are no external dependencies are allowed - e.g. just getting multi line strings properly parsed will be one major issue.

Also, awk seems to be required for at least the zsh completions.

systemd (through busctl) are also (not mentioned) dependencies since the introduction of the history subcommand of dunstctl.

Some more thoughts of how to get forward:

• don't install completions or any other scripts which require external dependencies at all - leave it to the user to get them from the repository - basically closing this PR
• drop human-readable formatting of dunstctl rules (not related to this PR, but it's affected by this discussion) as it is not easily feasible without introducing new dependencies

[fwsmit]

Jq is a little bit of a weird dependency to have for dunst.

I partially agree here, but I currently don't see any (time and code) efficient way of getting the needed information out of the DBus data while keeping dunstctl a simple shell script as it currently is. One way of getting forward would be to rewrite dunstctl in C.

Maybe the output of dunstctl history and rules could be tweaked?

Do you have any proposals here? I don't see how it can be made more simple than an array of array of key-values pairs (the inner one basically forming a dictionary). If the output of DBus needs to be parsed (which is required to provide the actual data), it will be really ugly in any kind of shell if there are no external dependencies are allowed - e.g. just getting multi line strings properly parsed will be one major issue.

Yeah, the only thing I can think of is to do some clever grepping of the output of dunstctl history. But that's more flakey than properly parsing the json. So I think adding the dependency is best. Most distro's ship jq anyways.

Also, awk seems to be required for at least the zsh completions.

systemd (through busctl) are also (not mentioned) dependencies since the introduction of the history subcommand of dunstctl.

Good point, could you add it, while you're at it?

Some more thoughts of how to get forward:

* don't install completions or any other scripts which require external dependencies at all - leave it to the user to get them from the repository - basically closing this PR

* drop human-readable formatting of `dunstctl rules` (not related to this PR, but it's affected by this discussion) as it is not easily feasible without introducing new dependencies

Yeah, I just wanted to bring it up to brainstorm a bit about alternatives, but they all seem worse than just adding the dependency.

[bynect]

As a side note, is the gdk-pixbuf dependency missing in the documentation?

[Narrat]

As a side note, is the gdk-pixbuf dependency missing in the documentation?

At least it isn't mentioned in the README. Was mentioned in the CHANGELOG and RELEASE_NOTES

[bynect]

I think it should be added in the readme, alongside librsvg which is a runtime dependency (of gdk-pixbuf)

As a side note, is the gdk-pixbuf dependency missing in the documentation?

At least it isn't mentioned in the README. Was mentioned in the CHANGELOG and RELEASE_NOTES

I think it should be added in the readme, alongside librsvg which is a runtime dependency (of gdk-pixbuf)

[zappolowski]

@bynect @Narrat I suggest to merge this branch as is and update the dependencies on another (dedicated) PR.

[bynect]

Fine by me. I noticed that several dependencies are outdated both in the readme, wiki pages and documentation in general. We may want also to update the website with some more up to date info.

[bynect]

Everything seems fine. I was wondering if we should check if the completion dirs exist before writing to them (I left a suggestion in the comments). But it's not really important I guess