dunst

dunst copied to clipboard

As noted in #1209 a completion for dunstctl rule is currently not feasible due to missing information about which rules exist. This PR proposes to add a new DBus method to list all configured rules and use this information for a proper completion.

NB: Completion for bash and zsh will be adjusted when this approach is considered okay. Also, I'd add more information about the rules in the response in this case.

• [ ] add new subcommand to dunstctl(1)

Oct 17 '23 14:10 zappolowski

Codecov Report

Attention: Patch coverage is 76.59574% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 66.04%. Comparing base (4384521) to head (30b83c4). Report is 7 commits behind head on master.

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

@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
+ Coverage   65.92%   66.04%   +0.12%     
==========================================
  Files          50       50              
  Lines        8070     8214     +144     
  Branches      925      961      +36     
==========================================
+ Hits         5320     5425     +105     
- Misses       2750     2789      +39     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Oct 17 '23 14:10 codecov-commenter

Please add some tests as well. Our dbus interface is currently very well tested

Oct 24 '23 12:10 fwsmit

These shell completions look very promising. There's so many exciting things you can do with it :smile:

Oct 24 '23 12:10 fwsmit

Let me know if you'd like me to give it another round of review

Nov 10 '23 13:11 fwsmit

Are you planning to finish this PR? No pressure, you don't have to. But the feature seems cool

Dec 31 '23 17:12 fwsmit

Yes, I will try to finish this, though I cannot give a timeline when this will happen. Hopefully, within the next two weeks I will find some time to work on this (and the other PR).

Jan 02 '24 12:01 zappolowski

@fwsmit I've implemented a human-readable output as well. If you agree with this, I'd continue to look into tests and also add all the other missing fields to both calls.

Edit: ~Just saw, that the build fails as GStrvBuilder is not available ... this was released with glib 2.68 almost three years ago. Can we switch to more recent versions (see my PRs on the other project) or shall I find some alternative solution?~ I've rewritten it using string_append from utils.c.

Jan 19 '24 19:01 zappolowski

@fwsmit I've implemented a human-readable output as well. If you agree with this, I'd continue to look into tests and also add all the other missing fields to both calls.

With the human readable printing I was not thinking to put it over dbus. Dbus is meant to be a stable interface, where as the human readable output could change at any time. I would prefer it if the dbus output was converted to a human-readable output by the script. The human-readable part is also just an idea. If you think it's readable enough from the json output, or that it's not necesary at all, you can also drop it.

Edit: ~Just saw, that the build fails as GStrvBuilder is not available ... this was released with glib 2.68 almost three years ago. Can we switch to more recent versions (see my PRs on the other project) or shall I find some alternative solution?~

Yeah, the docker images should probably be updated to remove EOL distro's

EDIT: I see that you've made a PR for that already :)

Jan 20 '24 20:01 fwsmit

With the human readable printing I was not thinking to put it over dbus. Dbus is meant to be a stable interface, where as the human readable output could change at any time. I would prefer it if the dbus output was converted to a human-readable output by the script. The human-readable part is also just an idea. If you think it's readable enough from the json output, or that it's not necesary at all, you can also drop it.

I could try to come up with some jq magic here as well. But then I think, that jq should be made a (at least) optional dependency of this project. The completion for zsh already uses it unconditionally and also some other scripts in contrib rely on it.

I'm also fine with dropping a human-readable variant as of now and maybe add it later on when there's an actual need for it.

Edit: Just one note on the dropping it now and adding it later: when the default behavior of dunstctl rules would then be changed to show the human-readable variant, that would be considered a breaking change ... or human-readable representation would be hidden behind an optional flag (like the inverse of what is currently implemented).

Edit2: I've implemented the human-readable variant using jq.

Jan 21 '24 08:01 zappolowski

Please add some tests as well. Our dbus interface is currently very well tested

I've added some simple test. I hope that is enough and not all branches need to be tested separately.

Btw. none of the methods in the org.dunstproject.cmd0 interface are tested yet (mine being the first one ... as can be seen by the need of creating a new function to reach that interface).

Another note, I assume the call to g_variant_type_new in dbus_cb_dunst_NotificationListHistory leaking memory, but don't want to bring it into this PR.

Jan 21 '24 17:01 zappolowski

@fwsmit: Do you consider having a specified order in the output of the JSON important (currently I'm listing filter rules first and then the adjusted values). I could shorten the code quite drastically by using g_variant_dict_* but then would lose that order. On the other hand, one should probably not assume any order in a JSON response anyhow (which means I will take another look at the jq-Script for the human readable output).

Jan 21 '24 18:01 zappolowski

No, order is not very important. It would help a bit with finding the rule in the settings, but not neccessary. As you said, it should not be relied on, so it can be changed in the future if it's needed

Jan 21 '24 20:01 fwsmit

Everything looks fine. Maybe you could add dunstctl rules to the manpage.

NB: Completion for bash and zsh will be adjusted when this approach is considered okay. Also, I'd add more information about the rules in the response in this case.

not sure what you mean by that

Apr 19 '24 13:04 bynect

not sure what you mean by that

This is outdated from the time when the output just consisted of some assorted fields.

But I will adjust the other shell completions as well as add the new subcommand to dunstctl's man page.

Apr 19 '24 13:04 zappolowski

I've updated the man page as well as some completions. I've tested the bash completion (and found some issues which I will fix in another PR) but the ones for zsh are dry-coded (and thus kept minimal).

Apr 19 '24 14:04 zappolowski