Refactor queues by bebehei · Pull Request #411 · dunst-project/dunst

bebehei · 2017-10-11T11:09:43Z

Ok, I'm done with the queue refactoring.

Every part about accessing a GQueue is now handled in queues.c.
All methods handling a queue are prefixed with queues_ to resemble the namespace.
The three queues have unique names waiting, displayed, history.
All stuff in notification_init about closing notifications is removed
- to dbus.c: command pause/resume interpretation
- to queues.c: duplicates stacking

The refactoring about splitting notification_init is still to be done, but not part of this PR.

Mostly, I have split the stuff into two commits (the first one plain moving code from file.c to queues.c and the second one renaming/refactoring).

All commits compile without warnings.

Albeit most of the commits are plain refactoring, two commits stand out and may need further testing:

Force management of queues to queues.c
Remove command handling from notification_init
Move id assignment to separate function in queues.c

tsipinakis

Haven't yet sat down to do a detailed review of this, but here are some passing remarks after messing around for a few minutes:

Bug: xmore is displayed when there are no notifications waiting for some reason, to reproduce run dunst with the default config and send a notification via dunstify. Bisect says the bad commit is 50332b1

tsipinakis · 2017-10-11T14:40:12Z

                g_queue_push_tail(history, n);
 }

+gint64 queues_get_next_datachange(gint64 time)


Why is time a paremeter? Is there a scenario where we'd need to pass a different value for it other than the result of g_get_monotonic_time?

Yes, there seems to be no rationale for this. I don't know why I thought this would be neccessary. Maybe because I have to pass the output of x_is_idle.

tsipinakis · 2017-10-12T13:25:07Z

+        if(n->category == NULL) n->category = g_strdup("");
+        n->redisplayed = false;
+        n->first_render = true;
+        n->script = NULL;


Nitpick but not sure these are needed, notifications should be created via create_notification which returns zero-ed out memory anyway.

Yeah that stuff about notification_init. I only moved these values to the top of the notification to show, that these are actually not really processed in notification_init.

bebehei · 2017-10-12T13:40:47Z

Haven't yet sat down to do a detailed review of this, but here are some passing remarks after messing around for a few minutes:

Yes, I know. I've currently experiencing some weird segfaults (which are actually outside of this PR).

Please wait with a detailed review. I will ping you then again.

bebehei · 2017-10-24T14:49:17Z

From IRC:

Is #411 supposed to be ready for a review? You said you'd ping me but never did.

@tsipinakis I've written you a few days ago, that it's working fine and I have not found a bug for the last week. I never said it explicitly, but yes it was this type of "ping".

bebehei · 2017-10-26T19:20:16Z

I found another memory issue (which also is on current master): Notifications marked with history_ignore never get freed. This is fixed in the last commit now.

Decouple the x11 stuff from dunst.c, to be able to push update_lists to queues.c in the next commit

Move all id-changing functions out of notification_init and handle this in queues.c Also use stack_duplicates in combination with replacement of notifications correctly. Fixes dunst-project#404 (issue not found)

Previously for every notification (also stacked ones), a new notification id was used, even if the notification got discarded.

Hint for dbus.c lines (134,138): pause_display => (displayed->length == 0)

Also refactor the names to match the namespace.

This gives all three queues unique names makes it easier to trace back, which queue should hold which notifications.

bebehei · 2017-10-29T19:42:51Z

@tsipinakis

Thread Bene:
pr.lock()
write_tests()

I started writing tests for this. But: first I refactored notification_init, so that I can actually test the queues stuff. And I rebased the whole thing on current master, because merge conflicts with the #418 PR came along.

tsipinakis · 2017-10-30T19:29:44Z

From IRC

20:09:49 bebehei | Hey nikos, I've chopped off the commits from 411 I added yesterday. I will add the tests in a different PR.
Could you please go on and review/merge 411?

Though it looks like you also removed some bugfix commits like the notification not being freed if it isn't pushed to the history or the timeout fix as mentioned in #419 which should IMO be in this PR.

bebehei · 2017-10-30T20:48:53Z

Yes, I chopped off all unrelated commits. The other stuff will get merged via different PR.

tsipinakis

Given the size of this PR and that you're already preparing a follow-up it seems good enough to merge for me, didn't find anything worth delaying the PR over.

Something to consider though: When a DUNST_COMMAND_* notification is sent dunst returns id 0 for the notification even though the spec makes it clear that this should never happen

The returned ID is always greater than zero. Servers must make sure not to return zero as an ID.

tsipinakis · 2017-10-31T04:52:19Z

                                             0) == settings.context_ks.sym
                            && settings.context_ks.mask == state) {
                                context_menu();
+                                wake_up();


Is this wake_up call needed? The context menu doesn't modify anything.

Refactor queues

tsipinakis requested changes Oct 12, 2017

View reviewed changes

bebehei force-pushed the refactor-queues branch 4 times, most recently from cdd1752 to 66bfc9b Compare October 13, 2017 14:45

tsipinakis added this to the v1.3.0 milestone Oct 24, 2017

bebehei mentioned this pull request Oct 24, 2017

Replacing notifications does not make them time out #419

Closed

bebehei mentioned this pull request Oct 27, 2017

Unref GVariants after use again #421

Merged

bebehei force-pushed the refactor-queues branch 2 times, most recently from 75bba96 to f4d3221 Compare October 29, 2017 18:48

bebehei added 14 commits October 29, 2017 20:38

Refactor: Move queue specific methods to queues.c

a7003e3

Uncouple dunst.h from queues.c again

7f335b7

Move queue initialization to queues.c

7110734

Move maximum notification display count to x11

af9f6b8

Decouple the x11 stuff from dunst.c, to be able to push update_lists to queues.c in the next commit

Move id assignment to separate function in queues.c

f869175

Move all id-changing functions out of notification_init and handle this in queues.c Also use stack_duplicates in combination with replacement of notifications correctly. Fixes dunst-project#404 (issue not found)

Increment id counter only for displayed notifications

3fab4c4

Previously for every notification (also stacked ones), a new notification id was used, even if the notification got discarded.

Move get_sleep_time to queues.c

59ac6d0

Move pause_display to queues.c

c70da44

Hint for dbus.c lines (134,138): pause_display => (displayed->length == 0)

Move update_lists and check_timeouts to queues.c

12fa9d6

Also refactor the names to match the namespace.

Refactor notification_* functions to match queues namespace

38e4bbb

Refactor history_* functions to match queues namespace

e388176

Force management of queues to queues.c

a536e3f

Rename queue to waiting

616b8a1

This gives all three queues unique names makes it easier to trace back, which queue should hold which notifications.

Remove command handling from notification_init

b574834

bebehei force-pushed the refactor-queues branch 2 times, most recently from be4d0e2 to b574834 Compare October 30, 2017 17:44

tsipinakis approved these changes Oct 31, 2017

View reviewed changes

tsipinakis merged commit e111f53 into dunst-project:master Oct 31, 2017

tsipinakis mentioned this pull request Nov 2, 2017

Behaviour of duplicate notifications (again) #427

Closed

bebehei deleted the refactor-queues branch November 2, 2017 10:30

bebehei mentioned this pull request Nov 7, 2017

Notifications still shown albeit timed out #433

Closed

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

Merge pull request dunst-project#411 from bebehei/refactor-queues

a1abcb1

Refactor queues

Conversation

bebehei commented Oct 11, 2017

Uh oh!

tsipinakis left a comment

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

bebehei Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

bebehei Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

bebehei commented Oct 12, 2017

Uh oh!

bebehei commented Oct 24, 2017

Uh oh!

bebehei commented Oct 26, 2017

Uh oh!

bebehei commented Oct 29, 2017

Uh oh!

tsipinakis commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bebehei commented Oct 30, 2017

Uh oh!

tsipinakis left a comment

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tsipinakis commented Oct 30, 2017 •

edited

Loading