Implement hiding on fullscreen #472

bebehei · 2018-01-06T01:17:26Z

Implements a rule to hide specific notifications on fullscreen

Fixes #142

bebehei · 2018-01-12T20:57:46Z

@tsipinakis I've reworked it now, like we have discussed it in IRC. Only new notifications will get delayed and already drawn ones will still get displayed further.

This makes processing of timeouts easier. Previously, when switching between fullscreen and normal, timeouts started always from 0.

bebehei · 2018-01-13T00:17:59Z

@tsipinakis I've also taken the best of two worlds. I added a field in the enum to optionally also push the notifications back.

bebehei · 2018-02-16T03:01:39Z

~~I guess, I've hit a bug when processing XEvents. Under some circumstances, some notifications get delayed until a new one or a focus change happens.~~

~~I'll investigate further, but I've got a hunch, that the last change in the dropgtk PRs about ignoring specific PropertyChange events is ignoring the event in too much cases.~~

edit: resolved

bebehei · 2018-02-21T12:10:56Z

Note for myself: When merging together with #475, it'll fail building. (~~and #491, it'll fail building. This needs a change in the for the queue_* methods, which get an additional parameter fullscreen.~~ edit: resolved)

bebehei · 2018-02-24T18:44:56Z

I guess, I've hit a bug when processing XEvents. Under some circumstances, some notifications get delayed until a new one or a focus change happens.

So, I finally found a reproducible event to test it and I had been able to debug and fix it.

While debugging, I also found another small issue with the handling of xctx.cur_screen. Further details in the commit.

@tsipinakis I'd kindly request your review.

Off-Topic: Something about coveralls: On Travis Job 678.2, coveralls failed with a segfault. I have no clue, why this happened, but this is very spooky, we should investigate this. I can give an all-clear for this PR. The actual commit got amended is therefore not in this PR anymore. But it should be still alerting, that coveralls can fail for some reason and I'd actually would like to know why.

tsipinakis · 2018-02-25T17:53:11Z

Off-Topic: Something about coveralls: On Travis Job 678.2, coveralls failed with a segfault. I have no clue, why this happened, but this is very spooky, we should investigate this.

This is not new it's been happening since we started using coveralls and I haven't been able to figure out why. In any case it doesn't affect much since the gcc build still reports results.

tsipinakis

I'd normally drop reviews like this as it's working perfectly and there are no relevant issues with the code but submitted anyway since @bebehei prodded me to.

tsipinakis · 2018-02-25T17:57:10Z

src/notification.h

@@ -9,6 +9,12 @@

 #define DUNST_NOTIF_MAX_CHARS 5000

+enum BehaviorFS {


There is no official style guide (I'll get to writing it soon) but camel_case is preferred for structs functions and enums.

I'd normally drop reviews like this as it's working perfectly and there are no relevant issues with the code but submitted anyway since @bebehei prodded me to.

This is exactly the reason, why I wanted to get a nitpicky review. One of the things, I dislike about dunst is the hard inconsistency of specific things (like typenames). Because even I fail for myself to stick to one schema.

tsipinakis · 2018-02-25T18:05:13Z

src/queues.c

+        GList *iter = g_queue_peek_head_link(waiting);
+        while (iter) {
+                notification *n = iter->data;
+                GList *nextiter = iter->next;


[nitpick] Is there a reason for having nextiter var rather than directly referring to iter->next?

~~Must be an older relict of the times, when I wanted to replace all queue iterations with two pointers and a for loop.~~

Yes, at the point when you move the notification from waiting to displayed, iter->next becomes invalid.

tsipinakis · 2018-02-25T18:25:08Z

src/x11/x.c

-                LOG_D("Processing event: %s\n", xevent_type_to_string(ev.type));
+                const char *typename = xevent_type_to_string(ev.type);
+                if (typename)
+                        LOG_D("XEvent: processing '%s'", typename);


You can avoid having to use if logic for a simple debug log (which I dislike) by changing the statement to

LOG_D("XEvent: processing '%d: %s'", ev.type, typename);

and update xevent_type_to_string to return "Unknown" on an unkown event.

tsipinakis · 2018-02-25T18:53:22Z

src/x11/x.c

@@ -69,6 +69,48 @@ static void setopacity(Window win, unsigned long opacity);
 static void x_handle_click(XEvent ev);
 static void x_win_setup(void);

+/* see x.h */
+const char *xevent_type_to_string(int type)


I'm not sure about having this monster of a function just to log a debug message :/

We definitely can remove it. It was just super useful while debugging and I left it in on purpose. I wanted to know, what you think about it.

The PropertyNotify handling has been changed in the dropgtk branch to ignore XEvents, when redrawing was unnecessary. But when the fullscreen state of a window changes, we can't ignore the event, because it didn't change the screen. Additionally, there had been a mistake in the handling of the cur_screen: The xctx.cur_screen field will only get updated, when the application is visible and gets redrawn. Therefore, when a PropertyNotify event arrived while the application had been hidden and the screens do not match anymore, wake_up() will be called albeit being unnecessary. Calling x_win_draw() when the screens change is also the preferable solution over wake_up(), as there is nothing subject to change in the queues when the displays change.

Implement hiding on fullscreen

bebehei added this to the v1.4.0 milestone Jan 8, 2018

bebehei force-pushed the fullscreen branch from 9b206fb to c4fc44d Compare January 12, 2018 20:51

bebehei force-pushed the fullscreen branch from c4fc44d to dda870f Compare January 12, 2018 21:01

bebehei added Feature graphics labels Jan 13, 2018

bebehei force-pushed the fullscreen branch from 2bfd499 to 3c05eba Compare February 12, 2018 12:19

bebehei requested a review from tsipinakis February 14, 2018 00:45

bebehei force-pushed the fullscreen branch 3 times, most recently from ee59879 to eca7949 Compare February 21, 2018 10:10

bebehei force-pushed the fullscreen branch 2 times, most recently from ddf3cf2 to 8b56253 Compare February 24, 2018 18:26

tsipinakis requested changes Feb 25, 2018

View reviewed changes

bebehei added 8 commits February 25, 2018 20:18

Add functions to check fullscreen windows

9e824a7

Add rule to show notification on fullscreen

bda8c1d

Delay incoming notifications when fullscreens set

f89ce8e

Add documentation for fullscreen rules

bbe2628

Catch screen without window

aa7a851

Add Error Handler to fullscreen check

dd28e62

Print fullscreen value of notification

0f46564

Add pushback mode

aa5cc97

bebehei force-pushed the fullscreen branch from 8b56253 to 77004ee Compare February 25, 2018 19:30

bebehei added 2 commits February 25, 2018 20:44

Debug processed XEvents

c128731

log wakeup

a7bc168

bebehei force-pushed the fullscreen branch from 77004ee to 0cf4753 Compare February 25, 2018 19:44

tsipinakis approved these changes Feb 25, 2018

View reviewed changes

bebehei merged commit f80e6fc into dunst-project:master Feb 25, 2018

bebehei deleted the fullscreen branch March 15, 2018 13:30

bebehei mentioned this pull request Oct 30, 2018

do not display notifications when screensaver/lock is on or when app is in full screen mode #553

Closed

karlicoss pushed a commit to karlicoss/dunst that referenced this pull request Mar 21, 2019

Merge pull request dunst-project#472 from bebehei/fullscreen

e17bc52

Implement hiding on fullscreen

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement hiding on fullscreen #472

Implement hiding on fullscreen #472

bebehei commented Jan 6, 2018

bebehei commented Jan 12, 2018

bebehei commented Jan 13, 2018

bebehei commented Feb 16, 2018 •

edited

Loading

bebehei commented Feb 21, 2018 •

edited

Loading

bebehei commented Feb 24, 2018 •

edited

Loading

tsipinakis commented Feb 25, 2018

tsipinakis left a comment

tsipinakis Feb 25, 2018

bebehei Feb 25, 2018

tsipinakis Feb 25, 2018

bebehei Feb 25, 2018 •

edited

Loading

bebehei Feb 25, 2018

tsipinakis Feb 25, 2018

tsipinakis Feb 25, 2018

bebehei Feb 25, 2018

		@@ -9,6 +9,12 @@

		#define DUNST_NOTIF_MAX_CHARS 5000

		enum BehaviorFS {

Implement hiding on fullscreen #472

Implement hiding on fullscreen #472

Conversation

bebehei commented Jan 6, 2018

bebehei commented Jan 12, 2018

bebehei commented Jan 13, 2018

bebehei commented Feb 16, 2018 • edited Loading

bebehei commented Feb 21, 2018 • edited Loading

bebehei commented Feb 24, 2018 • edited Loading

tsipinakis commented Feb 25, 2018

tsipinakis left a comment

Choose a reason for hiding this comment

tsipinakis Feb 25, 2018

Choose a reason for hiding this comment

bebehei Feb 25, 2018

Choose a reason for hiding this comment

tsipinakis Feb 25, 2018

Choose a reason for hiding this comment

bebehei Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

bebehei Feb 25, 2018

Choose a reason for hiding this comment

tsipinakis Feb 25, 2018

Choose a reason for hiding this comment

tsipinakis Feb 25, 2018

Choose a reason for hiding this comment

bebehei Feb 25, 2018

Choose a reason for hiding this comment

bebehei commented Feb 16, 2018 •

edited

Loading

bebehei commented Feb 21, 2018 •

edited

Loading

bebehei commented Feb 24, 2018 •

edited

Loading

bebehei Feb 25, 2018 •

edited

Loading