Bug 344885 - [Spec 1.2] Support image-path
Summary: [Spec 1.2] Support image-path
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Notifications (show other bugs)
Version: 5.2.1
Platform: Other Linux
: NOR normal
Target Milestone: 1.0
Assignee: Martin Klapetek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-05 22:34 UTC by Konstantin
Modified: 2015-04-19 15:33 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
attachment-19219-0.html (1.01 KB, text/html)
2015-03-06 06:22 UTC, Konstantin
Details
Patch to support dashed hints also (1.33 KB, patch)
2015-03-06 13:46 UTC, Konstantin
Details
Updated version. (1.56 KB, patch)
2015-03-06 14:00 UTC, Konstantin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin 2015-03-05 22:34:03 UTC
According to spec: 
Icons and Images
A notification can optionally have an associated icon and/or image.
The icon is defined by the "app_icon" parameter. The image can be defined by the "image-path", the "image-data" hint or the deprecated "icon_data" hint.
Priorities
An implementation which only displays one image or icon must choose which one to display using the following order:
"image-data"
"image-path"
app_icon parameter
for compatibility reason, "icon_data"
An implementation which can display both the image and icon must show the icon from the "app_icon" parameter and choose which image to display using the following order:
"image-data"
"image-path"
for compatibility reason, "icon_data"

For now, image-path parameters is not handled.

Reproducible: Always

Steps to Reproduce:
- Use GNotifications with icons in plasma.
- Use image-path according to spec.

Actual Results:  
No icon is shown in notification

Expected Results:  
Icon must be shown in notification
Comment 1 Martin Klapetek 2015-03-05 23:02:12 UTC
Thanks for the report

Can you post a link to the spec?
Comment 2 Konstantin 2015-03-06 06:22:40 UTC
Created attachment 91443 [details]
attachment-19219-0.html

Here it is:
https://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html
And here the same:
https://developer.gnome.org/notification-spec/
On Mar 6, 2015 5:02 AM, "Martin Klapetek" <mklapetek@kde.org> wrote:

> https://bugs.kde.org/show_bug.cgi?id=344885
>
> --- Comment #1 from Martin Klapetek <mklapetek@kde.org> ---
> Thanks for the report
>
> Can you post a link to the spec?
>
> --
> You are receiving this mail because:
> You reported the bug.
>
Comment 3 Martin Klapetek 2015-03-06 11:20:00 UTC
Right, so that's Gnome's specific extension of the notification standard and we've already decided that we won't support it as it's fully in Gnome's control and can change any time without giving notice, can break any time Gnome decides to change things etc.

If this should become more widespread, it should go to the proper xdg places.

Sorry.
Comment 4 Konstantin 2015-03-06 12:29:01 UTC
Some note about notification spec. It is not a gnome extension.
Here is a proof in XDG mailing list:
http://lists.freedesktop.org/archives/xdg/2015-February/013456.html
And here is spec in libnotify git tree:
https://github.com/GNOME/libnotify/blob/master/docs/notification-spec.xml
Comment 5 Martin Klapetek 2015-03-06 12:38:04 UTC
It's an extension that was designed by Gnome and Gnome only (afaik, for sure KDE was not involved there). As such, it still is Gnome's extension.
Comment 6 Konstantin 2015-03-06 12:52:32 UTC
It will not widespread if all developers will count it as a GNOME extension.
Can I see KDE version of org.freedesktop.Notifications spec? I want to make my app compatible with KDE also.
Comment 7 Martin Klapetek 2015-03-06 13:06:50 UTC
It will not widespread if it will be only under Gnome's control. I don't mean to be mean but they do tend to remove things randomly here and there. And that's perfectly fine, but not if things are supposed to be supported by others (namely we've hit a roadblock with their theming support as well as the CSD and all the bugs it caused in Plasma while Gnome's stance being "not our problem"). I sure don't want to end up at the same place wrt notifications.

It should go through proper collaboration and review and things.

The original spec is at http://www.galago-project.org/specs/notification/
Comment 8 Konstantin 2015-03-06 13:13:07 UTC
So, you use version 0.9 of this spec. Ok.

But this is not request for removal, but request for adding new hint. This is logical, AFAIK, because in 0.9 it is image_data (image-data in 1.2), and image-path is simply a way to set image-data to fdo compilant icon name (or filesystem path). I can add support of this myself, I think this will be easy.
Comment 9 Martin Klapetek 2015-03-06 13:29:12 UTC
I'd like to avoid implementing custom things that are not in the spec.

In fact, icons should always be installed into proper places if you want to ensure things will all work correctly.
Comment 10 Konstantin 2015-03-06 13:34:55 UTC
It is in the spec. Your daemon reports that it supports version 1.1 of the spec:
```
// FIXME: Signature is ugly
QString NotificationsEngine::GetServerInformation(QString& vendor, QString& version, QString& specVersion)
{
    vendor = "KDE";
    version = "2.0"; // FIXME
    specVersion = "1.1";
    return "Plasma";
}
```
Just checked version 1.1 of the spec in libnotify git - image_path was there.  Change between 1.1 and 1.2 is cosmetic: support dashes-named properties instead of underscores-named.

And to note: image-path property is not only about paths. It also about freedesktop.org compilant icon names, as spec (in libnotify git) stats.
For example, one can set image-path to "audio-volume-mute", and daemon must shows icon of strikethrough speaker on notification.
Comment 11 Konstantin 2015-03-06 13:38:34 UTC
and so, in plasma it has:
```
    if (hints.contains("image_data")) {
        QDBusArgument arg = hints["image_data"].value<QDBusArgument>();
        image = decodeNotificationSpecImageHint(arg);
    } else if (hints.contains("image_path")) {
        QString path = findImageForSpecImagePath(hints["image_path"].toString());
        if (!path.isEmpty()) {
            image.load(path);
        }
    } else if (hints.contains("icon_data")) {
        // This hint was in use in version 1.0 of the spec but has been
        // replaced by "image_data" in version 1.1. We need to support it for
        // users of the 1.0 version of the spec.
        QDBusArgument arg = hints["icon_data"].value<QDBusArgument>();
        image = decodeNotificationSpecImageHint(arg);
    }
```
Comment 12 Konstantin 2015-03-06 13:46:04 UTC
Created attachment 91448 [details]
Patch to support dashed hints also

Here is plasma-workspace patch to adopt a dahsed hints.

plasma already has support image_path property. All work that was needed - adopt it to change underscore into a dash. Do you like it?
Comment 13 Martin Klapetek 2015-03-06 13:46:54 UTC
Hah, so we already even have support for that, who knew.

Why don't you just use image_path then?
Comment 14 Martin Klapetek 2015-03-06 13:52:18 UTC
Wait, do I understand it correctly that 1.2 /removed/ the underscore values?
Comment 15 Konstantin 2015-03-06 13:55:09 UTC
I use GLib API (in fact, in XFCE), and GLib sends notifications according by version 1.2. it sends an icon in image-path property.
When I discover it, I already has a discussion in a GLib bugtracker about it. Here is a link:
https://bugzilla.gnome.org/show_bug.cgi?id=745634.
Just for summary: GLib does not want to use "deprecated" undersore values and app icon.
It is not removed from spec now, but cleanly stated that newly written programs (from 28 October 2010) must support a dashed values. Underscores only stays here for compatibility reason, like icon_data.
Comment 16 Konstantin 2015-03-06 14:00:36 UTC
Created attachment 91449 [details]
Updated version.

It is an updated version with correct spec_version
Comment 17 Martin Klapetek 2015-03-06 14:07:52 UTC
Sigh, that's exactly what the server version is for. If it detects older version (which it clearly does, unless it skips it altogether), it should not use features newer than the version it actually talks to. Imho that's GNotification bug.

I don't think it's that simple to just raise the version and be done; I'll look over what the 1.1/1.2 all expects to have and see what all is missing and what should we do with it.
Comment 18 Konstantin 2015-03-06 14:20:21 UTC
GNotification skips version altogether, assumes than 1.2 is supported.

Additions of 1.2:
1. Dashes replaces uderscores everywhere
2. "resident" boolean hint. It cleanly stated that notification will not automatically dismissed when action is activated.
3. "persistence" capability and "transient" hint. It does not makes sense to KDE, because KDE notification daemon does not return "persistence" capability. But if you want, you can implement it.
Comment 19 Martin Klapetek 2015-03-06 14:23:56 UTC
Well persistence should be done by setting the timeout to 0, as the spec says.

It makes me really reluctant however to fix our server to follow Gnome's extension only because GNotification does not work properly. That's really not our bug.
Comment 20 Konstantin 2015-03-06 14:28:31 UTC
But why dashed names is GNOME extension? I does not understand. I think this is freedesktop spec, not GNOME. XDG mailing list says that libnotify is reference implementation of org.freedesktop.Notifications spec and spec is hosted in libnotify git.

And next, GNOME does not use org.freedesktop.Notifications anymore. It have private bus named org.gtk.Notifications.
Comment 21 Konstantin 2015-03-06 14:32:04 UTC
1.2 persistence means that notifications must stored across daemon restarts.
For example, i send "resident" notification to daemon, kill it and start it, notification must shown again (or be in daemon database and accessible by user).
"transient" hint means that notification follows old behaviour (removed after daemon kill)
Comment 22 Martin Klapetek 2015-03-06 14:42:36 UTC
Proper xdg standards additions get proper xdg reviews (or at least I'd like to think so). The fact that it's hosted in Gnome's git and actually no KDE developer has (write) access to it and changes have to go through Gnome's review (that's my assumption though), makes it a Gnome extension.

If it's claimed as an xdg extension, freedesktop.org is still there with all the other specs, I see no reason why the notification one could not join.
Comment 23 Konstantin 2015-03-06 14:48:08 UTC
Citation from Sanel Zukan in XDG mailing list:
```
Because fd.o admin isn't responsive enough when there are problems.
Which happens often with the specs. And because it was always hosted
within libnotify.
```
And next, galago's last spec version was 0.9. KDE supports 1.1, dashed values added in 1.2.

Will dashed names be supported in KDE 5.3?
Comment 24 Martin Klapetek 2015-03-06 14:52:33 UTC
As I said already, first I need to look what is all missing. I will not declare support for something when we don't actually support it.

Second, it's GNotification's bug.
Comment 25 Konstantin 2015-03-06 15:03:57 UTC
Added GLib bugreport for GNotifications: https://bugzilla.gnome.org/show_bug.cgi?id=745749
Comment 26 David Edmundson 2015-04-07 18:37:50 UTC
Martin, shall we merge the patch; even if we just handle image-path without bumping the spec. 
The patch itself is fine.
Comment 27 Martin Klapetek 2015-04-07 19:09:37 UTC
Ok.
Comment 28 David Edmundson 2015-04-08 10:49:26 UTC
Git commit 8d4dd0bb9b491c2025664b86ded4a76d41d56d13 by David Edmundson.
Committed on 08/04/2015 at 10:49.
Pushed by davidedmundson into branch 'master'.

Support hints from notification spec 1.2

Even though the specification makes it perfectly clear that you should
query the notification server and only use supported hints, gnome and
libnotify does not.

Given the specification has changed in a non-compatiable way, this
causes image-data/image-path to break.

This patch supports 1.2 fields

Submitted by: Konstantin

Reviewed-by: David Edmundson

M  +12   -1    dataengines/notifications/notificationsengine.cpp

http://commits.kde.org/plasma-workspace/8d4dd0bb9b491c2025664b86ded4a76d41d56d13
Comment 29 David Edmundson 2015-04-19 15:33:26 UTC
The gnome devs seemed to have "forgotten" about making it a specification on fd.o

I've opened a new bug at: https://bugzilla.gnome.org/show_bug.cgi?id=748145