-
-
Notifications
You must be signed in to change notification settings - Fork 470
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
feat(notifications): linux notification actions #5976
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
if (action == OPEN_IN_BROWSER) | ||
{ | ||
toastReaction = ToastReaction::OpenInBrowser; | ||
} | ||
else if (action == OPEN_PLAYER_IN_BROWSER) | ||
{ | ||
toastReaction = ToastReaction::OpenInPlayer; | ||
} | ||
else if (action == OPEN_IN_STREAMLINK) | ||
{ | ||
toastReaction = ToastReaction::OpenInStreamlink; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a function findReactionFromString
next to findStringFromReaction
. At this point, we could also use qmagicenum.
switch (toastReaction) | ||
{ | ||
case ToastReaction::OpenInBrowser: | ||
QDesktopServices::openUrl( | ||
QUrl(u"https://www.twitch.tv/" % *channelName)); | ||
break; | ||
case ToastReaction::OpenInPlayer: | ||
QDesktopServices::openUrl( | ||
QUrl(TWITCH_PLAYER_URL.arg(*channelName))); | ||
break; | ||
case ToastReaction::OpenInStreamlink: { | ||
openStreamlinkForChannel(*channelName); | ||
break; | ||
} | ||
case ToastReaction::DontOpen: | ||
// nothing should happen | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as in the windows code. It should be in some common function.
// we only set onActionDestroyed as free_func in the first action because | ||
// all free_funcs will be called once the action is destroyed which would | ||
// cause a double-free otherwise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should mention that the free_func
is called when the notification is destroyed (as mentioned in the docs). The action
is only a string.
An alternative would be an intrusive pointer where a reference count is embedded into the object (both Qt and Boost have one; glib might have something too).
@@ -63,6 +63,63 @@ class AvatarDownloader : public QObject | |||
void downloadComplete(); | |||
}; | |||
|
|||
#ifdef CHATTERINO_WITH_LIBNOTIFY | |||
void onAction(NotifyNotification *notif, const char *actionRaw, void *user_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be userData
notify_notification_close(notif, nullptr); | ||
} | ||
|
||
void onActionClosed(NotifyNotification *notif, void * /*user_data*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change commented out user_data
to userData
here too
@@ -322,8 +411,9 @@ void Toasts::sendLibnotify(const QString &channelName, | |||
g_object_unref(img); | |||
} | |||
|
|||
g_signal_connect(notif, "closed", (GCallback)onActionClosed, nullptr); | |||
|
|||
notify_notification_show(notif, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle error here to ensure we free things
Adds actions to the live notification on Linux. Because libnotify supports adding multiple actions the behavior is as follows:
The default action is selected in the settings page (same dialog as on Windows). We add the default action and the 3 actions as action.
The behavior then depends on the notification client. Two examples:
The documentation for Dunst says:
On Gnome the default actions is performed when clicking the notification and the other options are displayed as buttons when you hover over the notification.
Let me know if you like this approach or would rather only have one default option that is specified in the settings.
Fixes #5937
Further notes:
channelNameHeap
is the best way to pass the channel name to theonAction
callback