After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 639959 - notification: be compatible with various names of the icon data hint
notification: be compatible with various names of the icon data hint
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-01-19 15:34 UTC by Cosimo Cecchi
Modified: 2011-01-19 16:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
notification: be compatible with various names of the icon data hint (1.15 KB, patch)
2011-01-19 15:34 UTC, Cosimo Cecchi
needs-work Details | Review
notification: be compatible with various names of the icon data hint (1.96 KB, patch)
2011-01-19 16:07 UTC, Cosimo Cecchi
reviewed Details | Review

Description Cosimo Cecchi 2011-01-19 15:34:06 UTC
Namely, these are:
- "image-data" in 1.2
- "image_data" in 1.1
- "icon_data" in previous iterations
Comment 1 Cosimo Cecchi 2011-01-19 15:34:10 UTC
Created attachment 178737 [details] [review]
notification: be compatible with various names of the icon data hint

The hint changed its name during various iterations of the
notification daemon spec; be compatible with all of them.
Comment 2 Owen Taylor 2011-01-19 15:41:55 UTC
Review of attachment 178737 [details] [review]:

* I think the code needs to be changed to canonicalize to the latest version, not to the older icon_data that is used currently
* I'd like to see brief comments mentioning versions - so like

 if (hints['icon_data']) // < version 1.1 of spec

or whatever
Comment 3 Cosimo Cecchi 2011-01-19 16:07:56 UTC
Created attachment 178745 [details] [review]
notification: be compatible with various names of the icon data hint

The hint changed its name during various iterations of the
notification daemon spec; be compatible with all of them.
Comment 4 Cosimo Cecchi 2011-01-19 16:08:32 UTC
Here's an updated patch, thanks for the review.
Comment 5 Owen Taylor 2011-01-19 16:20:11 UTC
Review of attachment 178745 [details] [review]:

::: js/ui/notificationDaemon.js
@@ +243,3 @@
+        if (hints['image_data'])
+            hints['image-data'] = hints['image_data']; // version 1.1 of the spec
+        else if (hints['icon_data'])

So, the end result of this logic is that priority is image_data, then icon_data, then image-data if multiple set. Which is weird. I think it's probably good to wrap this all in a 

 if (!hints(['image-data'])) {
    [...]
 }

Setting multiple would be weird of course, but I think it's better to do something logical. OK to commit with that change.
Comment 6 Cosimo Cecchi 2011-01-19 16:32:49 UTC
Attachment 178745 [details] pushed as 68c482e - notification: be compatible with various names of the icon data hint

Pushed with the suggested change, thanks for the review.