elementary / notifications

Gtk Notifications Server
https://elementary.io
GNU General Public License v3.0
38 stars 6 forks source link

Add support for sound-name and suppress-sound hints #124

Open bluesabre opened 3 years ago

bluesabre commented 3 years ago

The notification specification defines some additional hints for sounds.

This PR adds support for sound-name and suppress-sound. Supporting sound-file may be a bit more involved since Canberra doesn't seem to natively support files.

Testing sound-name (and category to verify no breakage) can be done with notify-send:

# category
notify-send --category=email test test
notify-send --category=im.received test test

# sound-name
notify-send --hint=string:sound-name:message-new-instant test test
notify-send --hint=string:sound-name:service-login test test

Testing suppress-sound is trickier since notify-send doesn't support boolean hints.

gdbus call --session \
    --dest org.freedesktop.Notifications \
    --object-path /org/freedesktop/Notifications \
    --method org.freedesktop.Notifications.Notify \
    "test" "42" "dialog-information" "test" "test" \
    '[]' '{"suppress-sound":<true>}' "5000"
bluesabre commented 3 years ago

@danrabbit I added tests for category, sound-name, and suppress-sound to the demo app. Since GLib.Notification doesn't support those hints, I added a new section for tests that specifically use libnotify. I would have replaced GLib.Notification for Notify.Notification, but the latter doesn't support all 4 priority states or notification ids.

Screenshot from 2021-05-25 21-08-13

danirabbit commented 3 years ago

I'm kind of hesitant about adding features that are outside the scope of GLib.Notification. I'm worried about scope creep but also what our recommendation is to developers who want to use the full set of supported features. As far as I know, LibNotify doesn't require apps to identify themselves, which is pretty problematic in terms of the notification center and allowing users to control if notifications are sent at all.

The notifications specification linked is out of date. Probably a more up-to-date spec is the notification portal spec, but realistically the library itself kind of is the spec currently.

If there are additional features here that we want to support, I wonder if it would be worth pursuing a Granite.Notification class so that we have a recommendation for developers who want to use these features

bluesabre commented 3 years ago

Yeah, it's definitely an interesting/difficult place to be. Flatpak'd applications seem to be a bit more restricted with the notifications portal. Whereas any non-sandboxed application have access to more powerful notification options, particularly hints that allow for controlling notification sound and image options. To get it all in one place, I've compiled the different combinations below.

FreeDesktop.org Notification Spec

The FreeDesktop.org specification lists the following components: Application Name, Replaces ID, Notification Icon, Summary, Body, Actions, Categories, Urgency (Low, Normal, Critical), Hints, Expiration Timeout

Libnotify

Libnotify.Notification seems to most closely match the the FD.o specification, supporting all but Replaces ID (from what I can tell): Application Name, Notification Icon, Summary, Body, Actions, Urgency (Low, Normal, Critical), Hints, Expiration Timeout

Portal / GLib.Notification

The notification portal and GLib.Notification support a subset of the FD.o spec, with an added urgency level: Replaces ID, Notification Icon, Summary, Body, Actions, Urgency (Low, Normal, High, Urgent)

And exclude support for: Application Name, Categories, Hints, Expiration Timeout

bluesabre commented 3 years ago

Expanding just a bit more (sorry for the notification spam), suppress-sound and image-data (#123) are probably the most impactful hints that are not currently supported.

suppress-sound is sent from Chromium web browsers for sites using their own notification sound, such as IRCCloud. Without suppression or adjusting site settings, each notification sound is both from the site and from the default system notification sound and the experience is sub-par. I can also see a use case where applications would want to provide their own notification sounds to customize the experience (it just occurred to me, this should be with sound-file, so the notification server could still control it).

image-data adds context to the notification, and allows sending an image without first writing it to the filesystem, or loading the image from difficult to reach part of the filesystem. This can be useful for showing album artwork for streaming audio players like Spotify, but I can also imagine this being useful for other applications as well. The notification server currently supports images, but only with the image-path hint. This particular hint is also outside of the GLib.Notification scope.

If we want to add support for these hints to the notification server, should we first create Granite.Notification with access to the expanded notification options?

danirabbit commented 3 years ago

AppCenter is all Flatpak now and our recommended sideload method is via Flatpak, so I think it's reasonable that we focus on what's possible/recommended from inside Flatpak.

Yeah there's been kind of a lot of upstream discussion about notification sounds with no real resolution as far as I remember: https://gitlab.gnome.org/GNOME/glib/-/issues/1340

I imagine the expectation is that apps now send a GBytesIcon for images like album art. image-path probably makes sense as a candidate for removal since that wouldn't really make sense for a sandboxed app to send a file path.

You wouldn't need to set Application Name in Glib.Notification or the Portal since apps are required to send their ID and you can get all of that from AppInfo.

Marukesu commented 3 years ago

AppCenter is all Flatpak now and our recommended sideload method is via Flatpak, so I think it's reasonable that we focus on what's possible/recommended from inside Flatpak.

it still possible to add --talk-name=org.freedesktop.Notifications and interact with it directly though.

bluesabre commented 3 years ago

it still possible to add --talk-name=org.freedesktop.Notifications and interact with it directly though.

That's good to know. It looks like there ~200 applications in Flathub that are using this. I definitely think it'd be a step backwards to reduce support to just GLib.Notification's feature scope.

jeremypw commented 2 years ago

I agree that supporting more of the FDO spec is desirable in order to accomodate existing apps using these features. If a Granite.Notification class can make it easier so much the better - we can just recommend developers use that. Ideally should not need an explicit extra dependency on libnotify here and the demo app should work as a Flatpak.