Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to run script when notification closes #753

Open
josefandersson opened this issue Aug 15, 2020 · 14 comments
Open

Option to run script when notification closes #753

josefandersson opened this issue Aug 15, 2020 · 14 comments
Labels

Comments

@josefandersson
Copy link

The current "script" option is great, but only runs when the notification is created. I propose adding a feature to run a script when the notification is clicked or closed by either:

  • Adding a "close_script" option similar to the "script" option, but that is also passed the mouse_action and reason as arguments. See my implementation of this here: master...josefandersson:master
  • Running the script specified by the "script" option when the notification closes as well, but adding a "created" or "closed" argument along with the mouse_action and reason, or similar.

The latter is not preferred though since it would destroy compatibility with a lot of old scripts.

While I do realize there exists "half workarounds" (#163) like using the dunstify command with the current "script" option to recreate the notification and then evaluate the result I do think a better solution is needed. For example, if the notification is using raw image data for the icon it won't be passed to the script and thus a full recreation won't be possible unless I'm missing something.

@tsipinakis
Copy link
Member

What would be the usecase for this? Can you provide a few examples?

Additionally, closes is a bit ambiguous as there are many cases where a notification is closed but it may not be clear why or even that a notification was closed.

Here are some events I can think of on the top of my head that close a notification, which of these would trigger this script in your opinion?

  • When the user dismisses a notification via mouse/keyboard
  • When a notification times out
  • When an application closes a notification (NotificationClose signal)
  • When a notification is stacked with another as duplicate? (Internally, the first notification is closed and the newer is preserved)
  • When a notification is replaced with another?

@josefandersson
Copy link
Author

@tsipinakis
I use my implementation of it to, when the notification is clicked, activate the proper window with xdotool when the application has a poor notification implementation, like RuneLite, that uses notify-send to send notifications. I also have a problem with Discord not focusing sometimes but it may just be the custom client I'm using, either way I use a script to focus correctly for it as well. I'm sure there are other ingenious thing's you could do with it that I can't think of.

Is it really ambiguous though? Either the notification exists or it doesn't, and when that change happens, from existing to not, the script is called? So it would run for all the cases you mentioned and pass the reason of 'closure' to the script, pretty much like I said. And it would always only run once for every notification. I only called it "close" because that was the function I hooked to first, the queues_notification_close_id function in queues.c which "closes" the notification with id.

@tsipinakis
Copy link
Member

tsipinakis commented Aug 18, 2020

I use my implementation of it to, when the notification is clicked, activate the proper window with xdotool when the application has a poor notification implementation, like RuneLite, that uses notify-send to send notifications.

I've been interested in something similar but haven't really looked into it. However, I think this is the wrong approach. i.e. abusing the close notification action as a 'clicked' event.

I think the cleanest solution here would be implementing #669 or something similar. This has the advantage that it would also call the default actions some programs include, for example in an email client it opens up the email the notification was about.

This is how I think this would work configuration-wise:

[my-action-rule]
summary = "*" # Match all notifications
action_script = +default:Activate:/path/to/script

Breaking this down this means:

  • +: If a default action doesn't exist, add one titled Activate
  • Whenever the default action is used call the script

It's issues like this that make me wish dunst used yaml as a configuration language instead of ini which would make specifying multiple variables like that easier rather than having to implement our own format.

@josefandersson
Copy link
Author

josefandersson commented Aug 20, 2020

@tsipinakis What do you mean "abusing the close notification action as a 'clicked' event"? It's not running as or claimed to be a 'clicked' event, it's a 'closed' event, thus called when the notification is closed, just like "signal_notification_closed" in dbus.c is called when the notification is closed to tell the owner of the notification that the notification is closed, it's got nothing to do with clicked?

What you are proposing is a step in the right direction, but in practice it would be the same thing as I said- except for it adding the default action and not being as capable by not also being run when the action isn't used. The former would be easily implemented by the user via a custom script either way (and would even be better since you can process the data yourself before doing something with it...) so going through the hassle of creating the feature for dunst and making the config hella confusing with that formatting seems unnecessary?

Adding a close_script run by my description from config still sounds the most reasonable to me:

With the "close_script" option you may also specify a script to run when the notification is closed. The script is called as follows:
   script appname summary body icon urgency mouse_action reason
where urgency can be "LOW", "NORMAL" or "CRITICAL" and
where mouse_action can be "NONE", "DO_ACTION", "CLOSE_CURRENT" or "CLOSE_ALL" and
where reason can be "TIME", "USER", "SIG" or "UNDEF".

This would be the final feature needed for custom script running as there would be no scenario not encapsulated by this feature or the 'script' option, it's easy to implement and the configuration for it is as simple as it gets.

@tsipinakis
Copy link
Member

tsipinakis commented Aug 20, 2020

What you are proposing is a step in the right direction, but in practice it would be the same thing as I said- except for it adding the default action and not being as capable by not also being run when the action isn't used.

This could be used to add any action not just the default, as mentioned in issue #669. For example, add a 'Copy code' action to copy 2FA codes from any notification, or add play/pause/skip etc actions to media player notifications that don't already implement them.
The difference being you can have as many actions as you want in a notification rather than the single close event that you can't differentiate on the intent (did I want to close that, or copy the 2FA code etc).

However I do agree that the config format I proposed is too complicated so it'll have to be revised somehow if it's implemented.

where mouse_action can be "NONE", "DO_ACTION", "CLOSE_CURRENT" or "CLOSE_ALL" and

Since 1.5 we allow multiple actions for mouse events, how would this deal with middle_click = do_action, close_current (which is the default)? Is the script called twice?

I guess this could be similar if in do_action case we also passed the name of the action that was chosen so the script is called for all actions in a notification without any configuration. It would greatly simplify the config. The usecase mentioned above can be satisfied with a new option to simply add custom actions and use the feature you propose to bind functionality to them.

@josefandersson
Copy link
Author

@tsipinakis That's a bit more than just adding an action if it doesn't exist like in your previous comment though? And again, harder implementation and confusing configuration. It would be nice to just be able to add a bunch of actions like you say if they don't already exist on the notification but that could be done easily and more 'customizably' by the user via script. It would however be nice to, with configuration format you mentioned, also be able to remove (-) default actions. Though I have no use case for that personally.

You can differentiate between intent since it's passed to the script?

Mine passes the first action (do_action in this case) and then sets the n->close_script_run to true which prevents it from running again for that notification. I guess you could also check settings.always_run_script and run multiple times like it's done for the 'script' option scripts.

@tsipinakis
Copy link
Member

It would be nice to just be able to add a bunch of actions like you say if they don't already exist on the notification but that could be done easily and more 'customizably' by the user via script

You can differentiate between intent since it's passed to the script?

mouse_action is passed to the script, but not the action name so you can't differentiate between multiple actions is my point. So you can basically only do one thing for each notification and not give a list of options as dunst does with the context/action selection menu (unless you add a menu to the script itself). My example was wrong.

Now with what you've said and that I've thought this through the config would look like this in my mind:

[my-action-rule]
summary = "*"
add_action = Copy 2FA code
remove_action = default
action_script = /path/to/script

action_script being the same as the close_script you suggest with a more descriptive name.

So in case any action is chosen action_script is called with do_action action and the $DUNST_ACTION_NAME variable or similar set to Copy 2FA code.

This, minus the 2 extra rules is basically what you have implemented in close_script minus the variable for the action name. So I believe we are pretty much saying the same thing.

@josefandersson
Copy link
Author

@tsipinakis When you say action you are still talking the notification action (like Activate) right? I don't ever use the context menu myself so the usefulness of having the action name passed as well escaped me.

Would action_script still be called when do_action isn't called? I.e. when the notification is closed or timed out? From the name I assume not, so why not?

Otherwise I do think being able to remove default actions is a great feature, and being able to add custom extra actions sounds great for context menu users.

@tsipinakis
Copy link
Member

When you say action you are still talking the notification action (like Activate) right?

Yes

Would action_script still be called when do_action isn't called?

Ideally, yes, we can have a script that is called for all events and filtering can be done from the script side. I'm also not sure about the name though. Neither close_script nor action_script seem good, script would be best but as you said above we don't want to break backwards compatibility.

@josefandersson
Copy link
Author

@tsipinakis Perfect. I'm more in favor of close_script since it's more descriptive than action_script (at least less misleading). Perhaps end_script, finished_script, completed_script or after_script? Would love to see these features added.

@tsipinakis
Copy link
Member

The problem with the close_script name is that it implies it's run when the notification is closed but is not always the case: You can call many actions on a notification without closing it.
And all the suggestions have that same flaw, any other ideas? I'll try to come up with something more apt by tomorrow.

@tsipinakis
Copy link
Member

Whoops sorry I totally forgot to update this. The best I came up with was event_script (i.e. it's run when an event happens on a notification).

@Midren
Copy link

Midren commented Feb 19, 2021

What is the status of the current issue? Is it possible to see it in near future?

@fwsmit
Copy link
Member

fwsmit commented Feb 19, 2021

There is no PR for this issue yet. I myself won't implement this at least until #803 is done, since that will overhaul the rules code. PR's are welcome though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants