Support buttons with action group by leolost2605 · Pull Request #263 · elementary/panel-notifications

leolost2605 · 2023-06-29T09:17:51Z

Works with Use GLib.ActionGroup to handle actions notifications#193

…tary/wingpanel-indicator-notifications into support-buttons-with-action-group

Marukesu · 2023-07-20T16:27:53Z

src/Widgets/NotificationEntry.vala

+        button_release_event.connect (() => {
+            unowned var action_group = get_action_group (ACTION_GROUP_PREFIX);
+            if (action_group.has_action (notification.default_action)) {
+                notification.app_info.launch_action (Notification.DEFAULT_ACTION, new GLib.AppLaunchContext ());


this is wrong, notifications actions aren't desktop actions.

Hmm I took that from the previous implementation. Maybe it's relied upon by an elementary app even though it's not in the spec? Or maybe @danirabbit can give background on that?

I’m not sure what is meant by “notifications aren’t desktop actions” but this is meant to support https://valadoc.org/gio-2.0/GLib.Notification.set_default_action.html

I’m not sure what is meant by “notifications aren’t desktop actions”…

a action name in the actions parameter of org.freedesktop.Notifications.Notify method aren't a "exported" action (a action in the org.gtk.Actions interface in the applications's object path), or a desktop action.

GLib.DesktopAppInfo.launch_action() is meant to launch desktop actions only (can be abused to launch exported actions because of how GLib.Application is written, but won't work for non-GLib.Application applications).

…but this is meant to support https://valadoc.org/gio-2.0/GLib.Notification.set_default_action.html

when GLib.Notification uses the org.freedesktop.Notifications interface, it will use GLib.Action.print_detailed_name() for buttons, and "default" for the default action. so we will never receive a real action that could be used with GLib.DesktopAppInfo.launch_action(), only false positives.

It only launches the AppInfo now without an action. I think that's something we want or shouldn't we launch it at all? IMHO it's probably expected to bring the app to foreground when the notification is clicked on regardless of default action or no action.

src/Widgets/NotificationEntry.vala

…her actions

leolost2605 · 2023-07-24T19:00:49Z

Sorry @Marukesu I thought I hadn't requested a review yet

src/Indicator.vala

src/Services/NotificationsMonitor.vala

src/Widgets/AppEntry.vala

src/Widgets/NotificationEntry.vala

src/Widgets/AppEntry.vala

Marukesu · 2023-07-27T22:37:30Z

I don't think this depend on the Session removal…

Thinking better, i take this back.

The session file store notifications by id, ids aren't meant to be persistent and are reused by the server after a restart, that makes the indicator unreliable to deal with removing/closing/activating a notifications because of colliding ids.

the fact this doesn't cause much problems currently is because the indicator don't report back notification closing, neither support action activation of any type (the current default action code is broken). but once this get merged theses issues will start to be recurrent.

Store notifications by an internal id with the format "timestamp.server id" Don't store notification actions Don't emit signals for notifications loaded from the last session Use notification_closed with UNDEFINED for closing the notificiation when an action was invoked

leolost2605 · 2023-08-21T13:04:21Z

Notifications are now stored by an internal id consisting of timestamp.server_id.
If we load a notification from the session file the server_id will be 0 and no signals will be emitted. If actions exist the buttons will be insensitive (I thought about not storing actions at all but i think this might be clearer as otherwise we might end up with a situation like "Do you want to allow this?" and you've got nothing to click on. Another solution might be to not store notifications with actions at all 🤷)
Requires elementary/notifications#211

src/Widgets/NotificationsList.vala

Marukesu · 2023-08-21T20:24:00Z

src/Widgets/NotificationsList.vala

+            try {
+                notification_entry.notification.app_info.launch (null, null);

-            close_popover ();
+                notification_entry.clear ();
+                close_popover ();
+            } catch (Error e) {
+                warning ("Unable to launch app: %s", e.message);
+            }


this should be in a else block and close_popover() should be called after it.

But if it's in a else block it can't do

It only launches the AppInfo now without an action. I think that's something we want or shouldn't we launch it at all? IMHO it's probably expected to bring the app to foreground when the notification is clicked on regardless of default action or no action.

right? Do we still want it?

We want to call GLib.AppInfo.launch() only when there's no default action, the current code always call it.

Marukesu · 2023-08-21T20:59:59Z

If actions exist the buttons will be insensitive (I thought about not storing actions at all but i think this might be clearer as otherwise we might end up with a situation like "Do you want to allow this?" and you've got nothing to click on.

i like the idea of the deactivated buttons, i think you could drop the actions but not the label from the session file, and simply don't try to validate anything when restoring. that way we don't risk ending re-enabling a button because the new notification for the same id has a matching action name.

leolost2605 · 2023-08-24T19:47:17Z

i like the idea of the deactivated buttons, i think you could drop the actions but not the label from the session file, and simply don't try to validate anything when restoring. that way we don't risk ending re-enabling a button because the new notification for the same id has a matching action name.

The server id will be 0 and therefore the action always invalid so AFAIK there is no way we could re-enable it by accident. If you want me to do it anyway because of a bit less space used I'd be happy to update it but it'd require quite a bit more code because without we only have to handle a single way buttons are given and I don't think it's really worth it 🤷

Marukesu

LGTM! Nice work.

danirabbit · 2023-08-29T22:07:56Z

🎉 amazing love it

leolost2605 and others added 10 commits June 26, 2023 20:56

Protoype buttons with action group

8c5bc90

Work with new format

5724ce4

Fix issue

3218564

Fix lint

245a0b2

Merge branch 'master' into support-buttons-with-action-group

8b52665

Remove unused interface

ee0d13a

Fix issue

b7e5b72

Change name that was forgotten

5ab45a5

Merge branch 'support-buttons-with-action-group' of github.com:elemen…

36e5492

…tary/wingpanel-indicator-notifications into support-buttons-with-action-group

Update action target

d8ec402

leolost2605 mentioned this pull request Jul 7, 2023

Use GLib.ActionGroup to handle actions elementary/notifications#193

Merged

leolost2605 and others added 2 commits July 7, 2023 12:40

Skip actions without label and close with clear all

ddb2271

Merge branch 'master' into support-buttons-with-action-group

aa930a1

leolost2605 marked this pull request as ready for review July 7, 2023 10:44

Fix closing when it shouldn't be closed

5f9d797

leolost2605 mentioned this pull request Jul 20, 2023

Don't expire notifications when the timeout expires elementary/notifications#211

Merged

Marukesu reviewed Jul 20, 2023

View reviewed changes

src/Widgets/NotificationEntry.vala Outdated Show resolved Hide resolved

Marukesu reviewed Jul 20, 2023

View reviewed changes

src/Widgets/NotificationEntry.vala Outdated Show resolved Hide resolved

src/Widgets/NotificationEntry.vala Outdated Show resolved Hide resolved

src/Widgets/NotificationEntry.vala Outdated Show resolved Hide resolved

src/Widgets/NotificationEntry.vala Outdated Show resolved Hide resolved

leolost2605 added 2 commits July 20, 2023 18:50

Don't pack the action area in the loop

6e38207

Insert action group in the notifications list

888a0c0

leolost2605 requested a review from Marukesu July 20, 2023 17:59

leolost2605 and others added 7 commits July 21, 2023 12:19

Simplify

8b374b3

Close notification on action invoked

a5fcb28

Cleanup

43e7a26

Fix an issue that default action can't be activated when there are ot…

98e4780

…her actions

Fix unowned issue and close issue

2d643b0

Merge branch 'master' into support-buttons-with-action-group

4585fb2

Only clear when the app is succesfully launched

e69a335

leolost2605 removed the request for review from Marukesu July 24, 2023 18:45

Marukesu reviewed Jul 24, 2023

View reviewed changes

Apply suggestion from code review

3c3384c

leolost2605 added 5 commits August 20, 2023 14:23

Improvements

6d17586

Store notifications by an internal id with the format "timestamp.server id" Don't store notification actions Don't emit signals for notifications loaded from the last session Use notification_closed with UNDEFINED for closing the notificiation when an action was invoked

Cleanup

bae9c20

Separate internal id from server id

1460e96

Cleanup + store actions

ea744cf

Fix internal_id

6e249c4

leolost2605 added 2 commits August 21, 2023 15:09

Style like button box

516f13b

Keep order of actions

71944d2

leolost2605 requested a review from Marukesu August 21, 2023 13:49

Use warning instead of critical

2905d1b

Marukesu reviewed Aug 21, 2023

View reviewed changes

Don't check for actions on row activated

99ac878

leolost2605 force-pushed the support-buttons-with-action-group branch from d7176ca to 99ac878 Compare August 24, 2023 19:53

Put app launch in else block

72ab83a

leolost2605 requested a review from Marukesu August 25, 2023 20:01

leolost2605 and others added 2 commits August 29, 2023 23:36

Merge branch 'master' into support-buttons-with-action-group

7bc4384

Fix merge

bf465da

Marukesu approved these changes Aug 29, 2023

View reviewed changes

danirabbit merged commit 35c0889 into master Aug 29, 2023

danirabbit deleted the support-buttons-with-action-group branch August 29, 2023 22:08

This was referenced Aug 29, 2023

Make clicking on notifications meaningful #239

Closed

Click on missed notification doesn't open corresponding application #181

Closed

Enable notification action on notification center #216

Closed

Uh oh!

Conversation

leolost2605 commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leolost2605 commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marukesu commented Jul 27, 2023

Uh oh!

leolost2605 commented Aug 21, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marukesu commented Aug 21, 2023

Uh oh!

leolost2605 commented Aug 24, 2023

Uh oh!

Marukesu left a comment

Choose a reason for hiding this comment

Uh oh!

danirabbit commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leolost2605 commented Jul 24, 2023 •

edited

Loading