Export rules via dbus #1212

zappolowski · 2023-10-17T14:52:05Z

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)

codecov-commenter · 2023-10-17T14:53:38Z

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 ⚠️

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

Additional details and impacted files

@@            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	Coverage Δ
unittests	`66.04% <76.59%> (+0.12%)`	⬆️

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

Your approach looks good. I have only one comment.

fwsmit · 2023-10-24T12:05:03Z

dunstctl

@@ -31,6 +31,7 @@ show_help() {
 	                                    history with given ID.
 	  is-paused                         Check if dunst is running or paused
 	  set-paused [true|false|toggle]    Set the pause status
+	  rules                             Displays configured rules (in JSON)


I know the history command already formats in json by default, but I'm wondering if it's not better to format in human-readable format by default and allow for a json toggle.

This should be easily feasible.

My current idea is to have:

rules (as-json|human-readable) Displays configured rules

with human-readable being the default when omitted. I don't know for sure, what's the correct notation for denoting an optional argument (or should it be mandatory, though I think it would be nice to have a default for interactive usage) and just used parentheses as an example.

How should the human-readable form look like? One could just emit ini-style information, basically recreating the original input.

I would prefer to use an argument like --json for json output. And it should definitely not be required, for the reason you mentioned.

Optional commands are put in [square brackets].

I don't know what the best human-readable output is, but I would just look at what's easy to create. On second thought, the pretty json output might be good enough as well

I currently don't have a good test setup to see, so you'll have to judge for yourself what's best

Sure, I can do --json as well. I just wanted to be consistent how other commands are used.

Optional commands are put in [square brackets].

Yes, that's how I would've expected it as well, but the usage output doesn't use it this way consistently. history-pop and history-rm look the same, but just history-pop uses it as an optional argument. set-paused and rule also use them for mandatory arguments.

Sure, I can do --json as well. I just wanted to be consistent how other commands are used.

You don't have to do it just because I mentioned it. Only if you think it's a good idea. Consistency between different commands is also nice.

Optional commands are put in [square brackets].

Yes, that's how I would've expected it as well, but the usage output doesn't use it this way consistently. history-pop and history-rm look the same, but just history-pop uses it as an optional argument. set-paused and rule also use them for mandatory arguments.

Oh that's a little bit weird. You would only need one of these commands. You can fix that in a separate PR if you want

fwsmit · 2023-10-24T12:10:11Z

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

fwsmit · 2023-10-24T12:11:02Z

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

fwsmit · 2023-11-10T13:45:11Z

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

fwsmit · 2023-12-31T17:33:17Z

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

zappolowski · 2024-01-02T12:43:28Z

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 · 2024-01-19T19:33:41Z

@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 · 2024-01-20T20:48:06Z

dunstctl

+					|| die "Dunst is not running."
+			;;
+			*)
+				die "Unknown format \"${2}\". Please use either \"as-json\" or \"human-readable\"."


This help message is outdated

fwsmit · 2024-01-20T20:52:48Z

@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 · 2024-01-21T08:07:34Z

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 · 2024-01-21T17:36:14Z

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 · 2024-01-21T18:41:48Z

@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 · 2024-01-21T20:10:49Z

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

fwsmit · 2024-01-25T09:49:45Z

src/dbus.c

+                if (r->set_stack_tag)
+                        g_variant_dict_insert(&dict, "set_stack_tag", "s", r->set_stack_tag);
+                if (r->override_pause_level != -1)
+                        g_variant_dict_insert(&dict, "override_pause_level", "i", r->override_pause_level);


In this function, is it not possible to use the data in settings_data.h to programmatically fill the dictionary? All rules in the settings data struct have a non-zero rule (except probably for the first one, if that's a problem that's fixable).
This would make it more easy to maintain. Otherwise it's easy to forget to add it here after adding a rule. If you have idea's for how to do this, please let me know. Then we can discuss if it's worth doing.

My knowledge of C is rather limited, but if I got you right, this would mean compile time code generation, which would mean using macros. I'm not 100% sure on this, but I would highly doubt, that this is possible. Run-time code generation via reflection is not possible in C AFAIK.

My (maybe overly simplistic) approach: just write some comment to the definition of rule to remind the implementer to add it to dbus_cb_dunst_RuleList as well.

Okay, could you then add some documentation to settings_data.h?

maybe I can make it automatic when I refactor rule parsing. for now it is fine though

The intention is to have a way to get all configured rules.

This can be re-used to retrieve other pairs of information (e.g. history id message could be helpful as well).

bynect · 2024-04-19T13:50:48Z

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 · 2024-04-19T13:56:20Z

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 · 2024-04-19T14:42:31Z

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

The old JSON formatted variant is now reachable with the `--json` flag.

zappolowski changed the title ~~Experimental rules via dbus~~ Draft: export rules via dbus Oct 17, 2023

fwsmit reviewed Oct 24, 2023

View reviewed changes

zappolowski force-pushed the experimental-rules-via-dbus branch from d93582c to 466267e Compare October 26, 2023 18:44

zappolowski force-pushed the experimental-rules-via-dbus branch from 466267e to d01cbd7 Compare November 9, 2023 18:22

zappolowski mentioned this pull request Nov 10, 2023

Use brackets consistently in dunstctl usage #1233

Merged

zappolowski force-pushed the experimental-rules-via-dbus branch from d01cbd7 to eaaedd9 Compare January 2, 2024 12:41

zappolowski force-pushed the experimental-rules-via-dbus branch from eaaedd9 to a733909 Compare January 19, 2024 19:31

zappolowski force-pushed the experimental-rules-via-dbus branch 7 times, most recently from f63eebd to 6c5991d Compare January 20, 2024 18:13

fwsmit reviewed Jan 20, 2024

View reviewed changes

zappolowski force-pushed the experimental-rules-via-dbus branch 2 times, most recently from 1f98aa8 to 5468661 Compare January 21, 2024 17:31

zappolowski force-pushed the experimental-rules-via-dbus branch from 4ecf35c to 63e48d4 Compare January 21, 2024 19:44

fwsmit reviewed Jan 25, 2024

View reviewed changes

zappolowski mentioned this pull request Jan 30, 2024

Feature: Install completions by default #1263

Merged

zappolowski force-pushed the experimental-rules-via-dbus branch 2 times, most recently from 8d03d0a to db6b9bf Compare February 19, 2024 05:14

zappolowski added 3 commits April 19, 2024 10:22

Add new Dbus method RuleList

5b209f3

The intention is to have a way to get all configured rules.

Teach dunstctl how to get all defined rules

27f2aea

Generalize __fish_dunstctl_history to __fish_dunstctl_info

adca2c3

This can be re-used to retrieve other pairs of information (e.g. history id message could be helpful as well).

zappolowski force-pushed the experimental-rules-via-dbus branch from 9802037 to dd3bc2a Compare April 19, 2024 08:33

zappolowski marked this pull request as ready for review April 19, 2024 08:35

zappolowski changed the title ~~Draft: export rules via dbus~~ Export rules via dbus Apr 19, 2024

zappolowski requested review from fwsmit and bynect April 19, 2024 13:21

zappolowski force-pushed the experimental-rules-via-dbus branch from dd3bc2a to 414d5d2 Compare April 19, 2024 14:41

zappolowski force-pushed the experimental-rules-via-dbus branch from 414d5d2 to 623ae02 Compare April 19, 2024 15:15

zappolowski added 4 commits April 19, 2024 17:28

Add new rules and implement proper rule completion in fish

677cac5

Implement human-readable variant of RuleList

78e4289

The old JSON formatted variant is now reachable with the `--json` flag.

Document new rules subcommand in man page of dunstctl

b852511

Update completions to use new method

30b83c4

zappolowski force-pushed the experimental-rules-via-dbus branch from 623ae02 to 30b83c4 Compare April 19, 2024 15:28

bynect approved these changes Apr 19, 2024

View reviewed changes

zappolowski merged commit b354f47 into dunst-project:master Apr 19, 2024
19 checks passed

zappolowski deleted the experimental-rules-via-dbus branch April 19, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export rules via dbus #1212

Export rules via dbus #1212

zappolowski commented Oct 17, 2023 •

edited

Loading

codecov-commenter commented Oct 17, 2023 •

edited

Loading

fwsmit left a comment

fwsmit Oct 24, 2023

zappolowski Oct 24, 2023

fwsmit Oct 24, 2023

fwsmit Oct 24, 2023

fwsmit Oct 24, 2023

zappolowski Oct 24, 2023 •

edited

Loading

fwsmit Nov 10, 2023

fwsmit commented Oct 24, 2023

fwsmit commented Oct 24, 2023

fwsmit commented Nov 10, 2023

fwsmit commented Dec 31, 2023

zappolowski commented Jan 2, 2024

zappolowski commented Jan 19, 2024 •

edited

Loading

fwsmit Jan 20, 2024

fwsmit commented Jan 20, 2024 •

edited

Loading

zappolowski commented Jan 21, 2024 •

edited

Loading

zappolowski commented Jan 21, 2024

zappolowski commented Jan 21, 2024

fwsmit commented Jan 21, 2024

fwsmit Jan 25, 2024

zappolowski Jan 25, 2024

fwsmit Feb 16, 2024

bynect Apr 19, 2024

bynect commented Apr 19, 2024

zappolowski commented Apr 19, 2024

zappolowski commented Apr 19, 2024

Export rules via dbus #1212

Export rules via dbus #1212

Conversation

zappolowski commented Oct 17, 2023 • edited Loading

codecov-commenter commented Oct 17, 2023 • edited Loading

Codecov Report

fwsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zappolowski Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwsmit commented Oct 24, 2023

fwsmit commented Oct 24, 2023

fwsmit commented Nov 10, 2023

fwsmit commented Dec 31, 2023

zappolowski commented Jan 2, 2024

zappolowski commented Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

fwsmit commented Jan 20, 2024 • edited Loading

zappolowski commented Jan 21, 2024 • edited Loading

zappolowski commented Jan 21, 2024

zappolowski commented Jan 21, 2024

fwsmit commented Jan 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bynect commented Apr 19, 2024

zappolowski commented Apr 19, 2024

zappolowski commented Apr 19, 2024

zappolowski commented Oct 17, 2023 •

edited

Loading

codecov-commenter commented Oct 17, 2023 •

edited

Loading

zappolowski Oct 24, 2023 •

edited

Loading

zappolowski commented Jan 19, 2024 •

edited

Loading

fwsmit commented Jan 20, 2024 •

edited

Loading

zappolowski commented Jan 21, 2024 •

edited

Loading