Sort types and ascending and descending strategy

[duarm]

A simple implementation of the idea discussed on #845, fixes #845. The new descending/ascending types also fixes #206 if you don't hit the notification_limit and put anything on the waiting queue.

The idea is to deprecate the sort option. Each new sort type would have a descending and ascending option. The other new config option is sort_ascending , which controls the order of the fallback id sort.

The idea is to repurpose the sort option to specify how you want to sort your notifications.

[fwsmit]

I don't think deprecating the sort option is neccesary. You can just add more options to sort while making sure the old behavior stays the same. For that it's probably easiest to add all the boolean options to sort_type_enum_data

[fwsmit]

I don't think we have many notifications to justify perfomance concerns while sorting, but having multiple notifications_cmp functions might make the code more manageable in the long term.

Don't think there are performance concerns either. We could always optimize later.

My plan was to implement the updated time sort method I wanted, but sorting by notification->start doesn't appear to update the position of the notification when the dup_count increases. Directions welcome.

I don't know. Maybe the notification->start value doesn't get updated correctly when duplicates arrive. You can try printing the value and see if it changes.

[duarm]

managed to get behaviour I wanted, also merged sort and sort_type as requested. The problem was that the queue didn't get sorted when a duplicate arrived, it just updated the values. Now if sort_type == SORT_TYPE_UPDATE, we sort both the displayed and waiting queue.

Here's a gif showing the SORT_TYPE_UPDATE, don't think an ascending/descending variation for update makes sense, so no option to use it.

I use my notifications to show the current song, sorting by update makes the currently playing song always at the top, even when going back and forth the playlist.

There's one test failure, I'll check it out.

[duarm]

I'm interested into a multiple sort approach too, I think sorting by update then sorting by urgency would be pretty useful, since I could keep the most recent at the relative top when switching songs, but a critical notification when arrived would be at the absolute top.

[fwsmit]

Hi, what's the status on this PR? Do you think I should review it in its current state, or are you still working on it?

[duarm]

Hi, what's the status on this PR? Do you think I should review it in its current state, or are you still working on it?

Hello, it's ready to review, there are just a few changes and it works, the only thing I'm not sure is about the sort I'm doing in queue.c, pretty sure there's a better way of doing that.

There are also clear avenues to improve, like having a different notification_cmp function for each sort type, if you think it's better that way, I can do it rather quickly.

Another nice feature would be multi sorts, like I explained on my last comment, but that can be another PR.

[fwsmit]

Hey, sorry for waiting so long to review this. Could you add the documentation in dunst.5.pod for the new configuration keys so I can check if the code does what you intend it to do (and if the configuration options are sensible)?

[duarm]

Hey, sorry for waiting so long to review this. Could you add the documentation in dunst.5.pod for the new configuration keys so I can check if the code does what you intend it to do (and if the configuration options are sensible)?

No problem, I'm not in a hurry

[duarm]

Added the docs to dunst.5.pod as requested, and resolved the conversations.

[codecov-commenter]

Codecov Report

Merging #1155 (4f8e7b8) into master (2cd719a) will decrease coverage by 0.05%. Report is 25 commits behind head on master. The diff coverage is 60.00%.

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

@@            Coverage Diff             @@
##           master    #1155      +/-   ##
==========================================
- Coverage   66.03%   65.98%   -0.05%     
==========================================
  Files          46       46              
  Lines        7595     7599       +4     
==========================================
- Hits         5015     5014       -1     
- Misses       2580     2585       +5     

Flag	Coverage Δ
unittests	65.98% <60.00%> (-0.05%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
test/queues.c	98.97% <100.00%> (ø)
src/queues.c	91.72% <66.66%> (-0.29%)	:arrow_down:
src/notification.c	60.18% <56.25%> (-0.44%)	:arrow_down:

... and 2 files with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[fwsmit]

I added a small bit of documentation, but other than that it looks good. Could check that bit of documentation and add it? Then I will merge this PR :)

[fwsmit]

Sorry for taking a while, but I'll merge this now! Thanks a lot for your work on this