search

elementary / notifications

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

Add support for image-data #123

Closed bluesabre closed 3 years ago

bluesabre commented 3 years ago

This PR adds support for the image-data notification hint. Spotify uses this for notification images, and it's part of the specification, so it seems like something we should add. This may also require updates to the indicator. Draft PR for now while I tidy it up.

Loosely related to https://github.com/elementary/wingpanel-indicator-notifications/issues/193

Specification: https://developer.gnome.org/notification-spec/#hints

Screenshot from 2021-05-22 08-02-56

@elementary/ux question, should the dialog-information icon not be displayed if we manage to source an image?

danirabbit commented 3 years ago

Is that what LoadableIcon is for?

The badge is intended to show the app icon so that you know which app is sending the notification in addition to the image data. We could probably not show the badge instead of showing a fallback icon, but it should still show if it's a valid app icon

bluesabre commented 3 years ago

Is that what LoadableIcon is for?

The badge is intended to show the app icon so that you know which app is sending the notification in addition to the image data. We could probably not show the badge instead of showing a fallback icon, but it should still show if it's a valid app icon

From what I understand, I think LoadableIcon would be overkill here. The image data is already defined and the pixbuf should be able to load quickly from it.

I tweaked the overlay logic to not add the badge when the fallback icon is used. If no image is found, the fallback icon will still be displayed for the notification.

danirabbit commented 3 years ago

Can we make sure to add a test for this to the demo app? That would help to make sure we don't regress here in the future as well

bluesabre commented 3 years ago

Can we make sure to add a test for this to the demo app? That would help to make sure we don't regress here in the future as well

Similar to https://github.com/elementary/notifications/pull/124#issuecomment-848382533, adding a demo for the image-data will require using Notify.Notification instead. If that gets merged in, the patch to add this test is pretty lightweight:

demo_images.txt

danirabbit commented 3 years ago

After reading through the Notification portal spec, it sounds like the expectation for apps sending through image data is to use a GLib.BytesIcon

See the icon section here: https://docs.flatpak.org/en/latest/portal-api-reference.html#gdbus-method-org-freedesktop-portal-Notification.AddNotification

davidmhewitt commented 3 years ago

@danrabbit Yep, that's the spec for applications to send notifications to the portal (if they're using it), not the spec for this method on the FDO notification server.

If we look at the code for the notifications portal, we can see that it's taking that BytesIcon, converting it into a pixbuf and then converting it to raw data to put on the image-data hint and send to this DBus interface: https://github.com/flatpak/xdg-desktop-portal-gtk/blob/f6c3122a0796fdb9069e8f0a48f8f364bd95503a/src/fdonotification.c#L290-L331

The code in this PR is doing the reverse and turning the raw data back into a pixbuf. It may be possible to turn that back into a BytesIcon, but since we need it as a pixbuf anyway, it's probably an extra step we don't need.

danirabbit commented 3 years ago

Sorry that wasn't clear! I was thinking in terms of the demo. But reading the code for GLib.Notification it looks like it uses a file icon and passed the image on the image-path property. Is the expectation that flatpak'd apps use libportal to send image data?

Marukesu commented 3 years ago

Sorry that wasn't clear! I was thinking in terms of the demo. But reading the code for GLib.Notification it looks like it uses a file icon and passed the image on the image-path property. Is the expectation that flatpak'd apps use libportal to send image data?

that's for the native backend. they accept only a FileIcon or ThemedIcon and uses the image-path hint on both. when using the portal, they use image-data hint for BytesIcon and the app_icon parameter for FileIcon and ThemedIcon.

i don't know if the notification portal is usable for non-sandboxed apps, so we probably need to make the test application a flatpak.

bluesabre commented 3 years ago

@danrabbit I've adapted my branch for the latest changes to image handling. It's simplified quite a bit now:

  1. Raw image data sent over a variant (image-data, image_data, or icon_data) is converted to a pixbuf. This pixbuf is loaded into MaskedImage.
  2. The generic dialog-information icon is only displayed if an image isn't loaded and a primary icon isn't found. I can remove this change if you'd prefer, but I don't think the generic icon adds any context as a badge.