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

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

tests/notification.c

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.

This makes it possible to use memfd_create() to create a temporary file. And in a future commit the portal will allow passing icons as a sealable memfd.

Images shouldn't be to big in size. 4MBs is more then enough for all cases.

Since in a future commit the notification portal will also accept sealable memfds we can also use it internally so that we don't have to create a temporary file and clean it up after the validation.

Icons could potentially be really big in size. Instead of passing the data via GBytesIcon (as part of the D-Bus message) allow passing a sealed fd created with `memfd_create()`

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.

jsparber · 2024-07-04T17:26:22Z

I think the memfd stuff needs a bit of a redesign. It probably makes sense to split that out into its own MR as well.

The goal with memfd is to avoid copying and we do that by passing the memfd fd around and then mmap'ing it. That means in particular that you should convert the bytes into a memfd as early as possible. It also needs to be sealed before we pass it on to anywhere (if the validator is compromised it shouldn't be able to screw up the memfd).

The validator process has to take in a fd instead of reading from stdin.

Then finally, when we want to pass things to the backend, we have to convert the memfd back to something else, if the backend doesn't support memfds. We'll need to introduce a version property for the notification impl service for that purpose.

I think I addressed everything. Expect for converting all bytes icons to fds, since it involves even more changes.

swick · 2024-07-05T10:26:44Z

src/xdp-utils.c

+  int output_fd = -1;
+  xdp_autofd int stdout_fd = -1;
+
+  stdout_fd = memfd_create("spawn-validator-stdout", 0);


I don't see the point of using a memfd for stdout. It's a few bytes of text.

swick · 2024-07-05T10:33:22Z

src/xdp-utils.c

+  lseek(input_fd, 0, SEEK_SET);
+
+  launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE);
+  g_subprocess_launcher_take_stdout_fd (launcher, xdp_steal_fd (&stdout_fd));


This is transferring ownership. At that point using the fd (output_fd or stdout_fd) is an error! If you want to be able to use the fd while the function takes ownership, you have to dup the fd.

Nothing here is crashing because the launcher keeps the fd around until it is disposed via the autoptr but conceptually, the launcher owns the fd and could close it at any time.

swick · 2024-07-05T10:33:35Z

src/xdp-utils.c

+    }
+
+  input_fd = dup(fd);
+  // We don't need to dup() it since it will life till the launcher is dropped


wrong comment style

swick · 2024-07-05T10:35:49Z

src/xdp-utils.c

  args[1] = "--sandbox";
-  args[2] = name;
-  args[3] = NULL;
+  args[2] = NULL;


Can we make the validator take either a file path or a fd argument and then mmap either of them? That way using it on the command line still works which will be helpful if this something is going wrong.

I was thinking about doing that but it's possible to just do ./_build/src/xdg-desktop-portal-validate-icon 3< image.png on the terminal so since the validator is only used by us there isn't much need to have a nicer command line interface.

Yes, but hardcoding some fd number is just bad design and having a filename argument takes a few lines of code.

swick · 2024-07-05T10:40:23Z

src/xdp-utils.c

+}
+
+static int
+bytes_to_fd (GBytes *bytes)


The name itself is useless. A fd can represent anything but this specifically creates a memfd.

swick · 2024-07-05T10:43:36Z

src/xdp-utils.c

-void
-cleanup_temp_file (void *p)
+static gboolean
+ensure_sealed_memfd (int      fd,


Not really a fan of this and the other function. Can we make this one mapped_file_new_sealed_from_fd and the other one mapped_file_new_sealed_from_bytes?

Ah, no, that doesn't work because it represents an mmap'ed area. Then maybe a XdpMemfd gobject with new_from_bytes, new_from_fd, seal, take_fd, dup_fd.

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 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 marked this pull request as ready for review April 16, 2024 12:50

jsparber changed the title ~~Start to implement v2 notification spec~~ Implement v2 notification spec Apr 16, 2024

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 added 22 commits July 4, 2024 19:06

validate-icon: Read icon from fd instead of a file

72f0d8a

This makes it possible to use memfd_create() to create a temporary file. And in a future commit the portal will allow passing icons as a sealable memfd.

validate-icon: Add file limit of 4MB

841d6dc

Images shouldn't be to big in size. 4MBs is more then enough for all cases.

xdp-utils: Use memfd_create() instead of temporary file

f80afc1

Since in a future commit the notification portal will also accept sealable memfds we can also use it internally so that we don't have to create a temporary file and clean it up after the validation.

notification: Bump interface to version 2

edde298

notification: Accept icons as file descriptor

170f749

Icons could potentially be really big in size. Instead of passing the data via GBytesIcon (as part of the D-Bus message) allow passing a sealed fd created with `memfd_create()`

tests: Implement tests for file-descriptor notification icons

209285a

notification: Add new sound property

16b3e0c

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

4c0b998

notification: Add new property markup-body to support markup

76d4c51

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

4e56fdd

test: Implement test for notification property markup-body

852dce9

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

56c5bdb

notification: Add display-hint property

d02de8b

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

test: Implement tests for display-hint notification property

5595c5d

notification: Improve wording around DBus Activation

be0e397

notification: Add platform-data to ActionInvoked signal

596a7f3

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

756dd90

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

notification: Add purpose property for buttons

5ede221

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

a28bab7

notification: Add supported-options property

5f415af

Let applications query supported options for category and button purpose.

test: Implement tests for supported-options property in notification

2b87e09

jsparber force-pushed the implement_v2_notification_spec branch from c3ad5be to 2b87e09 Compare July 4, 2024 17:23

swick reviewed Jul 5, 2024

View reviewed changes

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

Loading

Mikenux commented Mar 8, 2024

jsparber commented Mar 8, 2024

ilya-fedin commented Mar 9, 2024 •

edited

Loading

Mikenux commented Apr 13, 2024 •

edited

Loading

jsparber commented Apr 19, 2024

swick commented Apr 22, 2024

jsparber commented Jul 4, 2024

swick Jul 5, 2024

swick Jul 5, 2024

swick Jul 5, 2024

swick Jul 5, 2024

jsparber Jul 5, 2024

swick Jul 5, 2024

swick Jul 5, 2024

swick Jul 5, 2024

swick Jul 5, 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 Loading

Mikenux commented Mar 8, 2024

jsparber commented Mar 8, 2024

ilya-fedin commented Mar 9, 2024 • edited Loading

Mikenux commented Apr 13, 2024 • edited Loading

jsparber commented Apr 19, 2024

swick commented Apr 22, 2024

jsparber commented Jul 4, 2024

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

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 commented Mar 8, 2024 •

edited

Loading

ilya-fedin commented Mar 9, 2024 •

edited

Loading

Mikenux commented Apr 13, 2024 •

edited

Loading