notification: Add platform-data (with activation_token) to ActionInvoked #791
Conversation
3v1n0
force-pushed
the
notification-activation-token
branch
4 times, most recently
from
ea62c00
to
0b4aca4
Compare
|
@matthiasclasen wdyt about this? At the current time I decided to do an api-break on libportal too, otherwise it could be exposed as an another signal, but it looked overkill to me. |
3v1n0
force-pushed
the
notification-activation-token
branch
from
0b4aca4
to
5c60d2e
Compare
|
I wanted to add a test for checking on the old interface too, but to use libportal there too, we'd need to make a way to override the proto version, and sadly that can't be done once we've the proxy up. So can only test it using the dbus API. |
3v1n0
changed the title
3v1n0
force-pushed
the
notification-activation-token
branch
5 times, most recently
from
f43b77a
to
7950c64
Compare
Since some time FDO Notifications introduced the ActivationToken signal to ensure that clients will receive an X11-startup notification ID or a wayland xdg-activation token in order to support stealing-focus prevention protocols. However this isn't supported by xdg-desktop-portal's. So fill the gap by adding a further ActionInvoked signal that includes platform data, where such parameter can be exposed. As per this, bump the protocol version.
Ensure we don't regress in handling version 1 of the notification protocol, we can't do it through libportal as it's checking for the notification version early, we can still ensure all is handled as we expected.
3v1n0
force-pushed
the
notification-activation-token
branch
from
7950c64
to
80e5c1f
Compare
|
Hello, whats the state of this PR? Does the gnome shell know what to do with the activation token, so I can try testing this? |
| <arg type="s" name="app_id"/> | ||
| <arg type="s" name="id"/> | ||
| <arg type="s" name="action"/> | ||
| <arg type="a{sv}" name="platform_data"/> |
There was a problem hiding this comment.
Perhaps name this options? It's what the equivalent thing is called in other portals.
| <signal name="ActionInvoked2"> | ||
| <arg type="s" name="id"/> | ||
| <arg type="s" name="action"/> | ||
| <arg type="a{sv}" name="platform_data"/> |
There was a problem hiding this comment.
Same about calling it options for consistency.
| <arg type="s" name="action"/> | ||
| <arg type="a{sv}" name="platform_data"/> | ||
| <arg type="av" name="parameter"/> | ||
| </signal> | ||
| <property name="version" type="u" access="read"/> |
There was a problem hiding this comment.
Can we not bump the version, here? If version == 2, then ActionInvoked has more parameters, otherwise it doesn't.
Unless we assume that all portal implementations aren't checking the version.
I really don't want to have methods called Foo2; that's a great way to cause legibility issues, as well as the potential for typos. Might as well call this signal with another name entirely.
There was a problem hiding this comment.
That's not how it works; the apps don't "bind" a version it's compatible with, so we cannot change the type signature.
How do you imagine "app1" that is using the current version of the API to continue to work with a new xdg-desktop-portal if the type signature changes?
I think we just have to live with the fact that there will be a "Foo2" here and there. It's also why it's good to more or less always have a vararg so one can add more properties like this without changing the type signature.
Since some time FDO Notifications introduced the ActivationToken signal
to ensure that clients will receive an X11-startup notification ID or a
wayland xdg-activation token in order to support stealing-focus
prevention protocols.
However this isn't supported by xdg-desktop-portal's.
So fill the gap by adding a further activation token.
While it could be added by as an optional parameter, it's just better to
make it explicit.
It needs libportal from flatpak/libportal#88
Implemented in: