flatpak / xdg-desktop-portal

Desktop integration portal
https://flatpak.github.io/xdg-desktop-portal/
GNU Lesser General Public License v2.1
605 stars 195 forks source link

Notifications Portal v2 proposal #983

Closed jonas2515 closed 1 week ago

jonas2515 commented 1 year ago

Quick introduction for those who don't know me: I'm Jonas (aka verdre) from the GNOME community, mostly working on gnome-shell/mutter but also across the stack to get things ready for phones.

The notifications portal is currently of very limited use for apps with more advanced notification needs, especially with smartphone use-cases in mind.

Here's a proposal for a new version of the notifications portal, trying to kickstart development and open room for discussion.

Things we want

Things we don't want

Proposed API

Permission requests

Content Hints

Used for all notifications that need specific handling, eg.

There are three different namespaces that any hint must adhere to:

Requests and Responses

Requests and responses are the new API for actions.

Requests have an application-defined requestId (s) and requestParams (a{sv}).

The response object is passed to the responseAction GAction as the parameter (format ssa{sv}: notificationId (s), requestId (s), responseParams (a{sv})). The responseParams are usually empty and only set when using a preset request ID.

Preset request IDs

There's special request IDs for common requests in the preset namespace, proposed IDs (standardize with the protocol?):

Presentation hints

Presentation hints allow configuring specifics of how a notification should be presented. The system may ignore those depending on its policy.

Supported presentation hints are listed in presentationHints field returned by GetCapabilites().

Standardized hints:

Custom methods of alerting

Apps may use custom methods of alerting the user, for example to play audio from a special source like an online stream of a radio station.

DBus API

Methods:

Open questions

How would I do x with this?

Create and ring an alarm clock 1) Create "next alarm" notification a few hours before alarm - Set content hints to `class.alarm` - Add `preset.cancel` request to allow removing alarm again - Set `presentNonDisrupting` presentation hint so that the notification silently shows in the tray 2) When alarm rings, remove old notification and create a new one: - Set content hint to `class.alarm.ringing` - Add `preset.done` and `snooze` requests - Set `isPersistent` to true - In case a window of the app is already open and focused, set the `dontShowBanner` presentation hint 3) On response: - When `preset.done` is invoked, remove notification - When `snooze` is invoked, remove notification and go to 1)
Notifying an incoming call 1) Create the notification - Set content hint to `class.call.incoming`, this magically makes the system: - Style the notification banner in the "incoming call" style - Repeatedly play a ringtone and vibration pattern - Add `preset.accept` and `preset.decline` requests - Set `isPersistent` to true - In case a window of the app is already open and focused, set the `dontShowBanner` presentation hint 2) Handle responses and missed call - When `preset.accept` gets invoked, accept the call and update the existing notification - Change the content hint to `class.call.ongoing` - Replace requests with `preset.cancel` and `preset.call.toggleSpeakerphone` - When `preset.deny` gets invoked, deny call and remove notification - When the user didn't trigger a response until the the call was missed, update the existing notification - Set content hint to `class.call.missed` - Set `isPersistent` to false - Replace requests with `preset.call.returnCall`
Mikenux commented 1 year ago

Hello Jonas,

I think this requires organization by use case: progress notifications, alarms, calls/messages (contacts), weather/amber alerts... And those use cases may have other conditions.

Examples (non-exhaustive):

Mikenux commented 1 year ago
  • preset.onClicked: Get notified when the notification itself (so not a specific request) has been clicked

  • preset.onDismiss: Get notified when the notification is dismissed

Could you provide examples of use cases? This appears to be tracking... (apart from tracking when an action is triggered; e.g. clicking on a news notification opens the relevant news and the app is aware of it).

Presentation hints and custom methods of alerting: Even though it depends on system policy, I don't see the point of having them when things should be controlled by the user.

jonas2515 commented 1 year ago

Hi @Mikenux,

Maybe a good opportunity to condition this with applications exposing the contacts

Fwiw what you're talking about here is some kind of "People" API, that's what iOS and Android use for this kind of stuff these days. I think we'll eventually need something similar, yes. That said, this is is a whole different topic and very complicated problem in itself, which I'd rather solve properly. Once we have a format to expose and store contacts between apps, we can still add that to the notifications portal.

Could you provide examples of use cases? This appears to be tracking

Telling an app that its notification has been dismissed hardly sounds like tracking to me. The current APIs allow for this already, and iOS has API for that as well.

Even though it depends on system policy, I don't see the point of having them when things should be controlled by the user.

As mentioned in the proposal, it's API to support niche use-cases such as internet radio alarm clocks. There's no way we can integrate playing an online music stream into our API, so in this case, the app has to play the sound itself. The appPresentationPolicy gives the rules for the app whether it may use the speakers itself or not, this is what makes sure the global notification policy is still enforced.

Mikenux commented 1 year ago

Yes, I have no doubt that it is complicated, especially for contacts. The goal is to limit things to relevant use cases. Currently, the proposal seems to allow any app to use anything...

swick commented 1 year ago

We don't want persistent notifications to be abused as app indicators, that's why they are removed when app closes.

What do you mean by closed? When the app has no windows?

Permission requests Every app has to request permission to send notifications using the RequestNotifications() method.

The alternative here is to let apps provide all the data and then decide if and how to show notifications in the backend based on permissions without ever telling the app about the decision.

Is there a reason to prefer the explicit permission requests?

jonas2515 commented 1 year ago

What do you mean by closed? When the app has no windows?

Yup, exactly.

Is there a reason to prefer the explicit permission requests?

Good question, off-hand the other approach sounds fine to me too. The upside of an explicit permission request would be that we can show a dialog to users asking whether the app may send notifications. I found that dialog quite useful on ios, eg. when you're starting a game, you know right away that the notifications will be ads, so you say no right away.

swick commented 1 year ago

Yup, exactly.

Then what about apps running in the background? People use those, we show them in the quick settings menu and they can be interacted with. Why should they not be able to show a persistent notification when they're running?

The upside of an explicit permission request would be that we can show a dialog to users asking whether the app may send notifications. I found that dialog quite useful on ios, eg. when you're starting a game, you know right away that the notifications will be ads, so you say no right away.

Apps currently expect to be able to send notifications by default. Does that mean every app which potentially sends notifications will bother me with a popup asking for permission to send notifications? IMO sending notifications should be allowed by default and be turned off potentially in some granular way.

You also talk about the concept of important notifications and notifications which can still show when do-not-disturb is enabled. Should that require additional permissions?

If a user wants messages from a certain person to show even when do-not-disturb is enabled, that's something the app doesn't have to care about. On the other hand, if the app is warning about an incoming thunderstorm, that's a notification the app should have control over.

Mikenux commented 1 year ago

For progress notifications, perhaps these should be drawn by the system, not the apps for a consistent interface. By identifying the requested progress notification, the system can decide the UX. Examples: show alarms when appropriate, show file download when app is closed and/or when app has no focus or is covered by other windows.

For alarms, it's important that they should register when they start, so that apps can't play sound at will using notifications (given that the app is running in the background).

alice-mkh commented 1 year ago

Everything in the proposal, incl. progress notifications are always drawn by the system. Apps cannot draw any of this even if they wanted, even outside flatpak.

Mikenux commented 1 year ago

One thing I'm thinking about is whether preventing apps from using progress notifications for content fetching (e.g. fetching mails, news) is wanted.

Also, what about "autostart"? In GNOME, background app autostart is wanted, except for app that are closed to continue ongoing activity (e.g. web browser, Builder). However, I mentioned the case of download apps (e.g. downloading torrents), where some downloads may be completed, while others may be paused. So, is autostart still a thing, and does it apply to download apps?

alice-mkh commented 1 year ago

Please keep it on topic. Autostart is a background portal thing and already exists, this is about notifications.

Mikenux commented 1 year ago

Knowing the autostart experience might be needed to know what differentiation needs to be done for progress notifications... except if it can be done on the background portal side.

Examples:

Or are these conditions for the background portal, or something else? (e.g. progress notification + folder location).

Just to be sure...

alice-mkh commented 1 year ago

Again, this is about notifications. This is not about folders or file saving. Can we please keep things on topic and not devolve this into bikeshedding like e.g. https://github.com/flatpak/xdg-desktop-portal/issues/463? That issue is basically impossible to follow at this point, I don't want this one to end up the same way.

Asking you specifically because 40% of comments in that issue are yours (113 out of 278, to be specific).

Mikenux commented 1 year ago

GNOME folks, disrespectful as usual... to me. Mentioning the number 463 is really a bad example, proving that you don't even know what happened for there to be so many comments. You really like free bashing at GNOME.

alice-mkh commented 1 year ago

Well no, I do know what happened there - it got drowned in proposals like https://github.com/flatpak/xdg-desktop-portal/issues/463#issuecomment-1432463242, same way as e.g. https://gitlab.gnome.org/Teams/Design/os-mockups/-/issues/150 did.

Not exclusively that, of course, but in a large part that. So let's stop this before this issue is consumed as well. Thanks in advance.

jonas2515 commented 1 year ago

Then what about apps running in the background? People use those, we show them in the quick settings menu and they can be interacted with. Why should they not be able to show a persistent notification when they're running?

Well, it's a compromise, I don't see another way of avoiding that apps running in the background just put a persistent notification into the tray to let users know what they're up to. In theory that's the exact use-case covered by https://gitlab.gnome.org/Teams/Design/os-mockups/-/issues/150, but then again I'm not a fan of status notifiers either. IMHO apps which don't have a window open should never annoy users in any persistent way, that starts with music players which shouldn't be playing music when they're closed and ends with backup apps which should just quietly and resource-friendly do their job in the background.

Apps currently expect to be able to send notifications by default. Does that mean every app which potentially sends notifications will bother me with a popup asking for permission to send notifications? IMO sending notifications should be allowed by default and be turned off potentially in some granular way.

Fair point, that is what we're doing right now after all and it seems to be working okay-ish, as longs as we manage to keep apps which are actively hostile towards users (aka financed by ads) away from our platform, I think it'll be fine.

You also talk about the concept of important notifications and notifications which can still show when do-not-disturb is enabled. Should that require additional permissions?

I think it's fine to allow everyone to mark their notifications as important by default, you do want that feature in basically every chat app for example. I think the interesting part here is the "only if explicitly requested by user": I'm not sure how to enforce that other than shaming apps for violating the spec, but if everyone respects that we should be fine?

Fwiw, ios does allow this by default, but it can be disabled per-app in the notification settings, I think we could do the same.

If a user wants messages from a certain person to show even when do-not-disturb is enabled, that's something the app doesn't have to care about. On the other hand, if the app is warning about an incoming thunderstorm, that's a notification the app should have control over.

The thing is you want the UI for things like marking contacts or chat groups as important inside the app, because that's where users will look for it and that's where the context is. Android kinda moves that UI to the system with its notification channels, and that duplicates things and is generally quite awkward when it comes to features like this.

Things obviously get more complicated if we start to have a system-wide API for persons where you can actually mark one contact as important system-wide. In this case indeed the UI would be part of the system, and the "important" bit should no longer be set by the app. I don't see this happening anytime soon though, but if it would I don't think anything in here would stop that in particular.

swick commented 1 year ago

IMHO apps which don't have a window open should never annoy users in any persistent way, that starts with music players which shouldn't be playing music when they're closed and ends with backup apps which should just quietly and resource-friendly do their job in the background.

The reality is that apps can run in the background, they can show the media controls which pretty much appear as a persistent notification, we list apps running in the background in the quick settings menu and we're adding more context to apps running in the background with the background portal.

I don't think it makes a lot of sense to restrict persistent notifications out of all the things to apps with windows only.

I think it's fine to allow everyone to mark their notifications as important by default, you do want that feature in basically every chat app for example. [...] Fwiw, ios does allow this by default, but it can be disabled per-app in the notification settings, I think we could do the same.

Sounds good.

The thing is you want the UI for things like marking contacts or chat groups as important inside the app, because that's where users will look for it and that's where the context is. Android kinda moves that UI to the system with its notification channels, and that duplicates things and is generally quite awkward when it comes to features like this.

I still think that would be ideal. Punching though do-not-disturb can be really bad if it is unwanted so delegating it to the system makes a lot of sense.

Right now we really don't have the infrastructure for it though. So I'm fine with leaving it up to apps as long as we have a way forward to delegate it to the system eventually.

Mikenux commented 1 year ago

It would be good to know for which applications a persistent notification is necessary, except for media players using MPRIS and progress notifications...

ispanos commented 1 year ago

We don't want persistent notifications to be abused as app indicators, that's why they are removed when app closes.

My only note would apply to this. Do not remove the notification. Make it removable by the user once the app closes.

Also, there are valid use-cases for persistent notifications. I can think of many use-cases. , a company that make some smart buttons (Bluetooth switches) used to have a persistent notification to allow you to press digital buttons from your notifications panel on android. I know this is not an actual notification, but it was the absolute best way to do it. Maybe what I'm describing should ultimately be a widget panel, next to the notification panel, but that's a bit excessive.

jonas2515 commented 1 year ago

I don't think it makes a lot of sense to restrict persistent notifications out of all the things to apps with windows only.

Yeah I don't really have a strong opinion on that. I'd be happy to try that approach first, can always restrict it more if apps will abuse it. Then again I am pretty sure they will abuse it if this API gets implemented before the app indicator one.

Maybe what I'm describing should ultimately be a widget panel

Yup, that's exactly the point I'm coming from here. That's what persistent notifications are used for on android, but in practice very often what you really want is a proper widget API for apps.

Anyway, IMHO this is ultimately a design question and we should leave the answer to the design team and follow that in the API.

ispanos commented 1 year ago

On android I understand that there are some notifications that act like widgets, but there are some that are very annoying. And now that I think of it there are some that treat the notification like an app indicator and they are so annoying (zepp life app) .

Anyway, IMHO this is ultimately a design question and we should leave the answer to the design team and follow that in the API.

Which design team? As a swaywm user, I would rather have a solid API implementation that doesn't restrict dedicated notification daemons. It should definitely be coordinated by both sides, but in a DE agnostic way.

I am pretty sure they will abuse it if this API gets implemented before the app indicator one.

I don't understand why this is the case, but I am not so educated on that matter. There are already extensions for that on Gnome and other desktops/wm's have app indicators, so maybe this is not so concerning (?).

I don't think I have anything more to contribute. Thank you for opening that issue, I've been waiting for this for a long time.

Mikenux commented 1 year ago

Then again I am pretty sure they will abuse it if this API gets implemented before the app indicator one.

It's best not to implement anything about persistent notifications, even after implementing the status notifier specification, at least if GNOME sticks to its position of having "systray" apps (those that want a menu) in background apps UI. The point is that for users, having quick access to the menu is essential, and the background apps UI in GNOME does not allow this. So, because of this, apps will probably still abuse of persistent notifications even if they are implemented afterwards.

Also, widgets were briefly discussed in the status notifier spec, but were rejected as not being simple (unless they've changed their minds since then, but I doubt that). @ispanos: Feel free to open an issue about it with case examples. This is something wanted and I guess having a specific API for it shouldn't block the location of these (i.e. widgets can be located in the notification area if it is the wanted UX).

kbroulik commented 1 year ago

Hi, this is the KDE Plasma notification developers and we like your proposal a lot! Here’s a few suggestions and comments we have:

Lock Screen Visibility / Privacy

It should be possible to tag a notification with a visibility/privacy tag, similar to Android’s visibility property so we can decide whether the notification (and/or how much of it, if at all) is shown on the lock screen, shown during screen sharing, synced to your phone, etc.

Markup

Specify what “markup” is supported here. Probably a small subset of HTML (e.g. <b>, <i>, perhaps lists, etc) but we don’t want to see tables or CSS or anything overly complicated in here.

Also, we believe that plain text should be the default, with markup as an opt-in flag on the notification. In the past we’ve often had issues with apps either not escaping their text or ending up with double escape. Making markup a conscious choice on the app side (I enabled markup for this notification so now I have to sanitize my input) would fix that.

Custom sound file

Should this support XDG sound names rather than only custom paths?

On the other hand, a “default” sound could be implied from the class and then be configurable in the shell which sound to use for which class and/or per application.

"empty string" for sound means "play no sound at all". Clarify what the absense of this hint means, play default notification sound, if any, and "empty" suppresses it?

Progress

Is there an indeterminate state? NaN? :-)

Additionally, a preset.pause known action could be useful for suspending a task.

Is it in scope to add an additional progress details dict, e.g. "From" / "To" in a copy job, etc?

GAction

A GAction calls to a well-known DBus name, right? So it is only good for a unique application. What about having multiple instances of an application?

Especially, given you explicitly mention that portal notifications get revoked when the app quits, so there is no DBus activation happening, it seems like an unnecessary restriction.

Unless implied by GAction, there is no mention of the XDG Activation protocol, which is necessary for for raising an application window in response to a notification.

Image

The image, is that the notification icon, like user avatar that sent a message?

Please specify the wire format used (e.g. can we just send an iconName, can we supply multiple sizes and variants of an icon, etc). We don’t like sending image data over DBus when we could have the client pass a file descriptor to an image instead.

Both Plasma and Android support a large preview for example for screenshots or received images, which would be nice to have specified here, too.

Other

DemiMarie commented 1 year ago

Then what about apps running in the background? People use those, we show them in the quick settings menu and they can be interacted with. Why should they not be able to show a persistent notification when they're running?

Well, it's a compromise, I don't see another way of avoiding that apps running in the background just put a persistent notification into the tray to let users know what they're up to. In theory that's the exact use-case covered by https://gitlab.gnome.org/Teams/Design/os-mockups/-/issues/150, but then again I'm not a fan of status notifiers either. IMHO apps which don't have a window open should never annoy users in any persistent way, that starts with music players which shouldn't be playing music when they're closed and ends with backup apps which should just quietly and resource-friendly do their job in the background.

This should be up to the portal backend, and ultimately to the user. Just because you dislike background status notifications does not mean that everyone else also dislikes them.

Mikenux commented 1 year ago

From what I understand, what is NOT wanted is a notification that stupidly stays at the top of the notification list, especially when it is not showing anything (i.e. no specific status).

There is the background portal which already handles such "persistence", it just lacks the menu for actions.

DemiMarie commented 1 year ago

From what I understand, what is NOT wanted is a notification that stupidly stays at the top of the notification list, especially when it is not showing anything (i.e. no specific status).

That’s understandable, but at the same time users need chat apps to be able to show a notification (one for each chat message) that persists even when the app is not running.

Mikenux commented 1 year ago

That’s understandable, but at the same time users need chat apps to be able to show a notification (one for each chat message) that persists even when the app is not running.

I agree with such persistence. This is the same for all messaging apps (including email) and news apps. Here, the notifications are used to open the app in the right location in the app. I myself use the notifications on my iPad for the news that I will consult later, and especially since not all of them are directly findable in the app. This can also be useful in the event that a system decides to close an app to free up system resources or to save power (in the absence of apps supporting a means that limits their resource usage) .

andyholmes commented 1 year ago

I agree with such persistence. This is the same for all messaging apps (including email) and news apps.

I would tend to disagree, assuming a "closed app" actually means an application that is not running, as opposed to an application with no open windows.

Having notifications persist when an application has actually quit can lead to "dead" notifications (e.g. content that is no longer available, or out-of-date information), or give the user the impression that closing the notification will inform the application it has been dismissed or acknowledged.

I see no reason why a messaging application that relies on this behaviour should not simply keep running in the background. As for applications abusing persistent notifications as status notifiers, that's really something that should be enforced socially.

Mikenux commented 1 year ago

Having notifications persist when an application has actually quit can lead to "dead" notifications (e.g. content that is no longer available, or out-of-date information)

For messaging apps, this shouldn't be a problem: emails are stable unless you delete them, and chat apps should be tied to a conversation. Messaging and news apps should also be able to handle such cases.

give the user the impression that closing the notification will inform the application it has been dismissed or acknowledged.

As a user, I never imagined that an app would be informed when I close one of its notifications...

I see no reason why a messaging application that relies on this behaviour should not simply keep running in the background.

The app runs in the background. However, the app may crash. There is also the case of developers of a DE wishing, for example, to use the app wake-up for scheduled content fetching in the event of saving energy and/or system resources (note that this is speculative, but not impossible).

pwithnall commented 1 year ago

Some drive-by thoughts from me. I was linked to this and it’s relevant to my interests since I’d probably have to be the one to implement/review it in GLib. However, I don’t have the full context in my head around portals, or mobile use cases; and I haven’t done a line-by-line review of the API or its potential implementation in GLib.

DemiMarie commented 1 year ago

Some drive-by thoughts from me. I was linked to this and it’s relevant to my interests since I’d probably have to be the one to implement/review it in GLib. However, I don’t have the full context in my head around portals, or mobile use cases; and I haven’t done a line-by-line review of the API or its potential implementation in GLib.

  • Could this be built directly on the existing Desktop Notification Spec? That would allow projects to reuse some code which already implements that spec. The spec already supports a GetCapabilities() method, so it could be extended without breaking existing implementations. Otherwise, projects like GLib and GTK will end up having to implement a third notification spec (there’s already the FDO one and the internal GTK one).

No. The Desktop Notification Spec uses server-generated IDs that are not guaranteed to be unique for all time. This results in unavoidable race conditions and prevents notification actions from safely being invoked after the program that generated them has shut down.

  • If an image is set on a notification, should the API also allow for alt-text to be sent with it, for accessibility? Similarly, do any other visual parts of the API need accessibility adaptations?

Any API with an image should require alt text for accessibility, and should pass the image as an uncompressed bitmap for security.

  • Why does RemoveNotification() need to take an appId argument? It should be able to look up the app ID from the notification ID.

Notification IDs are generated by the app and are not trusted. A malicious sandboxed application might intentionally cause a collision.

jonas2515 commented 1 year ago

Sorry it took me so long to get back to this, thanks everyone for the detailed answers and for sharing your thoughts, great to hear the KDE Plasma team is on board with this!

It should be possible to tag a notification with a visibility/privacy tag

Agreed, although as you already mentioned this is opening quite the box if it's not only about lockscreen, but also screensharing, syncing to the phone etc.

In GNOME Settings we do already have control over lock-screen visibility on both a system-wide and a per-app level. I guess it can be nice to allow apps control over this, eg. for secret chats with individual people.

I'm not 100% sure if it's a good idea to give apps control over whether individual notifications are on the lockscreen or not, this might cause potential confusion as to why randomly notifications aren't visible on the lockscreen, especially if it can be controlled from both the app and from system settings.

Maybe a simple presentation hint like hideContentOnLockscreen (b) would make sense?

Specify what “markup” is supported here

Absolutely, maybe a simple list of tags would do the trick yeah. As an initial proposal I'd say <b>, <i>, <u>, <a href=>, and yeah I guess <ul> and <li> makes sense too (for showing multiple messages on the same chat).

Also, we believe that plain text should be the default, with markup as an opt-in flag on the notification

Good point! I'd say this should just be another required field: Could add useMarkupForBody (b) before body (s).

Should this support XDG sound names rather than only custom paths?

On the other hand, a “default” sound could be implied from the class and then be configurable in the shell which sound to use for which class and/or per application.

Right, I'd tend to say that all the sounds where we want a system wide default should go via the class hint. This might also provide a good incentive for apps to set their class hint...

In general I'm not sure how much XDG sound names were actually used in practice and how much we should continue supporting them. Personally, I think for most sounds that apps want to play, the Event naming spec from feedbackd is the way to go rather than XDG sound names. Then again I think feedbackd should only cover the scope of feedback for input events, and events like "message-new-instant" or "phone-incoming-call" should go via the notification server instead of directly from the app, but that's a different question anyway...

"empty string" for sound means "play no sound at all". Clarify what the absense of this hint means, play default notification sound, if any, and "empty" suppresses it?

Yeah that was the idea: No hint at all means "leave decision to server", empty string means "never play sound", and passing a sound means "leave decision to server but if you play something, play this".

Is there an indeterminate state? NaN? :-)

Hmm, on first though I'd say no, because a progress bar that's bumping back and forth is useless and might as well just be a "xyz is in progress" text? But yeah, not really sure about that, maybe there's valid use-cases for it...

preset.pause known action could be useful for suspending a task

Sure, sounds good!

Is it in scope to add an additional progress details dict, e.g. "From" / "To" in a copy job, etc?

For me personally no, but given that additionalContent is a vardict and servers can advertise support for individual entries, nothing would stop you from implementing progressDetails (a{sv}) I guess :)

A GAction calls to a well-known DBus name, right? So it is only good for a unique application. What about having multiple instances of an application?

I guess as long as the unique name is still on the bus, we'd talk back to that one when invoking a notification response, if it isn't, we'd use the bus name. In the end, the application should have an internal store of notification IDs that were sent anyway, so it could map that to its instances.

Especially, given you explicitly mention that portal notifications get revoked when the app quits, so there is no DBus activation happening, it seems like an unnecessary restriction.

Hmm I don't think I proposed that, that only thing that I'd disallow once an app quits is persistent notifications, normal non-persistent ones would remain around and clicking their actions would activate the app.

Unless implied by GAction, there is no mention of the XDG Activation protocol, which is necessary for for raising an application window in response to a notification.

Right, sorry if I was unclear in the proposal there. A GAction is basically the GNOME way of defining a new desktop action that can be invoked via ActivateAction(): The activation token is passed as usual via platform_data, action_name would be the responseAction of AddNotification(), and parameter would be the response specific data (ssa{sv}: notificationId (s), requestId (s), responseParams (a{sv})) .

The image, is that the notification icon, like user avatar that sent a message?

Yup!

Please specify the wire format used (e.g. can we just send an iconName, can we supply multiple sizes and variants of an icon, etc).

Yeah, "serialized GIcon" supports quite a few types of images, including icons themes (GThemedIcon or images from the network (GFileIcon and g_file_new_for_uri()).

I'm not sure if the serialization format is documented anywhere, but it's fairly simple: For a themed icon it looks like this (string: 'themed', variant: [ 'network-connected-symbolic', 'network-connected' ]), or for a file (string: 'file', variant: 'file:///home/verdre/picture.jpg').

Both Plasma and Android support a large preview for example for screenshots or received images, which would be nice to have specified here, too.

Yup, this is a different thing which I've left out for now. If we do that, I'd make it an additionalContent field. And given that Plasma already supports this, it's probably a good idea to make it part of the spec, I agree.

AddNotification should specify which hints are mandatory.

I wouldn't say any hints are mandatory, of course if you want nice behavior from the notification server, for example for your "incoming call" notification, you really want to set that hint.

time and endTime should probably be of x type (int64) rather than i.

Clarify whether AddNotification replaces an existing notification with the same ID and if so, how (silently/re-popup) and/or allow to influence this behavior.

Sounds good!

Perhaps add known action presets for toggle webcam, microphone, hangup to control a video call.

Yeah having a mute preset for microphone sounds good, maybe hangup could use preset.done or preset.cancel (should that possible be only one preset?)? "Toggle webcam" on first thought sounds slightly to complex to be in a notification, as that might trigger additional dialogs and whatnot.

pwithnall commented 1 year ago

I'm not sure if the serialization format is documented anywhere, but it's fairly simple: For a themed icon it looks like this (string: 'themed', variant: [ 'network-connected-symbolic', 'network-connected' ]), or for a file (string: 'file', variant: 'file:///home/verdre/picture.jpg').

That format is not part of GLib’s API, so you can’t depend on it implicitly in this spec — you’d have to specify the format explicitly in the spec, or point at another spec for icon interchange.

pwithnall commented 1 year ago

Some drive-by thoughts from me. I was linked to this and it’s relevant to my interests since I’d probably have to be the one to implement/review it in GLib. However, I don’t have the full context in my head around portals, or mobile use cases; and I haven’t done a line-by-line review of the API or its potential implementation in GLib.

  • Could this be built directly on the existing Desktop Notification Spec? That would allow projects to reuse some code which already implements that spec. The spec already supports a GetCapabilities() method, so it could be extended without breaking existing implementations. Otherwise, projects like GLib and GTK will end up having to implement a third notification spec (there’s already the FDO one and the internal GTK one).

No. The Desktop Notification Spec uses server-generated IDs that are not guaranteed to be unique for all time. This results in unavoidable race conditions and prevents notification actions from safely being invoked after the program that generated them has shut down.

Sounds to me like that could be fixed in the original Desktop Notification spec (and hence here, if this ends up being based on it) by exposing a new capability from the notification server which says it supports guaranteed-unique notification IDs, and supporting that in the server by adding app-controlled notification IDs as a hint. That would allow the app to identify notifications after it’s restarted. Assuming that is the crux of the issue?

  • Why does RemoveNotification() need to take an appId argument? It should be able to look up the app ID from the notification ID.

Notification IDs are generated by the app and are not trusted. A malicious sandboxed application might intentionally cause a collision.

A malicious sandboxed app might also send the wrong appId. For notifications to be tied to an app, the server needs to get the app’s connection credentials and use that for identification, unless I’m misunderstanding the security model here.

jadahl commented 1 year ago

A malicious sandboxed app might also send the wrong appId. For notifications to be tied to an app, the server needs to get the app’s connection credentials and use that for identification, unless I’m misunderstanding the security model here.

In portals, the app ID never comes directly from the app itself, but from the sandbox environment. Sometimes it makes sense for the app to send a app ID that extends the app ID with a specific "sub app" (e.g. org.libreoffice.LibreOffice is the app ID, but org.libreoffice.LibreOffice.Writer is the "sub app", but primarily, the base app ID is always derived, not told by the app.

DemiMarie commented 1 year ago

Yeah, "serialized GIcon" supports quite a few types of images, including icons themes (GThemedIcon or images from the network (GFileIcon and g_file_new_for_uri()).

This is not a good idea for multiple reasons:

Here is an alternative:

An image is an RGBA bitmap with 4 bytes per pixel. The height of the image is passed explicitly and the width is computed from it. If the number of bytes is not a multiple of 4 * height, this is a violation of the protocol and a D-Bus error MUST be returned.

pwithnall commented 1 year ago

Here is an alternative:

An image is an RGBA bitmap with 4 bytes per pixel. The height of the image is passed explicitly and the width is computed from it. If the number of bytes is not a multiple of 4 * height, this is a violation of the protocol and a D-Bus error MUST be returned.

I think it would be useful to allow icons to be passed by name (as per the icon naming spec) in some form, as that allows for:

But I agree that the way icons are passed should be fully defined in this spec, rather than depending on an implementation detail of GIO.

jonas2515 commented 1 year ago

Could this be built directly on the existing Desktop Notification Spec? That would allow projects to reuse some code which already implements that spec. The spec already supports a GetCapabilities() method, so it could be extended without breaking existing implementations. Otherwise, projects like GLib and GTK will end up having to implement a third notification spec (there’s already the FDO one and the internal GTK one).

We can definitely explore this, the reason why I didn't really consider it was that FDO notifications API is not really a preexisting thing in case of the portal anyway. But yeah agreed that it would be nice to not have 3 different APIs there...

I'm not sure if the serialization format is documented anywhere, but it's fairly simple: For a themed icon it looks like this (string: 'themed', variant: [ 'network-connected-symbolic', 'network-connected' ]), or for a file (string: 'file', variant: 'file:///home/verdre/picture.jpg').

That format is not part of GLib’s API, so you can’t depend on it implicitly in this spec — you’d have to specify the format explicitly in the spec, or point at another spec for icon interchange.

Mmhh, that's great to know, I'm definitely no expert in that area and not sure what's needed or wanted exactly there. I'd happily leave this to people who know more in the area, I'm sure there's similar cases where good solutions have been found already.

It’s not really clear from the proposal why a separate RequestNotifications() call is needed. If it’s for permission, why not just resolve that permission on the first AddNotification() call? Most apps will call RequestNotifications() lazily anyway.

I think you want to be able to explicitly request notifications in the context of registering for a push service. If we make push notifications as transparent as possible to the user, the "can register for push" permission is probably somewhat bound to the "send notifications" permission.

Why does RemoveNotification() need to take an appId argument? It should be able to look up the app ID from the notification ID.

Yeah, in case of the portal API, I guess we can drop the appId from both AddNotification() and RemoveNotification().

EDIT: ah okay, with the explanation from @jadahl I think it makes sense to drop it from RemoveNotifcation(), but to keep it on AddNotification()

If an image is set on a notification, should the API also allow for alt-text to be sent with it, for accessibility? Similarly, do any other visual parts of the API need accessibility adaptations?

Good question, would be interesting to see how other platforms are handling this.

DemiMarie commented 1 year ago

For well-known icon names (that are required by spec to always be present) this is fine, but otherwise this puts host implementations in a bad position if the icon lookup fails and no icon bitmap was provided. IMO an icon bitmap should be mandatory if the icon name is not one in a specified list.

andyholmes commented 1 year ago

Is there a reason to restrict icons to bitmaps instead of allowing SVGs, for example? I see no reason why the server-side would have any more or less responsibility in terms of security.

DemiMarie commented 1 year ago

Is there a reason to restrict icons to bitmaps instead of allowing SVGs, for example? I see no reason why the server-side would have any more or less responsibility in terms of security.

Yes: the portal client is often a sandboxed Flatpak, and is therefore not trusted, whereas the server is trusted. In Qubes OS this is even more obvious, since the client and server will be in different virtual machines so there is very obviously a security boundary there.

andyholmes commented 1 year ago

Sorry, I do understand the premise of sandboxed applications, but I don't understand restricting images to bitmaps.

To be clearer: is there a reason clients can not send SVGs in the form of bytes (not paths) to the server?

ilya-fedin commented 1 year ago

Maybe use file descriptors instead of file paths? It could be then a file on disk or shared memory.

DemiMarie commented 1 year ago

Sorry, I do understand the premise of sandboxed applications, but I don't understand restricting images to bitmaps.

To be clearer: is there a reason clients can not send SVGs in the form of bytes (not paths) to the server?

SVGs (and other non-bitmap image formats) require complex processing that has a large attack surface. As just one example, the infamous zero-click FORCEDENTRY iOS exploit used a vulnerability in Core Graphics’s JBIG2 implementation. Even Rust libraries are not immune to vulnerabilities, as shown by the recent arbitrary file read in librsvg. If the server had to implement image decompression and/or rasterization, a code execution vulnerability in the server would result in a sandbox escape. By far the most reliable way to prevent this is to ensure that the server does not need to perform such complex processing on untrusted input. This means that the client (which runs inside the sandbox) is what does any needed processing, and the server just displays the raw bitmap on screen without any processing.

ilya-fedin commented 1 year ago

without any processing

What about scaling up/down?

DemiMarie commented 1 year ago

without any processing

What about scaling up/down?

Scaling is extremely simple when compared to decompressing PNG or JPEG or rasterizing SVGs. It is also part of the Wayland compositor, so this attack surface is not avoidable. The decompression attack surface is avoidable.

andyholmes commented 1 year ago

By far the most reliable way to prevent this is to ensure that the server does not need to perform such complex processing on untrusted input. This means that the client (which runs inside the sandbox) is what does any needed processing, and the server just displays the raw bitmap on screen without any processing.

Thanks, that's more the sort of answer I was after :)

In that case, it would be lovely if there were either a protocol-level hint or spec-level recommendation for clients regarding icon dimensions. Client-side library abstractions may even be able to perform this processing automatically.

tintou commented 1 year ago

On the elementary OS side, we require the use of SVG assets everywhere to make sure that it would look good in mixed DPI situations, so having a spec only supporting raster wouldn't be ideal for us.

jonas2515 commented 1 year ago

I think it'd be a good idea to collect the requirements we have for the image/icon transfer before discussing the technical details:

1) For things like the icon that's displayed together with an action, it should be possible to use named/themed icons. 2) For the main notification icon, we would mainly show app icons or user avatars in case of a messenger. The app icon we can show ourselves in case no icon is passed, so can probably get away without support for themed icons here. 3) Right now there's two kinds of icons (the main icon and the icons for actions). Eventually larger, high-res images will need to be handled when we introduce a larger image preview in the notification. 4) Mixed DPI is a problem indeed. If an app is on a lower DPI monitor than the one we'll show the notification on, we'd likely get a lower-res image asset. That makes a point in favor of using SVG assets whenever possible...

In general, I don't think it's viable to send high-res uncompressed image data over dbus. So we'll have to find ways to do sandboxed decompression on the compositor side anyway. Besides from that, keeping the raw data of high-res images in memory for the possibly weeks that a notification is in the tray seems like a bad idea performance wise.

alyssais commented 1 year ago

In general, I don't think it's viable to send high-res uncompressed image data over dbus.

Couldn't you send it in an fd, rather than directly over dbus?