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.

Sign up for GitHub

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

Jump to bottom

notification: Set correct platform data for action activation #468

Closed
wants to merge 2 commits into from
Closed

notification: Set correct platform data for action activation #468

wants to merge 2 commits into from

Conversation

jsparber
Copy link
Contributor

@jsparber jsparber commented Feb 19, 2024 •
edited
Loading

Since [1] FDO notifications can transfer a ActivateToken to the client application, we can use this token to set the correct platform data to get wayland startup notification working correctly. GNOME Shell gained the feature already in [2], so this is the last piece missing to get rid of the annoying " is ready" notifications, when clicking on a notification.

We can set the platform data for actions that are activated via org.freedesktop.Application.
For apps that don't implement the org.freedesktop.Application interface we can add the
platform data to the ActionInvoked signal.

Fixes: #406

[1] https://gitlab.freedesktop.org/xdg/xdg-specs/-/commit/b9a470004d
[2] https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/3199/

TingPing
TingPing reviewed Feb 19, 2024
View reviewed changes
src/notification.c Outdated Show resolved Hide resolved
@jsparber
Copy link
Contributor Author

jsparber commented Feb 19, 2024 •
edited
Loading

I didn't actually try the code yet since i didn't find a way to use it without writing a test app therefore it's marked as draft.

@TingPing
Copy link
Member

GNOME Shell gained the feature already

On GNOME-Shell it shouldn't be using org.freedesktop.Notifications anyway, it will use org.gtk.Notifications.

@TingPing
Copy link
Member

Ah, it does use FDO in some fallback paths.

@jsparber jsparber force-pushed the add_fdo_notification_activation_token branch from 7eae929 to 2725627 Compare March 1, 2024 16:12
@jsparber jsparber marked this pull request as ready for review March 1, 2024 16:13
@jsparber
Copy link
Contributor Author

jsparber commented Mar 1, 2024 •
edited
Loading

I figured out how to try this. We can just monitor DBUS and use ashpd demo.

@jsparber jsparber force-pushed the add_fdo_notification_activation_token branch from 2725627 to 2d8d831 Compare March 1, 2024 16:25
@jsparber
Copy link
Contributor Author

jsparber commented Mar 1, 2024

Oh i didn't notice before doing this that there was already an effort few years back by @3v1n0 with #380 I think this is a better approach since we don't break or change any API.

@jsparber jsparber force-pushed the add_fdo_notification_activation_token branch from 2d8d831 to 47bfdab Compare March 1, 2024 16:38
3v1n0
3v1n0 reviewed Mar 1, 2024
View reviewed changes
Copy link

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why we decided to change the API was because I also wanted to make possible for apps using GNotification or portal api directly to be able to send such info, and not being limited to using the fdo API

@jsparber
Copy link
Contributor Author

jsparber commented Mar 1, 2024

The reason why we decided to change the API was because I also wanted to make possible for apps using GNotification or portal api directly to be able to send such info, and not being limited to using the fdo API

If an action is activated via org.freedesktop.Appliaction.ActivateAction the platform data needs to be set by the caller. In our case GNOME Shell, I fixed that see https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/3198. Gio/GTK.Application already picks up the platform data (I'm not sure for other toolkits).

ebassi
ebassi reviewed Mar 2, 2024
View reviewed changes
Copy link
Collaborator

@ebassi ebassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good to me, with a couple of minor nitpicks.

src/notification.c Outdated Show resolved Hide resolved
src/notification.c Outdated Show resolved Hide resolved
src/fdonotification.c Show resolved Hide resolved
@jsparber
notification: Set correct platform data for action activation
6d1d0b1 
Since [1] FDO notifications can transfer a `ActivateToken` to the client
application, we can use this token to set the correct platform data to
get wayland startup notification working correctly.
GNOME Shell gained the feature already in [2], so this is the last piece
missing to get rid of the annoying "<Application> is ready" notifications,
when clicking on a notification.

We can set the platform data only for actions that are activated via
`org.freedesktop.Application`.

Fixes: flatpak#406

[1] https://gitlab.freedesktop.org/xdg/xdg-specs/-/commit/b9a470004d
[2] https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/3199/
@jsparber jsparber force-pushed the add_fdo_notification_activation_token branch from d069cf5 to 760cfdd Compare March 2, 2024 23:17
jsparber
jsparber commented Mar 3, 2024
View reviewed changes
src/notification.c Outdated Show resolved Hide resolved
gnomesysadmins pushed a commit to GNOME/libnotify that referenced this pull request Mar 3, 2024
@jsparber
notification: Get the activation-token form ActionInvoked when using …
6c83f4b 
…portal

Since [1] the xdg-desktop-portal-gtk adds the activation-token to the
ActionInvoked signal. We can store it and let apps use it like the
activation-token we get from FDO.

[1] flatpak/xdg-desktop-portal-gtk#468
@jsparber
Copy link
Contributor Author

jsparber commented Mar 3, 2024

Once this is merged we can consume the activation-token passed to ActionInvoked from libnotify https://gitlab.gnome.org/GNOME/libnotify/-/merge_requests/38

@jsparber
notification: Add platform data to ActionInvoked signal
c589c7f 
Apps may not implement the `org.freedesktop.Application` interface so
they won't get the platform data. We can do a little better and add the
platform data to the `ActionInvoked` signal.
@jsparber jsparber force-pushed the add_fdo_notification_activation_token branch from 760cfdd to c589c7f Compare March 4, 2024 12:11
gnomesysadmins pushed a commit to GNOME/libnotify that referenced this pull request Mar 4, 2024
@jsparber
notification: Get the activation-token form ActionInvoked when using …
910ffe0 
…portal

Since [1] the xdg-desktop-portal-gtk adds the activation-token to the
ActionInvoked signal. We can store it and let apps use it like the
activation-token we get from FDO.

[1] flatpak/xdg-desktop-portal-gtk#468
@TingPing
Copy link
Member

I believe this was merged as 48bbf07

@TingPing TingPing closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebassi ebassi ebassi left review comments

@3v1n0 3v1n0 3v1n0 left review comments

@swick swick swick left review comments

@TingPing TingPing TingPing left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

org.freedesktop.Notifications.ActivationToken support
5 participants
@jsparber @TingPing @ebassi @3v1n0 @swick