add support for transient hints

[bebehei]

transient notifications. #310

• ignore notification automatically in history
• remove notification from stack, even if X is idle.

[tsipinakis]

Why is transient an int? The notification spec says it should be a boolean.

[bebehei]

Thanks for the feedback. At first, I thought shame on me. 🙈

The value type for the hint dictionary in D-BUS is of the DBUS_TYPE_VARIANT container type. This allows different data types (string, integer, boolean, etc.) to be used for hints.

So let's look at notify-send:

[bebe:~] % notify-send asdf 'asdf asdf asdf asdf asdf asdf asdf' --hint=bool:transient:1
Invalid hint type "bool". Valid types are int, double, string and byte.

It would be stupid to require this a boolean. We have to fix upstream bugs first, if we want to stick to the standard. 🤣

I have the feeling, that the gnome devs do not even look at their own standard sometimes.

[tsipinakis]

Some additional things that needs to be done to complete this:

• Update the history and idle_threshold documentation to mention transient notifications
• Implement transient support into dunstify (If we're going to maintain a debug tool, might as well keep it up to date).

[bebehei]

Implement transient support into dunstify (If we're going to maintain a debug tool, might as well keep it up to date).

dunstify supports --hints=... so there is no special support needed. Please correct me, if I'm wrong. I never used dunstify.

Update the history and idle_threshold documentation to mention transient notifications

done

[tsipinakis]

Wow, you're right about it not being supported by notify-send, and even more spectacularly according to the libnotify docs boolean hints are not even supported by the notification library.

dunstify supports --hints=... so there is no special support needed. Please correct me, if I'm wrong. I never used dunstify.

You're also correct here, I shouldn't do PR reviews while in a hurry, sorry!

Can you squash the commits before merging?

[bebehei]

Can you squash the commits before merging?

done.

[tsipinakis]

I decided to do a final check for the boolean issue, apparently this is how the transient hint needs to be specified by the notification clients so we should probably also handle booleans the same way as ints for now.

And as a codestyle suggestion, it'll probably be cleaner if we make the check and assignment inline as if ((dict_value = g_variant_lookup_value(content, "transient", G_VARIANT_TYPE_UINT32))) in this case, since triple nesting if statements hurts readability.

[bebehei]

And as a codestyle suggestion, it'll probably be cleaner if we make the check and assignment inline as if ((dict_value = g_variant_lookup_value(content, "transient", G_VARIANT_TYPE_UINT32))) in this case, since triple nesting if statements hurts readability.

👍 I've done it.

I decided to do a final check for the boolean issue, apparently this is how the transient hint needs to be specified by the notification clients so we should probably also handle booleans the same way as ints for now.

Added support for it.

[tsipinakis]

Small nitpick, but these should probably be else ifs, I don't see any point checking the rest if one is set.

[bebehei]

yes. that's good. I've always wondered, why you gave me this snippet:

if ((dict_value = g_variant_lookup_value(content, "transient", G_VARIANT_TYPE_UINT32)))

With else if constructs this makes totally sense.

btw: updated it.

[bebehei]

a bit off topic: I found a patch in the gnome bugtracker.

[tsipinakis]

Reported: 2012-02-17

Look like that bug isn't going to be fixed anytime soon unfortunately.

Since the main selling point of dunst is customizablility we should also add a rule to be able to override the transient hint but I'm fine with merging it as-is implementing that myself if you don't want to work on this.

[bebehei]

Since the main selling point of dunst is customizablility we should also add a rule to be able to override the transient hint but I'm fine with merging it as-is implementing that myself if you don't want to work on this.

I'm quoting @tsipinakis from #310:

At this point having history_ignore_transient and history_ignore starts to become a case of featuritis or at least having way too many similar knobs that users would have to dig a bit to find out the difference between them.

So, what do you want?

[tsipinakis]

In #310 the submitter mentioned adding history_ignore_transient as a global option on whether transient notifications should be ignored from history. My opinion still stands that this is a bit too specific to have as a global option.

But I think adding it as a rule as in match_transient and set_transient is reasonable since I'd expect that most, if not all the notifications attributes to be overridable via rules because that's what they were designed for.

I already know of a few programs that I use that (albeit customizable) set the transient hint by default while I'd prefer they didn't. In that case one can easily disable that by adding

[no-transient]
match_transient = true
set_transient = false

in their config file, and this still leaves it open to add more filters and restrict that to a per-app basis.

[bebehei]

I see your case.

I've added rule support for transient notifications. It went easier than I thought. I may have to add docs. Edit: added docs, too.

[bebehei]

Some other thoughts: After adding transient support and having also a rule to modify it. Is there any reason to still have history_ignore?

[tsipinakis]

After adding transient support and having also a rule to modify it. Is there any reason to still have history_ignore?

Not really, I'd mark it as obsolete for now but wouldn't deprecate it just yet, after cleaning up the codebase a bit one of the things I'd like to attempt is to try to move as much of the notification logic into the rule system as default rules instead of hardcoded logic, it can come in handy in that case.

Additionally, I think the set_transient and match_transient is a much better name than old_ and new_`, while that's not the naming scheme we've been using I think the current one doesn't make much sense and doesn't help to easily recognize what's a filtering rule and what's an action. See #349.

[bebehei]

Not really, I'd mark it as obsolete for now but wouldn't deprecate it just yet, after cleaning up the codebase a bit one of the things I'd like to attempt is to try to move as much of the notification logic into the rule system as default rules instead of hardcoded logic, it can come in handy in that case.

Just removing history_ignore option would be stupid of course ;-)

My thought was to remove the internal history_ignore field and save the option history_ignore (if set) in the transient field and let take transient precedence.

[tsipinakis]

My thought was to remove the internal history_ignore field and save the option history_ignore (if set) in the transient field and let take transient precedence.

I'd argue it's best to do the opposite, as in: Remove the transient hint from the history ignore logic and instead make it so that when a transient notification comes in history_ignore is set to true.

This has 2 main advantages: 1) users can disable transient notifications being removed from history if they want them to simply bypass the idle timeout and 2) it can be implemented using a hardcoded rule without touching the code at all.

[bebehei]

I'd argue it's best to do the opposite, as in: Remove the transient hint from the history ignore logic and instead make it so that when a transient notification comes in history_ignore is set to true.

I don't understand this. Having the name history_ignore in the struct makes no sense in context of notifications, while you save the transient hint value in it.

Also in respect of #349 I would use the name transient for the options, as there is no history_ignore what you could match. You only can match the transient hint.

1. users can disable transient notifications being removed from history if they want them to simply bypass the idle timeout

?

2. it can be implemented using a hardcoded rule without touching the code at all.

Actually after removing history_ignore logic in the code, it's just a refactoring.

---

I guess we're talking crossover currently. I don't understand what's your intention. I added in the last two commits the bits I described in my previous comment. I hope discussion based on this is made easier.

[tsipinakis]

I don't understand what's your intention

Looking back, you're right, I never mentioned publicly what my opinions on the rule system are and how I think it should work. Lets take the transient hint as a good example of how I see the rule system expanding.

For better or for worse the notification spec leaves ambiguous what exactly hints like transient are supposed to do and their behaviour is entirely subjective based on the notification servers and their developers, one might think that transient should mean that it bypasses history & idle timout, another user thinks that history should contain all notifications etc.

But, since we have a working (albeit kind-of simplistic atm) rule system we can use that to our advantage by exposing dunsts functionalities to it and letting it make the decision on how things should be handled. For example the transient hint currently ignores history and timeout, we could bury this logic somewhere inside dunsts internals or (the in my opinion better design), expose rules that allow setting which notifications bypass the timeout and history and setting a rule that enables these on transient notifications.

I am not opening a separate issue on this (for now, at least) because I think this is something that needs to be done passively while the codebase is being reworked. But the design idea is definitely up to discussion and debate.

[bebehei]

So you want the to have two options: transient and history_ignore, while transient controls, if the notification gets closed although x_is_idle() and history_ignore controls, whether the notification gets pushed onto history after it's closed?

So you want to have an absolute atomic rule system?

[tsipinakis]

Just tested this, it's working as intended with the exception of the history_ignore default, apparently in config.def.h the default of history_ignore is false. You should change it to -1 for it to work properly.

PS: Might want to improve your the commit messages a bit e.g. adapt if/else construct to upper analogy doesn't say much, Refactor if chain to avoid nested statements is much better for example. I recommend reading this in its entirety, it's the best commit message guide I have come across.

[bebehei]

Just tested this, it's working as intended with the exception of the history_ignore default, apparently in config.def.h the default of history_ignore is false. You should change it to -1 for it to work properly.

1. The part with the rules is ugly formatted.
2. Why don't we throw out the commented rules?
3. Why the fuck are there 2 times the exact same rules?
4. IIUC: The rules listed in config.def.h are hard coded into dunst and always valid rules, right? If so, the stuff from f905cf4 is the wrong approach. IMHO we should put a new rule where match_transient is set to 1 and history_ignore is also set to 1.

PS: Might want to improve your the commit messages a bit e.g. adapt if/else construct to upper analogy doesn't say much, Refactor if chain to avoid nested statements is much better for example. I recommend reading this in its entirety, it's the best commit message guide I have come across.

I've just copied your commit message now. Thanks for the link. I only browsed over it yet, but I already see, it's a must read. In general, the topic of writing good commit messages is often evaded. Thanks for the link.

[tsipinakis]

The part with the rules is ugly formatted.

Agreed, I've been meaning to look into finding better approach for hardcoded configurations because config.h is indeed quite ugly and having that many global variables starts polluting the namespace but this is out the scope of this PR.

Why don't we throw out the commented rules?

They are meant as an example in case anyone want to hardcode some custom rules.

Why the fuck are there 2 times the exact same rules?

Not a clue, there is only 1 uncommented rule in master, you added the second one :p

IIUC: The rules listed in config.def.h are hard coded into dunst and always valid rules, right? If so, the stuff from f905cf4 is the wrong approach. IMHO we should put a new rule where match_transient is set to 1 and history_ignore is also set to 1.

That's much better indeed, thanks for pointing it out.

I've just copied your commit message now. Thanks for the link. I only browsed over it yet, but I already see, it's a must read. In general, the topic of writing good commit messages is often evaded. Thanks for the link.

No problem, might want to convert all the commits to start with a capital too, as it's the standard convention.

[bebehei]

Not a clue, there is only 1 uncommented rule in master, you added the second one :p

Yes, this was a mistake of mine.

IIUC: The rules listed in config.def.h are hard coded into dunst and always valid rules, right? If so, the stuff from f905cf4 is the wrong approach. IMHO we should put a new rule where match_transient is set to 1 and history_ignore is also set to 1.

That's much better indeed, thanks for pointing it out.

Did it.

[tsipinakis]

This is also ready to be merged but it looks like merging #351 created a conflict. Can you resolve it?

[bebehei]

rebased it.

[tsipinakis]

This is long overdue for a merge, thanks as usual.