search

dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.44k stars 338 forks source link

Export rules via dbus #1212

Closed zappolowski closed 3 months ago

zappolowski commented 9 months ago

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.

codecov-commenter commented 9 months ago

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.

Files Patch % Lines
src/dbus.c 64.13% 33 Missing :warning:

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

Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1212/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1212/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `66.04% <76.59%> (+0.12%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

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

fwsmit commented 9 months ago

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

fwsmit commented 9 months ago

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

fwsmit commented 8 months ago

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

fwsmit commented 6 months ago

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

zappolowski commented 6 months ago

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).

zappolowski commented 6 months ago

@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.

fwsmit commented 6 months ago

@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 :)

zappolowski commented 6 months ago

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.

zappolowski commented 6 months ago

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.

zappolowski commented 6 months ago

@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).

fwsmit commented 6 months ago

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

bynect commented 3 months ago

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

zappolowski commented 3 months ago

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.

zappolowski commented 3 months ago

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).