Skip display rule option #590

mgsloan · 2019-01-16T03:21:17Z

The hack described in #588 - setting timeout = "1ms" in a rule to skip display - doesn't always work. Sometimes I see the notification for a brief flash.

There could potentially be a different workaround for this, say, skipping enqueue to the waiting list if the timeout is sufficiently low.

I've gone ahead and implemented an explicit option. This will also make things work better in the case that sticky_history is set to false. It also means that if DUNST_COMMAND_PAUSE has been used, when it gets unpaused it won't need to work its way through a queue of waiting notifications with tiny timeout.

bebehei · 2019-01-16T06:51:01Z

On my mobile, but some thoughts:

There's no NotificationClosed signal sent for notifications with skip_display set.
The queue length of history isn't respected.

But I definitele appreciate your new PR!

mgsloan · 2019-01-16T21:29:57Z

@bebehei Ah, good point! I've updated the PR to address those points.

tsipinakis · 2019-01-17T20:57:01Z

👍 As I mentioned in the previous issue, perhaps making this happen implicitly if the timeout is really short could save us adding another knob. @bebehei what do you think?

mgsloan · 2019-01-17T21:55:45Z

Yeah I'd be fine with timeout == 1 implying skipping display. It's a bit funky, would be better if it was timeout == 0, but that already has a special meaning.

Certainly can understand the desire to reduce the number of knobs. On the other hand, that would increase the number of particularly distinguished values on one of the knobs - maybe that's preferred?

bebehei

Sorry, this review was lying around for a few days. I obviously forgot to click the submit button.

+1 As I mentioned in the previous issue, perhaps making this happen implicitly if the timeout is really short could save us adding another knob. @bebehei what do you think?

Well, I'd rather choose complexity over weird behavior. But actually I'd appreciate none of both 😁

The functionality is implemented as a rule and I think this just supports our main feature, the rules.

But a no merger will be: missing tests.

src/queues.c

mgsloan · 2019-01-20T22:54:39Z

@bebehei Thanks for reviewing. I believe I've addressed your feedback in subsequent commits.

I think it's easier to review with incremental commits, but let me know if I should squash it into one (though I think it's best to keep the revert separate).

bebehei

Ok, we're getting into the corner cases now: What shall we do with notifications having a stack_tag, or qualify for stack_duplicates or have a replacement id. Skip_display behaves inconsistent.

bebehei · 2019-01-23T10:55:08Z

test/queues.c

+
+ queues_init();
+ queues_notification_insert(n);
+ QUEUE_LEN_ALL(0, 0, 1);


Honestly, I'm missing another check for the signal.

How do we check for the signal? Note that this code is exercising the queues, not end-to-end dbus functionality (the signal is done in dbus.c not queues.c). I don't see any end-to-end tests that also involve rules.

mgsloan · 2019-01-23T21:07:30Z

What shall we do with notifications having a stack_tag, or qualify for stack_duplicates or have a replacement id. Skip_display behaves inconsistent.

Good question. I considered this, but at the time thought it was reasonable that if a notification is being displayed, then stacking a skip_display notification atop it should continue being displayed.

However, I can see it being confusing if a notification that has a skip_display rule ends up being displayed in this case. I don't think there is necessarily a right answer here, but I also don't think it's worth making the knob more complicated to allow both semantics.

I can certainly try adding a commit that implements latter semantics, but I don't see a way to implement it without making the code a fair bit more complicated.

mgsloan · 2019-01-28T23:05:32Z

@bebehei Ping! As far as I can tell the next steps here are:

Determine how skip_display and notification stacking should interact
How to test signaling of issue close, particularly when the test is unit testing the queues which is not responsible for sending the close signal. At least, this is how I'm interpreting your comment on the test code.

bebehei · 2019-01-29T00:16:31Z

Determine how skip_display and notification stacking should interact

This would be my turn master...bebehei:pr590. You can take the code fully.

Every replacement case is covered by: Just replace it as usual, but vanish the notification right with the next update. see the docs from queues_notification_insert():

dunst/src/queues.h

Lines 126 to 127 in 711b11c

  * @post Call wake_up() to synchronize the queues with the UI 

  * (which closes old and shows new notifications on screen)

How to test signaling of issue close,

Sorry, I've sent you into the wrong direction. The basics are actually testable. But in conjunction together, it's too tricky to get tested right now.

This reverts commit 81de607.

mgsloan · 2019-01-31T07:56:26Z

@bebehei LGTM, thanks for making those changes. They seem to work well. I've rebased atop master and pushed to my branch.

I think this is ready for merge, right?

mgsloan · 2019-02-05T04:50:16Z

@tsipinakis Ping! Looks like @bebehei is requesting that you review this. Thanks!

tsipinakis · 2019-02-05T05:54:30Z

Opps, I completely missed the notification for this, sorry for the delay!

I did a review and realised halfway through that when pulling @bebehei 's tests you accidentally rolled back the commit fixing the initial mistakes.
e.g. There's still no NotificationClosed signal.

bebehei · 2019-02-05T09:48:06Z

e.g. There's still no NotificationClosed signal.

The signal is there. But there are no tests for it. Right now the signals aren't feasible to test.

tsipinakis · 2019-02-05T10:37:17Z

With 977a7a7 I don't see a NotificationClosed signal when skip_display is used. Am I missing something?

bebehei · 2019-02-05T10:44:44Z

Ouch, queues_history_push is the wrong method. I should have proposed queues_notification_close 🙈

…ory_push dunst-project#590

mgsloan · 2019-02-05T21:36:51Z

@tsipinakis @bebehei Ah, good catch! I've pushed a commit that fixes the problem, and have verified via dbus-monitor that the notification close is sent. I also moved an invocation of g_queue_delete_link, because queues_notification_close only works if the notification is still on the waiting queue.

…ory_push dunst-project#590

tsipinakis

LGTM

My perfectionist side wants to somehow merge this setting and format = "" into a single value but I can't come up with a reasonable way to do that.

Will merge after that small nitpick is resolved/answered (not a blocker, just curious about why it's there).

tsipinakis · 2019-02-06T18:08:20Z

src/queues.c

@@ -133,6 +133,9 @@ static bool queues_notification_is_finished(struct notification *n, struct dunst
 {
 assert(n);

+ if (n->skip_display && !n->redisplayed)
+ return true;


This hunk seems redundant, the notifications never see the displayed queue but skip directly from waiting to history. Was there a reason for adding this?

Hmm, would need to ask @bebehei as he wrote this. It does seem redundant for the current use of queues_notification_is_finished, but if there are future usages of it, it might be good to have this check.

It may seem redundant, but actually it isn't. For the case, that an already displayed notification gets replaced, this would trigger the immediate removal.

mgsloan · 2019-02-09T09:28:10Z

@tsipinakis Ping! (No rush, just wanted to check if this had fallen off your radar)

tsipinakis · 2019-02-09T09:32:58Z

It hasn't, was just waiting for @bebehei to respond but it looks like he's busy. I'll go ahead and merge this as it's not an actual issue and as you said some redundancy is a good thing.

Resolved

mgsloan · 2019-02-09T09:37:42Z

Great, thanks a bunch for reviewing, y'all!

bebehei · 2019-02-11T09:47:27Z

It hasn't, was just waiting for @bebehei to respond but it looks like he's busy

Yeah, I had been a few days off. Thanks for merging and caring here.

mgsloan force-pushed the skip-display-rule-option branch from f6a883e to 6e87503 Compare January 16, 2019 21:29

bebehei previously requested changes Jan 20, 2019

View reviewed changes

src/queues.c Outdated Show resolved Hide resolved

bebehei reviewed Jan 23, 2019

View reviewed changes

bebehei added this to the v1.4.0 milestone Jan 30, 2019

bebehei added the Feature label Jan 30, 2019

bebehei assigned mgsloan Jan 30, 2019

mgsloan added 2 commits January 30, 2019 23:50

Revert "Add a tip to the docs about how to skip initial display"

e2854c1

This reverts commit 81de607.

New rule option: skip_display

977a7a7

mgsloan force-pushed the skip-display-rule-option branch from 52a1f8e to 977a7a7 Compare January 31, 2019 07:55

bebehei requested a review from tsipinakis January 31, 2019 08:43

mgsloan added a commit to mgsloan/dunst that referenced this pull request Feb 5, 2019

Use queues_notification_close for skip_display instead of queues_hist…

3b409b0

…ory_push dunst-project#590

Use queues_notification_close for skip_display instead of queues_hist…

802b1ab

…ory_push dunst-project#590

mgsloan force-pushed the skip-display-rule-option branch from 3b409b0 to 802b1ab Compare February 5, 2019 21:37

tsipinakis approved these changes Feb 6, 2019

View reviewed changes

tsipinakis merged commit 63ec283 into dunst-project:master Feb 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip display rule option #590

Skip display rule option #590

mgsloan commented Jan 16, 2019

bebehei commented Jan 16, 2019

mgsloan commented Jan 16, 2019

tsipinakis commented Jan 17, 2019

mgsloan commented Jan 17, 2019

bebehei left a comment

mgsloan commented Jan 20, 2019

bebehei left a comment

bebehei Jan 23, 2019

mgsloan Jan 23, 2019

mgsloan commented Jan 23, 2019

mgsloan commented Jan 28, 2019

bebehei commented Jan 29, 2019

mgsloan commented Jan 31, 2019

mgsloan commented Feb 5, 2019

tsipinakis commented Feb 5, 2019 •

edited

Loading

bebehei commented Feb 5, 2019

tsipinakis commented Feb 5, 2019

bebehei commented Feb 5, 2019

mgsloan commented Feb 5, 2019

tsipinakis left a comment •

edited

Loading

tsipinakis Feb 6, 2019

mgsloan Feb 8, 2019

bebehei Feb 11, 2019

mgsloan commented Feb 9, 2019

tsipinakis commented Feb 9, 2019

mgsloan commented Feb 9, 2019

bebehei commented Feb 11, 2019

Skip display rule option #590

Skip display rule option #590

Conversation

mgsloan commented Jan 16, 2019

bebehei commented Jan 16, 2019

mgsloan commented Jan 16, 2019

tsipinakis commented Jan 17, 2019

mgsloan commented Jan 17, 2019

bebehei left a comment

Choose a reason for hiding this comment

mgsloan commented Jan 20, 2019

bebehei left a comment

Choose a reason for hiding this comment

bebehei Jan 23, 2019

Choose a reason for hiding this comment

mgsloan Jan 23, 2019

Choose a reason for hiding this comment

mgsloan commented Jan 23, 2019

mgsloan commented Jan 28, 2019

bebehei commented Jan 29, 2019

mgsloan commented Jan 31, 2019

mgsloan commented Feb 5, 2019

tsipinakis commented Feb 5, 2019 • edited Loading

bebehei commented Feb 5, 2019

tsipinakis commented Feb 5, 2019

bebehei commented Feb 5, 2019

mgsloan commented Feb 5, 2019

tsipinakis left a comment • edited Loading

Choose a reason for hiding this comment

tsipinakis Feb 6, 2019

Choose a reason for hiding this comment

mgsloan Feb 8, 2019

Choose a reason for hiding this comment

bebehei Feb 11, 2019

Choose a reason for hiding this comment

mgsloan commented Feb 9, 2019

tsipinakis commented Feb 9, 2019

mgsloan commented Feb 9, 2019

bebehei commented Feb 11, 2019

tsipinakis commented Feb 5, 2019 •

edited

Loading

tsipinakis left a comment •

edited

Loading