GNOME Bugzilla – Bug 621009
Add background image to notifications
Last modified: 2011-08-29 17:59:53 UTC
Created attachment 163094 [details] [review] Adds background image to notifications Simply adds the posibility to stick an image behind the contents of a notification. Makes notifications use the sending app's icon. This along with another patch I'm finishing is a step towards the bttom mockup: http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/MusicPlayer
Created attachment 163114 [details] [review] Fix typo and clean out a comment Already noticed a bug, this one seems to be working
(In reply to comment #1) > Already noticed a bug, this one seems to be working Wrong patch? This one looks suspiciously like bug 621014 ... BTW: you should use git-format-patch instead of git-diff (includes your name/email and a nice commit message). If you haven't done so already, check out git-bz - it makes working with bugzilla a _lot_ more comfortable.
Created attachment 163433 [details] [review] Add background image to notifications This allows a better approximation of the music player mockups and hopefully will be useful for other things further down the line.
Created attachment 165002 [details] [review] Add background image to notifications Changes the way we handle all the various ways images can be used in notifications. Now only the icon parameter gets to be the icon in the corner of the notification. The hints image_data, image_path, and icon_data specify background images and are respected in that order.
(In reply to comment #4) > Now only the icon parameter gets to be the icon in the > corner of the notification. The hints image_data, image_path, and > icon_data specify background images and are respected in that order. Have you investigated how that affects other notification clients? Eg, on Fedora 13, "repoquery --whatrequires libnotify.so.1" gives 114 packages.
Created attachment 165920 [details] [review] Add background image to notifications After the last comment, I went back and checked to make sure I was completely following the notification spec. I think I'm now in full compliance and that the code is also a bit more organized.
(In reply to comment #5) > (In reply to comment #4) > > Now only the icon parameter gets to be the icon in the > > corner of the notification. The hints image_data, image_path, and > > icon_data specify background images and are respected in that order. > > Have you investigated how that affects other notification clients? Eg, on > Fedora 13, "repoquery --whatrequires libnotify.so.1" gives 114 packages. Unfortunately, the number of things I can get to notify things for me is smallish... but as far as I can tell, things work fine for pidgin, xchat-gnome, rhythmbox, empathy and the libnotify tests.
Well... this is certainly less likely to cause problems, since apps shouldn't currently be setting both icon and image_data. But it still seems odd. It means, for example, that rhythmbox can show a background image, but empathy cannot (because empathy uses the avatar image as the icon, and so needs to use image_data to pass that). Why not just have an explicit background_image hint?
Why can't empathy show a background image, then? It can supply an icon and also pass any of those hints. It can even supply a uri as the icon parameter or the name of any named icon, so, I don't understand why you think this will limit empathy somehow.
Empathy *needs* to use image_data to describe its icon, because the icon it's using is actually the user's avatar image, which only exists as raw image data. There's no stock icon name or URI it could pass. Well, I suppose it could write the image out to disk, and then pass a URI to that file... (In fact, it does write the image out to disk, for caching purposes.) But it seems odd to force it to do that, when you could instead just keep using image_data like it's used now, and add new background_icon/background_image_data hints to explicitly specify a background image.
Created attachment 189707 [details] [review] Sticks background image to the notification for rhythmbox This currently uses image_path as the hint, since the image-data hint for rhythmbox isn't being recognized in notificationDaemon.js. The UI needs to be improved based on the review.
Review of attachment 189707 [details] [review]: > This currently uses image_path as the hint, since the image-data hint for > rhythmbox isn't being recognized in notificationDaemon.js. Is it that rhythmbox isn't sending the image-data hint, or is it a limitation in GJS or our DBus wrapper? ::: js/ui/messageTray.js @@ +648,3 @@ + this._imageBin = new St.Bin({ x: 1, y: 1, width: 125, height: 125 }); + this._imageBin.child = image; + this._imageBin.opacity = 230; It's much better if you give the bin a style_class instead of setting these properties directly from code: this._imageBin = new St.Bin({ style_class: 'notification-background', x: 1, y: 1, width: 125, height: 125 }); And then add to data/theme/gnome-shell.css: .notification-background { opacity: 0.9; } ::: js/ui/notificationDaemon.js @@ +336,3 @@ + if (icon && (hints['image-data'] || hints['image_path'] || hints['icon_data'])) { + let image = null; + if (hints['image_path']) Above, you use 'image-path'. Here, you seem to be using 'image_path'. It's probably better to do something like: let imagePath = hints['image-path'] || hints['image_path']; above, and then using imagePath down here. @@ +338,3 @@ + if (hints['image_path']) + image = St.TextureCache.get_default().load_uri_async(GLib.filename_to_uri(hints['image_path'], null), IMAGE_SIZE, IMAGE_SIZE); + notification.setImage(image); This line adds some whitespace at the end. Remove it please. @@ +340,3 @@ + notification.setImage(image); + } else + notification.unsetImage(); Style nit: if one "branch" needs braces, any related ifs/else ifs/elses need them too.
(In reply to comment #12) > Review of attachment 189707 [details] [review]: > > > This currently uses image_path as the hint, since the image-data hint for > > rhythmbox isn't being recognized in notificationDaemon.js. > > Is it that rhythmbox isn't sending the image-data hint, or is it a limitation > in GJS or our DBus wrapper? Rhythmbox doesn't send it.
Created attachment 189710 [details] [review] Sticks background image to the notification for rhythmbox (In reply to comment #12) Rhythmbox appears to be sending only the image_path hint. Setting imagePath gives "imagePath is not defined" reference error,there's a need to find a better way of handling hints with rhythmbox and all other apps concerned in future.
(In reply to comment #12) > ::: js/ui/messageTray.js > @@ +648,3 @@ > + this._imageBin = new St.Bin({ x: 1, y: 1, width: 125, height: 125 }); > + this._imageBin.child = image; > + this._imageBin.opacity = 230; > > It's much better if you give the bin a style_class instead of setting these > properties directly from code: > > this._imageBin = new St.Bin({ style_class: 'notification-background', > x: 1, y: 1, width: 125, height: 125 }); > > And then add to data/theme/gnome-shell.css: > > .notification-background { > opacity: 0.9; > } You got that backwards - width and height can be set in the stylesheet, but not opacity (we had a patch for that at some point, but it was rejected in https://bugzilla.gnome.org/show_bug.cgi?id=607821#c16)
Review of attachment 189710 [details] [review]: My bad... the 'opacity' trick doesn't work... I'll review the rest of it as if it never happened. Sorry. ::: js/ui/notificationDaemon.js @@ +333,3 @@ } + if (icon && (hints['image-data'] || hints['image_path'] || hints['icon_data'])) { What does icon or the 'icon_Data' have to do with this? @@ +336,3 @@ + let image = null; + if (hints['image_path']) + image = St.TextureCache.get_default().load_uri_async(GLib.filename_to_uri(hints['image_path'], null), IMAGE_SIZE, IMAGE_SIZE); This is still incorrect. You're setting 'image-path' above, but using 'image_path' here.
Comment on attachment 165920 [details] [review] Add background image to notifications Marking rejected to get off patch review list.
Created attachment 191748 [details] [review] Sticks background image for rhythmbox notifications (In reply to comment #16) The hints for image data and image path are stored in hints['image-data'] and hints['image-path'] respectively. The patch doesn't check for "icon" since we aren't falling back on image-data or image-path for notification's icon in _iconForNotificationData().
Review of attachment 191748 [details] [review]: Your commit message should be a bit better. This isn't Rhythmbox-specific, and definitely should not have "(In reply to comment #16)". Try: Implement background images for notifications Background images are part of the fd.o notifications spec. Add support for them in the Shell. ::: js/ui/messageTray.js @@ +645,3 @@ + setImage: function(image) { + if (this._imageBin) + this._imageBin.destroy(); You should have "this._imageBin" in the _init method of the class, and probably call this.unsetImage() instead of just doing a destroy (you don't remove it from its parent, for instance). @@ +646,3 @@ + if (this._imageBin) + this._imageBin.destroy(); + this._imageBin = new St.Bin({ x: 1, y: 1, width: 125, height: 125 }); Why the x: 1, y: 1? You should probably add this as a style, though. Modify gnome-shell.css and add a "style_class": this._imageBin = new St.Bin({ style_class: "notification-image" }); @@ +651,3 @@ + this._table.add(this._imageBin, { row: 1, col: 3 }); + this._imageBin.lower_bottom(); + this._table.raise_top(); What are these lowering and raising doing? @@ +657,3 @@ + if (this._imageBin) { + this._table.remove_actor(this._imageBin); + this._imageBin.destroy(); You should set this._imageBin to null. ::: js/ui/notificationDaemon.js @@ +215,3 @@ + // Be compatible with the various hints for image data and image path + // 'image-data' and 'image-path' are the latest name of these hints, introduced in 1.2 This is getting to be a bit spaghetti-ish. A comment explaining the relative priorities of each of the hints would be nice. Instead of setting hints['image-data'] or hints['image-path'], I'd do: let image_path = hints['image-path'] || hints['image_path']; if (image_path) { let image = ...; notification.setImage(image); return; } let image_data = hints['image-data'] || hints['image_data'] || hints['icon_data']; if (image_data) { let image = ...; notification.setImage(image); return; } @@ +342,3 @@ + notification.setImage(image); + } else { + notification.unsetImage(); This is only going to happen if a notification gets updated, right? I'd add a comment explaining that you need to destroy the previous actor.
Review of attachment 191748 [details] [review]: ::: js/ui/notificationDaemon.js @@ +334,3 @@ + let [width, height, rowStride, hasAlpha, + bitsPerSample, nChannels, data] = hints['image-data']; + image = St.TextureCache.get_default().load_from_raw(data, data.length, hasAlpha, Oh, and as I was writing this review, gcampax pushed a fix for bug 654349, so we need to adapt here. (drop data.length).
Neha, can you please incorporate Jasper's comments. Except, I don't agree with needing to change how we handle deprecated hints. The idea is that we handle the deprecated hints as soon as we get the notification, and then we can just rely on having the right values for hints in _notifyForSource(). Also, I played around with the code to achieve the look that is closer to the mockup here: https://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/MusicPlayer I'll attach a patch for it that applies on top of your patch, as well as a few screenshots to get some feedback from the designers.
Created attachment 191764 [details] [review] Place the background image behind the notification content The previous patch was placing the background image to the right of the notifcation content, but it should be placed behind it instead.
Created attachment 191765 [details] Rhythmbox notification - example 1
Created attachment 191766 [details] Rhythmbox notification - example 2, the text goes all the way over the image
Created attachment 191767 [details] Rhythmbox notification in the banner mode - should the top of the image be visible?
Created attachment 191768 [details] Rhythmbox notification - example 3, with regular action buttons We won't only be using the background image for Rhythmbox notifications, and some other notifications might have regular buttons.
Created attachment 191769 [details] [review] Place the background image behind the notification content The previous patch was placing the background image to the right of the notifcation content, but it should be placed behind it instead. Sets y_expand and y_fill for the action area to false, so that the buttons have the right size.
Just a few comments. The second screenshot shows an action selected by default. We are planning to remove that. I think we shouldn't show the background image in the banner mode, but instead fade it in when the notification is expanded. The mockup shows the background image centered with respect to the notification controls. It would be somewhat tricky to do so for arbitrary controls. There is also a case when a notification does not have any controls. Right now, the background image is aligned to the right of the notification, so it has the same padding as the notification text would have. We should also consider how the background image looks if controls are regular buttons - I've attached a screenshot with that case too.
In the case of a music app I really dislike having the album art at the background at all. It belongs to the foreground just like the song title does. Album art isn't a low frequency abstract texture that makes the forgeround legible, quite the contrary. You most certainly don't want to do this for the music app case. Please aim for the foreground: http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-music-playback.png
(In reply to comment #30) > Please aim for the foreground: > > http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-music-playback.png Looks sexy, but I don't see how that would be possible using libnotify (unless we change the protocol radically, which is not really an option). So maybe we should support MPRIS[0]? [0] http://www.mpris.org/2.1/spec/
Florian, I think Jakub said at some point that we don't have to have the progress bar. Jakub, that mockup looks good, but there are a few concerns about how the notification should look in certain cases. 1) How should the banner notification look? Right now, we rely on the small icon being visible in it to help the user identify what is the source of the notification. Also, when the notification is expanded, we just raise it up to reveal the rest of it, so the top portion should stay relatively constant. We do have an option of fading something in/out. For example, right now we fade out the body text that we display ellipsized after the title, and move it to the next line. 2) When we stack notifications from a particular source in the summary view, we put the icon for the source on the left, and stack the content of all the notifications on the right. How would a stack of notifications with images or where some have images and some don't look? 3) The notification spec suggests displaying both the icon and the image in notifications. Omitting the icon is not as big of a deal for summary notifications, because we display the source there, but the icon seems very useful for the new notifications that are displayed in the center. Beyond that, so far, we've kept the style of both new and summary notifications the same, with both displaying the icon. http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html#icons-and-images
Hopefully this addresses all of the issues above: http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-music-playback-banner-collapsed.png http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-music-playback-banner.png http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-music-playback.png
Created attachment 192413 [details] [review] Implement background images for notifications Background images are part of the notifications spec. Add support for them in the Shell.
Review of attachment 192413 [details] [review]: This patch needs some more testing. In particular, the one-line new notification slightly moves up when you mouse over it, while it should not
Review of attachment 192413 [details] [review]: ::: js/ui/messageTray.js @@ +447,3 @@ + this._table.add(this._notificationDetails, { row: 1, col: 1 }); + this._notificationDetailsTable = new St.Table({ name: 'notification-details-table' }); + this._notificationDetails.add(this._notificationDetailsTable, { row: 0, col: 1 }); Having two new actors, "notificationDetails" and "notificationDetailsTable", in combination with the existing actor "table", seems very confusing. Why can't you do this as one table, but with rowspan/colspan? ::: js/ui/notificationDaemon.js @@ +340,3 @@ + } else if (hints['image-path']) { + image = St.TextureCache.get_default().load_uri_async(GLib.filename_to_uri(hints['image-path'], null), + IMAGE_SIZE, IMAGE_SIZE); You indentation here is a bit weird.
Created attachment 192420 [details] [review] Implement background images for notifications Background images are part of notifications spec. Add support for them in the Shell.
Review of attachment 192420 [details] [review]: The image is stretched and layout needs some more tweaking
Reassigning; Marina is on vacation and Jasper reviewed the last version of the patch anyway.
(In reply to comment #38) > Review of attachment 192420 [details] [review]: > > The image is stretched and layout needs some more tweaking If you've fixed this, ACN. The code looks good.
(In reply to comment #40) > If you've fixed this, ACN. The code looks good. It turned out that st-table.c is ignoring height of anything with row_span > 1, so use multiple tables again?
Created attachment 194988 [details] [review] messageTray: implement showing images in notifications Updated Neha's patch to apply cleanly to master, removed one trailing whitespace, and removed the mention of the image serving as a background image from the commit message.
Created attachment 194994 [details] [review] Fix the layout of a notification containing an image This patch applies on top of the previous patch to fix the layout of a notification containing an image. It should be merged in. It adds x_expand: false when adding this._bannerBox to the table, which ensures that the image cell that is placed in the same column doesn't expand, but only takes as much space as it needs. Further, it removes row_span: 2 for the image cell and adds a BoxLayout to put this._scrollArea and this._actionArea in, so that there is only one cell on the right of the image. This is necessary because the StTable code ignores the preferred height of cells with row_span > 1, so that height ends up being limited by the combined height of the other cells that fill the corresponding rows. In the case with Rhythmbox notifications, the image was not getting the necessary height because the combined height of the track info and playback controls was less than the height of the image.
Created attachment 195003 [details] [review] Fix the layout of a notification containing an image Same as previous patch. Just adds the spacing for the BoxLayout.
Created attachment 195005 [details] [review] Moves IMAGE_SIZE to the Notification class to be consistent with how we define ICON_SIZE for Source. This patch should be merged with the other two patches.
Created attachment 195028 [details] [review] messageTray: implement showing images in notification Images are part of notification spec, so we should support them. Marina Zhurakhinskaya provided some code for getting the layout right for this patch.
Created attachment 195035 [details] [review] Make sure we only add cells to the table if they will have content This ensures that notifications expand only if they need to and that there is no unnecessary spacing. There is still a bug in that when you move between tracks using the playback controls in a new notification, the updated notification does not get expanded full height. I'll continue looking into this tomorrow.
Created attachment 195083 [details] [review] Fix the layout of the notification containing an image This patch sets min_height for the notification containing an image to ensure that it's given enough vertical space. It also sets the column numbers correctly to avoid unnecessary spacing added by empty columns. This patch applies on top of attachments 194988 and 195005. This is an alternative approach to the one that adds a BoxLayout and, as far as I can tell, it works well.
Created attachment 195093 [details] [review] messageTray: Implement showing images in notifications Images are part of notification spec, so we should support them. Marina Zhurakhinskaya provided some code for getting the layout right for this patch.
Comment on attachment 195093 [details] [review] messageTray: Implement showing images in notifications Looks good
Review of attachment 195093 [details] [review]: Pushed it.