Add mouse action to show context menu

[lukasrad02]

This PR adds the mouse actions context and context all, which open the context menu for either the notification clicked on or all notifications visible, as suggested in #234 instead of clickable action buttons.

[fwsmit]

Thank you for the clear PR, it makes it easy to review. I will note however that there is already do_action, which does almost the same as context, except it doesn't open the context menu when there is only 1 option.
Could you explain how you would use the context and context all mouse actions?

[lukasrad02]

In my experience and as stated in the docs, do_action does not only check whether there is only one action but also whether one of the actions is marked as default. In many programs I am using, the default action is to bring the application window to the front so that I can directly interact with the application, while it would be much faster and less interrupting to call the action from the notification, as for example the mark as read-action of messengers, since you've already read the message in the notification window and do not need to open the messenger.
I know that there is dunstctl context, but I only got it working with keyboard-shortcuts and not mouse buttons in i3, so I thought adding context and context-all would be the easiest way to solve the problem.

[fwsmit]

In my experience and as stated in the docs, do_action does not only check whether there is only one action but also whether one of the actions is marked as default. In many programs I am using, the default action is to bring the application window to the front so that I can directly interact with the application, while it would be much faster and less interrupting to call the action from the notification, as for example the mark as read-action of messengers, since you've already read the message in the notification window and do not need to open the messenger.
I know that there is dunstctl context, but I only got it working with keyboard-shortcuts and not mouse buttons in i3, so I thought adding context and context-all would be the easiest way to solve the problem.

Okay that makes sense. context-all would be somewhat useful then to mark multiple notifications as read slightly quicker (although you would still have to go through multiple dmenu prompts).
I still think that it might be confusing for users to have two settings that do almost the same thing. I have a proposal that wouldn't be too hard to implement, but make it a lot more useful. My idea is to add a few options to control. The options would be named something like use_default, which is a boolean for if the default action should be immediately taken and action_number which would allow to set an action number (or maybe a name) to immediately take. These two could be made rules so you can set your preferences per application (or per notification even).
I haven't completely thought it out yet, but does that sound like a useful addition to your proposal?

[lukasrad02]

This sounds like a good way to solve the problem, but let me make sure I understood it correctly:
In addition to mouse_middle_click = do_action, one could specify use_default and action_name (or number). If use_default is set to true, the behaviour would not differ from the current implementation of do_action. Otherwise, the action named action_name would be executed, or, if not found, the context menu will be opened.
This would make it even faster, if you always choose the same action (e.g. mark as read), and is also suitable for users who want a mouse action to always open the context menu.
The only question left is, whether the context menu opened should list the actions of all visible notifications (as it does so far) or only of the notification that was clicked (as my new context action does)? I would suggest to show the notification-specific menu, since you can only trigger one action at once, but at least the menu would have less actions and therefore be clearer.

I could imagine implementing this, but I would have to take a closer look at the code first, since I'm still a beginner.

[fwsmit]

This sounds like a good way to solve the problem, but let me make sure I understood it correctly:
In addition to mouse_middle_click = do_action, one could specify use_default and action_name (or number). If use_default is set to true, the behaviour would not differ from the current implementation of do_action. Otherwise, the action named action_name would be executed, or, if not found, the context menu will be opened.

Correct. I actually thought of a better way to do it. We can scrap use_default and instead make sure action_name=default will select the default action. If the action name is not found, the context menu will be opened indeed. Also, if 1 action is found it will be automatically selected.
context would still be useful if you wanted to open the context menu with the right click and do the default action with middle click` (unfortunately rules aren't flexible enough to match specific actions yet). I think it will be worth adding it, since dunst is now quite lacking in the action implementation.

This would make it even faster, if you always choose the same action (e.g. mark as read), and is also suitable for users who want a mouse action to always open the context menu.
The only question left is, whether the context menu opened should list the actions of all visible notifications (as it does so far) or only of the notification that was clicked (as my new context action does)? I would suggest to show the notification-specific menu, since you can only trigger one action at once, but at least the menu would have less actions and therefore be clearer.

Oh I didn't know it did that. I think the way you did it makes way more sense.

I could imagine implementing this, but I would have to take a closer look at the code first, since I'm still a beginner.

That would be great! I can of course guide you through. One hint I will give already is that you should take a look at notification_do_action in src/notification.c.

[fwsmit]

Thank! Let me know if you want to implement action_name as well

[lukasrad02]

Thank! Let me know if you want to implement action_name as well

I made me familiar with the code and I think that I can do this.

[lukasrad02]

I can of course guide you through.

@fwsmit I have some questions concerning the new feature. What way may I contact you to not bloat this PR with more off-topic messages?

[fwsmit]

I can of course guide you through.

@fwsmit I have some questions concerning the new feature. What way may I contact you to not bloat this PR with more off-topic messages?

You can just go ahead and ask them here. It's not off topic since you can just add it to this PR. Only if it's private (which I don't expect) you can contact me by mail.

[lukasrad02]

I thought it might be a useful addition to split action_name into an option for each mouse button, so that one can set up different action names for the different mouse buttons. Would it make sense to add such a feature and, if yes, is there a better way than just adding left_action_name, middle_action_name and so on?

[fwsmit]

I thought it might be a useful addition to split action_name into an option for each mouse button, so that one can set up different action names for the different mouse buttons. Would it make sense to add such a feature and, if yes, is there a better way than just adding left_action_name, middle_action_name and so on?

It's true that there is are possibilities missing when you don't implement action_name per button. That's why I still want context to be added. If you don't, you can't have one button select an particular action and open the context menu with another button. If action_name is implemented per button (and as a rule, so per notification), then context isn't needed anymore.
If there isn't an immediate use case for per-button action_name, then I don't think we should implement it the way you mention. I've been thinking if we can implement the mouse buttons as a matching rule so that you can say something like

[mouse_right_mpd]
appname = "mpd"
button = mouse_right_release
action_name = "default"

[mouse_middle_mpd]
appname = "mpd"
button = mouse_middle_release
action_name = "mark_read"

I'm not sure if that would work though, because rules are currently only evaluated upon notification creation.

I'd say, keep it to a simple action_name. We could always expand it later. We might consider marking context as experimental, since it will eventually be superseded by action_name.
Edit: I've opened a separate issue about that #853.

[lukasrad02]

I've implemented the feature and can push the commits if you want, but while testing I've noticed some case where I'm not sure how to handle: First, if no action named action_name can be found, should dunst open the context menu, invoke the default action or do nothing? I think opening the context menu would be the best way, since the user can see which action names are available and fix the config right away, but I'm also open for other ideas.

The second problem is trickier: So far, I've only changed the string "default" in

dunst/src/notification.c

Lines 653 to 654 in 3acffdb

	if (g_hash_table_contains(n->actions, "default")) {
	signal_action_invoked(n, "default");

to the action name variable, so urls are still handled independently from actions. At the moment, dunst would simply open the notification's url in the browser if there are no actions, no matter whether action_name is set to "default" or some other string. In case of "default" this might be intended, but in other cases not. I first thought about defining action_name = "url" to always open the url, but this might break notifications of applications using "url" as action name.
Do you have some suggestions how to solve this or should we leave it as it is, since this problem will only occur when there are wrong action names in the config file and not regularly?

[fwsmit]

I've implemented the feature and can push the commits if you want, but while testing I've noticed some case where I'm not sure how to handle: First, if no action named action_name can be found, should dunst open the context menu, invoke the default action or do nothing? I think opening the context menu would be the best way, since the user can see which action names are available and fix the config right away, but I'm also open for other ideas.

Yes you're right, just open the context menu. That seems like the most user friendly way.

The second problem is trickier: So far, I've only changed the string "default" in

dunst/src/notification.c

Lines 653 to 654 in 3acffdb

	if (g_hash_table_contains(n->actions, "default")) {
	signal_action_invoked(n, "default");

to the action name variable, so urls are still handled independently from actions. At the moment, dunst would simply open the notification's url in the browser if there are no actions, no matter whether action_name is set to "default" or some other string. In case of "default" this might be intended, but in other cases not. I first thought about defining action_name = "url" to always open the url, but this might break notifications of applications using "url" as action name.
Do you have some suggestions how to solve this or should we leave it as it is, since this problem will only occur when there are wrong action names in the config file and not regularly?

Hmm right. Those are handled a bit weirdly atm. I'd say we should have a separate open_url and remove it from the do_action function. It would be great if you could do it, but it'll be fine if you leave it as well.

Good that you're making progress, and it sounds like you're testing it pretty well. You can go ahead and push your commits, I won't merge it before it's reviewed :)

[codecov-commenter]

Codecov Report

Merging #848 (a5ab5ec) into master (3acffdb) will decrease coverage by 0.18%.
The diff coverage is 8.57%.

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
- Coverage   58.54%   58.35%   -0.19%     
==========================================
  Files          37       37              
  Lines        5987     6011      +24     
==========================================
+ Hits         3505     3508       +3     
- Misses       2482     2503      +21     

Flag	Coverage Δ
unittests	58.35% <8.57%> (-0.19%)	⬇️

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

Impacted Files	Coverage Δ
src/input.c	0.00% <0.00%> (ø)
src/menu.c	14.28% <0.00%> (-0.18%)	⬇️
src/option_parser.c	85.40% <0.00%> (-0.70%)	⬇️
src/settings.c	58.33% <0.00%> (-0.20%)	⬇️
src/notification.c	58.97% <11.76%> (-0.72%)	⬇️
src/rules.c	48.57% <33.33%> (-0.69%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3acffdb...a5ab5ec. Read the comment docs.

[lukasrad02]

open_url and action_name are implemented now. One last thing might be to split up the context menu into one menu for actions and one for urls, but I thought this would lead to many necessary changes while not being an important feature.

Edit: I found some issue that might lead to null pointers or memory leaks, I'll fix it within the next days.

[lukasrad02]

I found some issue that might lead to null pointers or memory leaks, I'll fix it within the next days.

The issue is fixed now. I also re-added the code to invoke the only available action if action_name is set to default.

[fwsmit]

Can you squash those last two commits into the right one? After that it LGTM. Thank you!

[tsipinakis]

Sorry for how long it took to get to this @lukasrad02. I'll now hopefully have a bit more time to actually take care of maintainer duties. Thanks for the PR and the clean code :)