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

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bluesabre
Copy link
Member

The notification specification defines some additional hints for sounds.

  • sound-file
  • sound-name
  • suppress-sound

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 bluesabre requested a review from a team May 23, 2021 07:38
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Nice work. Can we add settings for this in the demo app please?

@bluesabre
Copy link
Member Author

@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

@bluesabre bluesabre requested a review from danirabbit May 29, 2021 23:41
@danirabbit
Copy link
Member

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
Copy link
Member Author

bluesabre commented Jun 2, 2021

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
Copy link
Member Author

bluesabre commented Jun 2, 2021

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
Copy link
Member

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
Copy link
Contributor

Marukesu commented Jun 2, 2021

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
Copy link
Member Author

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
Copy link

jeremypw commented Mar 6, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants