Implement v2 notification spec #1298

jsparber · 2024-03-08T16:07:32Z

After lot of consideration I started implementing parts of the proposed and discussed notification API

For now this includes:

Set icon additionally via file descriptor
Set sound via bytes, file descriptor
desktop-file-id property
markup-body property
display-hint property
~~new actions, similar to buttons (the name may be a little confusing, open for suggestions)~~ Part of buttons now
~~content-type~~ category property
Button purpose
Supported options for ~~content-type~~ category, ~~action purpose~~ and button purpose API

This needs changes in libportal flatpak/libportal#147 for tests.

I also started writing the changes needed in xdg-desktop-portal-gtk: https://github.com/jsparber/xdg-desktop-portal-gtk/tree/implement_notification_v2 and other portal backends need to do the same thing.

Parts that didn't made it from #1304 into this MR may be added it a later revision of the portal.

Fixes: #485

Mikenux · 2024-03-08T18:11:28Z

What is the case for letting an app play a sound freely? In the discussion there was talk of linking sound playback to specific cases, e.g. alarm, new message, new email. Also, it seems to me that the question of custom sounds versus system sounds still needs to be resolved for these example cases, unless there is a valid case to allow an app to play sound freely.

jsparber · 2024-03-08T18:34:01Z

What is the case for letting an app play a sound freely?

It's not that apps are allowed to play sound freely, they are allowed to set it. The server may decided whether to play the sound. In GNOME Shell we have a policy system that controls whether a notification can have a sound or not.

In the discussion there was talk of linking sound playback to specific cases, e.g. alarm, new message, new email.

I think this doesn't need to be exclusive and I don't see how it would conflict with class specific sounds.

Also, it seems to me that the question of custom sounds versus system sounds still needs to be resolved for these example cases, unless there is a valid case to allow an app to play sound freely.

Apps already can play any sound if they have the correct sandbox permission. So we prefer that apps use the notification sound so that the system policy can be respected e.g. do-not-disturb. Although, we may want to restrict the type and length of the sound a notification can have.

ilya-fedin · 2024-03-09T00:09:38Z

What is driving me nuts in the freedesktop notification spec is that there's no way to know that system sounds are supported and which music formats are supported for custom sounds. This made the sound spec part just unusable when I attempted to play sound with it:

First I tried to set .mp3 files via sound-file. No notification daemon has played it. The .mp3 file may come from embedded resources of the application or from the cloud uploaded by the user (so pre-converting the embedded one won't really help). The application has no transcoding capabilities and there's no manpower to implement it in the application.
Then I tried to set sound-name to message-new-instant. To my surprise this worked only on GNOME and Pantheon. KDE, MATE, Xfce, LXQt, Budgie, Cinnamon, Deepin: none of them were able to play it. Deepin was always playing its own sound, even with disable-sound = true. Although, KDE and Deepin don't support the sound capability at all so their behavior is quite expected.
Ok, let's set sound-name only when it's supported and only when the user has chosen the default sound embedded in the application resources (so the user choice is still respected yet most users will get sound via notification daemon). No, you can't, there's only sound capability that says "sound-file must be supported and sound-name may be supported".

This resulted in application always playing the notification sound on its own. I really hope these problems will be solved in the new API.

data/org.freedesktop.portal.Notification.xml

Adding a notification may fail, instead of waiting for the timeout we can check the result we get in the callback.

For debugging it's important that the backend doesn't just crash without telling the reason. It would be even nicer if the test would fail immediately instead of hitting the timeout, but that' currently not possible since the client doesn't know whether the backend failed.

We can reuse most of the test setup for all tests. This will make it easier to test more properties.

This filters unsupported properties from a notification request without failing it. This allows backwards compatibility after adding new properties. This should had been the case from the beginning so that we don't have to break API when extending the notification interface. See [1]. [1] https://dbus.freedesktop.org/doc/dbus-api-design.html#extensibility

This allows setting a custom path for the icon validator, especially useful when testing without installing.

The sandbox for the icon-validator needs a privileged container. And it needs librsvg to be able to validate SVGs.

If debug messages are turned on the output of validate-icon isn't a valid key file since it contains log output as well.

Mikenux · 2024-04-13T13:57:57Z

Apps already can play any sound if they have the correct sandbox permission. So we prefer that apps use the notification sound so that the system policy can be respected e.g. do-not-disturb.

I think one thing to check is if there are static permissions that can help apps guess what mode the device is in (sound, vibration, silent, DND). For example, alarms will certainly be allowed by users, because they are logically important to them. In fact, apps that will use alarm notifications will be able to play sound (because in any mode you cannot completely mute the sound, unless it can be done selectively without dynamic permission), but especially to present a notification (even more so if this is also not constrained by specific experience and actions).

data/org.freedesktop.impl.portal.Notification.xml

data/org.freedesktop.portal.Notification.xml

jsparber · 2024-04-19T15:30:45Z

@pwithnall thanks for the review :)

src/notification.c

src/xdp-utils.c

src/notification.c

tests/notification.c

src/notification.c

swick · 2024-04-22T14:05:35Z

src/notification.c

+  GString *composed = user_data;
+  gchar *str = (gchar *) text;
+
+  while (*str)


I really don't like this entire function (its hard to tell what it is trying to achieve) but i don't have a good suggestion either.

The idea is to remove new lines and replace them with a space, without introducing multiple spaces.

tests/notification.c

swick · 2024-04-22T14:10:02Z

data/org.freedesktop.portal.Notification.xml

@@ -171,6 +171,21 @@
          * ``target`` (``v``)

            Target parameter to send along when activating the action.
+
+        * ``desktop-file-id`` (``s``)


I really doubt this is a good idea. The portal should be able to determine the app id and fetch the desktop file based on that. What are you trying to accomplish here?

This tries to solve the following:

The current API can't really be used outside of flatpak, because there isn't a reliable why to get the app id.

A single flatpak may export multiple desktop files and there isn't any way how the portal can know which desktop-file-id should be used for D-Bus activation, app name and icon.

data/org.freedesktop.portal.Notification.xml

src/notification.c

swick · 2024-04-22T14:46:42Z

Still have to look at the sound validator but I'm through with everything else.

jsparber · 2024-04-23T11:08:45Z

Still have to look at the sound validator but I'm through with everything else.

Thanks for the review. I think i addressed everything.

src/xdp-utils.c

tests/notification.c

data/org.freedesktop.portal.Notification.xml

swick · 2024-04-23T16:01:34Z

data/org.freedesktop.portal.Notification.xml

+          This is the list of options that can be used as `purpose` for `buttons`.
+    -->
+    <property name="SupportedOptions" type="a{sv}" access="read"/>
+
    <!--
        ActionInvoked:


Because this is modeled after org.freedesktop.Application.ActivateAction. Shouldn't this also split out the platform_data into its own argument instead of sending it in the @parameter array?

would be nice, but doesn't that break backwards compatibility?

Ah, true, but you could always add a ActionInvoked2

i prefer to not have a second signal. I think it's not too bad to have as a second value in @parameter. Honestly i would like this signal not to exist at all :)

I think a second signal would be better. Adding a new value into parameter may count as an API break anyway, because user code may be strictly validating the parameter they receive, and not expect an additional entry in the av.

why would that count as an API break? It's defined as a av already. And it doesn't say anywhere that there is only one value set.

Hmm, does this parameter not match the parameter of a GAction? Or does it just look similar but have different semantics? Reading the rest of the spec it looks like parameter is actually just a dumping ground for vaguely-typed response data? Typically an a{sv} with well-defined-but-extensible keys would be used for this in a D-Bus API, but changing to that would definitely be an API break (or require a new signal).

Hmm, does this parameter not match the parameter of a GAction?

No, it doesn't match. The GAction parameter or actually target is placed in the parameter array. It just looks similar.

The current specs don't even say that it will be the first value in the parameter argument:

array which will contain the target parameter for the action, if one was specified

Having two similar but slightly different interfaces will be really confusing. Having a ActionInvoked2 which matches the arguments from org.freedesktop.Application.ActivateAction would be an improvement (and we could add a a{sv} just for good measure).

Having two similar but slightly different interfaces will be really confusing.

You mean org.freedesktop.Application.ActivateAction and the ActionInvoked signal?

The two different interfaces are pretty much never used in the same application. I think it may be a little confusing (but already having the two is confusing enough), and requiring applications, or client libs, to implement a third interface may be even more confusing, since they need to support version one 1 the interface.

I agree it's sub optimal but i strongly think that adding a ActionInvoked2 creates unnecessary extra work for applications (or client libraries).

This property allows applications to specify a sound to be played whenever the notification is displayed. The format used is inspired by the serialized from of GIcons.

The `markup-body` property allows applications to set markup on the body. The used markup is a subset of html limted to <b>, <i> and <a>.

The `desktop-file-id` allows applications to specify the desktop file that should be used to look up information about the app. This is especially useful for unsandboxed apps where the portal can't look up the desktop file id based on the app id.

This property allows apps to specify how the notification is displayed.

We need to hand out the activation token for XDG Activation in some way. I think it's pretty nice that we can just add the same platform data as used for DBus Activation to the ActionInvoked signal.

The category allows the notification server to handle specific notification different. E.g. calls notifications.

The purpose for a button allows the notification server to style the button specially and know the purpose of the button.

Let applications query supported options for category and button purpose.

Mikenux mentioned this pull request Mar 8, 2024

Notifications Portal v2 proposal #983

Open

jsparber changed the title ~~Implement v2 notification spec~~ Start to implement v2 notification spec Mar 9, 2024

jsparber force-pushed the implement_v2_notification_spec branch from eae298c to 6cb0d0b Compare March 22, 2024 17:28

ilya-fedin reviewed Mar 22, 2024

View reviewed changes

jsparber force-pushed the implement_v2_notification_spec branch 2 times, most recently from 1c893d7 to 9c11013 Compare March 26, 2024 17:08

ilya-fedin mentioned this pull request Mar 30, 2024

Add an option to display notification content even if screen is locked telegramdesktop/tdesktop#27666

Open

bertob reviewed Apr 3, 2024

View reviewed changes

data/org.freedesktop.portal.Notification.xml Outdated Show resolved Hide resolved

jsparber added 9 commits April 4, 2024 21:35

tests/notification: Check result in notification add callback

6e2f90a

Adding a notification may fail, instead of waiting for the timeout we can check the result we get in the callback.

tests/notification: Remove redundant code

13f78fa

We can reuse most of the test setup for all tests. This will make it easier to test more properties.

test: Implement tests for notification icons

0dcec82

icon-validator: Allow custom path for icon validator via env

a16aee0

This allows setting a custom path for the icon validator, especially useful when testing without installing.

utils: Print error log when icon-validator fails to validate a image

3482e33

CI: Fix running icon-validator

a66d939

The sandbox for the icon-validator needs a privileged container. And it needs librsvg to be able to validate SVGs.

validate-icon: Don't log command ever

e66041b

If debug messages are turned on the output of validate-icon isn't a valid key file since it contains log output as well.

jsparber force-pushed the implement_v2_notification_spec branch from 9c11013 to 60db472 Compare April 5, 2024 17:59

jsparber force-pushed the implement_v2_notification_spec branch 8 times, most recently from 5705ffc to 049806c Compare April 16, 2024 11:14

jsparber mentioned this pull request Apr 16, 2024

notification v2 specs: Allow passing GFileIcon, sound and add additional properties flatpak/libportal#147

Open

pwithnall reviewed Apr 19, 2024

View reviewed changes

jsparber force-pushed the implement_v2_notification_spec branch from 049806c to fc2483f Compare April 19, 2024 16:05

swick reviewed Apr 22, 2024

View reviewed changes

jsparber force-pushed the implement_v2_notification_spec branch from fc2483f to fed1cdd Compare April 23, 2024 11:05

swick reviewed Apr 23, 2024

View reviewed changes

jsparber added 19 commits April 26, 2024 10:08

notification: Accept icons as file descriptor

74fb5c0

tests: Implement tests for file-descriptor notification icons

c4b0d97

notification: Add new sound property

812c39c

This property allows applications to specify a sound to be played whenever the notification is displayed. The format used is inspired by the serialized from of GIcons.

test: Implement tests for notification sound property

ef5727f

notification: Add new property markup-body to support markup

d2857f0

The `markup-body` property allows applications to set markup on the body. The used markup is a subset of html limted to <b>, <i> and <a>.

notification: Improve description of title and body

1ab753f

test: Implement test for notification property markup-body

8c897e4

test: Implement tests for desktop-file-id notification property

2667b73

notification: Add display-hint property

2668733

This property allows apps to specify how the notification is displayed.

test: Implement tests for display-hint notification property

e72ecb0

notification: Improve wording around DBus Activation

4f78be7

notification: Add platform-data to ActionInvoked signal

60b1135

We need to hand out the activation token for XDG Activation in some way. I think it's pretty nice that we can just add the same platform data as used for DBus Activation to the ActionInvoked signal.

notification: Add category property

0e77c32

The category allows the notification server to handle specific notification different. E.g. calls notifications.

notification: Add purpose property for buttons

7a0beee

The purpose for a button allows the notification server to style the button specially and know the purpose of the button.

test: Implement tests for category notification property

eb21fb3

notification: Add supported-options property

c1c7b24

Let applications query supported options for category and button purpose.

test: Implement tests for supported-options property in notification

60d912e

notification: Bump interface to version 2

3ad1832

jsparber force-pushed the implement_v2_notification_spec branch from fed1cdd to 3ad1832 Compare April 26, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement v2 notification spec #1298

Implement v2 notification spec #1298

jsparber commented Mar 8, 2024 •

edited

Mikenux commented Mar 8, 2024

jsparber commented Mar 8, 2024

ilya-fedin commented Mar 9, 2024 •

edited

Mikenux commented Apr 13, 2024 •

edited

jsparber commented Apr 19, 2024

swick Apr 22, 2024

jsparber Apr 22, 2024

swick Apr 22, 2024

jsparber Apr 22, 2024 •

edited

swick commented Apr 22, 2024

jsparber commented Apr 23, 2024

swick Apr 23, 2024 •

edited

jsparber Apr 23, 2024

swick Apr 23, 2024

jsparber Apr 23, 2024

pwithnall Apr 23, 2024

jsparber Apr 23, 2024 •

edited

pwithnall Apr 23, 2024

jsparber Apr 23, 2024 •

edited

swick Apr 24, 2024

jsparber Apr 26, 2024

Implement v2 notification spec #1298

Are you sure you want to change the base?

Implement v2 notification spec #1298

Conversation

jsparber commented Mar 8, 2024 • edited

Mikenux commented Mar 8, 2024

jsparber commented Mar 8, 2024

ilya-fedin commented Mar 9, 2024 • edited

Mikenux commented Apr 13, 2024 • edited

jsparber commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsparber Apr 22, 2024 • edited

Choose a reason for hiding this comment

swick commented Apr 22, 2024

jsparber commented Apr 23, 2024

swick Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsparber Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsparber Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsparber commented Mar 8, 2024 •

edited

ilya-fedin commented Mar 9, 2024 •

edited

Mikenux commented Apr 13, 2024 •

edited

jsparber Apr 22, 2024 •

edited

swick Apr 23, 2024 •

edited

jsparber Apr 23, 2024 •

edited

jsparber Apr 23, 2024 •

edited