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.

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

notification: Use categories from xdg desktop notification spec #1500

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

agx
Copy link
Contributor

@agx agx commented Oct 31, 2024

The xdg spec¹ uses slightly different names thus making it hard for apps
to use API like g_notification_set_category in a way that works for
portaled and non-portaled use cases.

It also makes things slighlty more consistent as the current call.*
sub-categories (.incoming, .ongoing) indicate a state while the im
category indicates a type (message).

  1. https://specifications.freedesktop.org/notification-spec/1.2/categories.html

@swick
Copy link
Contributor

swick commented Oct 31, 2024

/cc @jsparber

Technically that would be an API break but the API in question is not part of any release so far.

@jadahl
Copy link
Collaborator

jadahl commented Oct 31, 2024

We can indeed still change things, ABI isn't set in stone until the next stable release.

@jsparber
Copy link
Contributor

I don't thing that the categories will stay consistent over time and inn Gio.Notification we will need to translate them between different backend APIs to make g_notification_set_category() work well for apps. I started doing that in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4027

I find the proposed strings more odd then the current ones, but don't have a strong opinion.

@Mikenux
Copy link

Mikenux commented Oct 31, 2024

I also think the current naming is more... current.

For messages:

  • Is "im.message" only for instant messaging or for messages in general (non-instant like emails)?
  • If statuses should be used for messages, it also means that other statuses should be implemented in the future, such as "error" (but does "error" has value, need specific styling?).

@smcv
Copy link
Collaborator

smcv commented Oct 31, 2024

I don't think it really makes sense to invent a whole new taxonomy of categories here, if it's going to be 90% the same as org.freedesktop.Notifications.

If there's a compelling reason why xdg-desktop-portal Notifications need a completely separate taxonomy, then it should be completely separate, and ideally obviously separate, so that no string is valid in both taxonomies (for example it could use a different delimiter instead of . or something).

Or, if the o.fd.Notifications taxonomy is "good enough" (which it seems that it is, because ours is 95% the same anyway), then we should just use it as-is, even if with hindsight some of the strings are imperfect.

I don't think the middle ground of a taxonomy that is almost-but-not-quite the same as o.fd.Notifications is wise.

@smcv
Copy link
Collaborator

smcv commented Oct 31, 2024

Is "im.message" only for instant messaging or for messages in general (non-instant like emails)?

Honestly, I think the answer to this is: ask the xdg-specs (o.fd.Notifications) maintainers.

And, if we have this problem with im.received, it's equally a problem with our current im.message, so being unhappy with im.* should not block this PR.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

data/org.freedesktop.portal.Notification.xml also mentions these categories, so the same change will be needed there.

@smcv smcv added this to the 1.20 milestone Oct 31, 2024
@smcv
Copy link
Collaborator

smcv commented Oct 31, 2024

This PR needs to be either accepted or intentionally rejected before 1.20.0.

@agx
Copy link
Contributor Author

agx commented Nov 1, 2024

I don't think it really makes sense to invent a whole new taxonomy of categories here, if it's going to be 90% the same as org.freedesktop.Notifications.

That was my thinking as well. If we update this here I want to take the new categories to hopefully get them added to the xdg spec as well (although they're not optimal regarding e.g. the CBS related notifications either)

@jsparber
Copy link
Contributor

jsparber commented Nov 4, 2024

The categories are already part of an asphd release https://bilelmoussaoui.github.io/ashpd/ashpd/desktop/notification/enum.Category.html

@jadahl
Copy link
Collaborator

jadahl commented Nov 4, 2024

The categories are already part of an asphd release https://bilelmoussaoui.github.io/ashpd/ashpd/desktop/notification/enum.Category.html

I don't think we should care about that, that is a ashpd problem of creating a release of an unstable API that can change in any way before xdg-desktop-portal 1.20. The notification version 2 is not yet declared stable.

@agx
Copy link
Contributor Author

agx commented Nov 4, 2024

Related xdg spec MR so we can have the two in sync in case this one can land.

@jsparber
Copy link
Contributor

jsparber commented Nov 5, 2024

I would be fine with this changes but I'm afraid that this gives the wrong impression, that the categories between fdo and portal will stay in sync and the user can uses them interchangeable. Also note long term the idea would be that applications only use the portal for notifications.

Additionally, I said it before already, we will need to make sure that Gio.Notification.set_category() works well for different backends (windows, mac, fdo, xdg-portal, maybe other in future) so keeping the categories in sync between fdo and portal doesn't solve any problem but is more a band-aid to hide the real issue.

@agx
Copy link
Contributor Author

agx commented Nov 5, 2024

There's other libs than glib which would need to differentiate the flatpak from non flatpak case for trivial things like receiving an im. I don't see why we would make life harder for everyone for the categories that do match.

@ebassi
Copy link
Collaborator

ebassi commented Nov 5, 2024

I would be fine with this changes but I'm afraid that this gives the wrong impression, that the categories between fdo and portal will stay in sync and the user can uses them interchangeable.

Why would they be different? We control the portal and the spec.

I said it before already, we will need to make sure that Gio.Notification.set_category() works well for different backends (windows, mac, fdo, xdg-portal, maybe other in future) so keeping the categories in sync between fdo and portal doesn't solve any problem but is more a band-aid to hide the real issue.

Why would this be a problem?

I'm afraid you have explained nothing, and I am missing a ton of context and backstory. Could you explain why would we need a whole new taxonomy of categories, here?

@bilelmoussaoui
Copy link
Member

The categories are already part of an asphd release https://bilelmoussaoui.github.io/ashpd/ashpd/desktop/notification/enum.Category.html

Changing the string representation of the enums variants would not be an API break, if an enum variant gets renamed/removed, I can soft deprecate it or even replace it in the next API breaking release.

@AdrianVovk
Copy link

Why would they be different? We control the portal and the spec.

If this portal exists and apps use it, I don't expect that the FDO spec will continue to get updates. It'll probably stick around in GNOME Shell and Plasma for backwards compatibility, but I don't expect it to be actively developed really by anyone. Not that it was actively developed in the first place: the published version was last updated in October 2010, GNOME has its own prefered notifications API, and KDE's impl of the spec uses vendor additions for new features (like inline replies, or progress bars). New features that will be introduced into the portal won't necessarily make their way to the FDO spec, and thus the list of notification categories will diverge over time.

In other words: this portal replaces the spec, and the spec will be deprecated. It's already unmaintained.

Even if desktop environments choose to use their existing implementations of the FDO spec to communicate between their portal implementation and their notifications UI, they won't really be using the FDO spec anymore. They'll be using a private spec, with a bunch of vendor additions, that happens to be backwards compatible with the FDO spec.

Julian, just curious: have you decided what we'll use to communicate between xdg-desktop-portal-gnome and gnome-shell? Private protocol, or FDO notifications?

I said it before already, we will need to make sure that Gio.Notification.set_category() works well for different backends (windows, mac, fdo, xdg-portal, maybe other in future) so keeping the categories in sync between fdo and portal doesn't solve any problem but is more a band-aid to hide the real issue.

Why would this be a problem?

I think Julian's point was that Glib will need to maintain a per-backend mapping of Glib category names to backend category names anyways. Even if the portal and fdo notifications category names match, GLib will need to translate category names for Windows, macOS, Android, etc.

@jsparber
Copy link
Contributor

I think Julian's point was that Glib will need to maintain a per-backend mapping of Glib category names to backend category names anyways. Even if the portal and fdo notifications category names match, GLib will need to translate category names for Windows, macOS, Android, etc.

Yes, correct. I was concerned with the motivation of this MR which I don't think is fully correct. Anyhow, I think this is a good change and I agree that we don't need a new or different taxonomy for categories in the portal.
@agx Can you add this fact to the commit message and I will merge it :)

Julian, just curious: have you decided what we'll use to communicate between xdg-desktop-portal-gnome and gnome-shell? Private protocol, or FDO notifications?

This isn't relevant for this MR, please don't bikeshedding. We will keep using the private org.gtk.Notifications in the xdg-desktop-portal-gnome and org.freedesktop.Notifications in xdg-desktop-portal-gtk

@agx
Copy link
Contributor Author

agx commented Nov 14, 2024

@agx Can you add this fact to the commit message and I will merge it :)

I dropped the mention of glib as it's not really relevant to the portal (and was just used as an example of a library emitting notifications). Hope that's o.k.

@jsparber
Copy link
Contributor

I'm actually not allowed to merge. @ping @GeorgesStavracas

@jadahl
Copy link
Collaborator

jadahl commented Nov 14, 2024

Lets land this since everyone seems onboard.

@jadahl jadahl enabled auto-merge (rebase) November 14, 2024 09:42
The xdg spec¹ uses slightly different names thus making it hard for apps
to use API in a way that works for portaled and non-portaled use cases.

It also makes things slighlty more consistent as the current call.*
sub-categories (`.incoming`, `.ongoing`) indicate a state while the `im`
category indicates a type (`message`).

1) https://specifications.freedesktop.org/notification-spec/1.2/categories.html

Signed-off-by: Guido Günther <[email protected]>
@jadahl jadahl merged commit 0c5a2f8 into flatpak:main Nov 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

9 participants