Add support for image-data

[bluesabre]

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 elementary/wingpanel-indicator-notifications#193

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

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

[danirabbit]

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]

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.

[davidmhewitt]

Other than my two minor nitpicks, I'm happy with the code.

Would need an extra approval from someone that has tested the behaviour is correct regarding the badging of the icon on the image.

[danirabbit]

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]

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 #124 (comment), 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]

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]

@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]

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]

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]

@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.

[danirabbit]

Wow what a huge difference in the PR. This looks very clean. Nice work. I can confirm it works expected with images sent from Flatpak apps. Great work!

Also, agreed that the fallback image doesn't need to be set if we have image data. Would be nice to get an app icon, but oh well

I think we can figure out doing the flatpak for the demo in another branch

changes were made