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 621009 - Add background image to notifications
Add background image to notifications
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Jasper St. Pierre (not reading bugmail)
gnome-shell-maint
Depends on:
Blocks: 630847 657077
 
 
Reported: 2010-06-08 19:33 UTC by Matt N
Modified: 2011-08-29 17:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds background image to notifications (3.53 KB, patch)
2010-06-08 19:33 UTC, Matt N
none Details | Review
Fix typo and clean out a comment (1.98 KB, patch)
2010-06-08 21:31 UTC, Matt N
none Details | Review
Add background image to notifications (4.09 KB, patch)
2010-06-11 20:55 UTC, Matt N
none Details | Review
Add background image to notifications (6.42 KB, patch)
2010-06-30 23:15 UTC, Matt N
none Details | Review
Add background image to notifications (7.30 KB, patch)
2010-07-14 21:23 UTC, Matt N
rejected Details | Review
Sticks background image to the notification for rhythmbox (3.14 KB, patch)
2011-06-11 05:48 UTC, Neha
reviewed Details | Review
Sticks background image to the notification for rhythmbox (3.66 KB, patch)
2011-06-11 08:22 UTC, Neha
needs-work Details | Review
Sticks background image for rhythmbox notifications (4.99 KB, patch)
2011-07-11 18:02 UTC, Neha
needs-work Details | Review
Place the background image behind the notification content (5.20 KB, patch)
2011-07-12 04:51 UTC, Marina Zhurakhinskaya
none Details | Review
Rhythmbox notification - example 1 (89.52 KB, image/png)
2011-07-12 05:01 UTC, Marina Zhurakhinskaya
  Details
Rhythmbox notification - example 2, the text goes all the way over the image (98.83 KB, image/png)
2011-07-12 05:04 UTC, Marina Zhurakhinskaya
  Details
Rhythmbox notification in the banner mode - should the top of the image be visible? (21.46 KB, image/png)
2011-07-12 05:05 UTC, Marina Zhurakhinskaya
  Details
Rhythmbox notification - example 3, with regular action buttons (81.91 KB, image/png)
2011-07-12 05:26 UTC, Marina Zhurakhinskaya
  Details
Place the background image behind the notification content (5.64 KB, patch)
2011-07-12 05:30 UTC, Marina Zhurakhinskaya
none Details | Review
Implement background images for notifications (8.17 KB, patch)
2011-07-21 18:56 UTC, Neha
reviewed Details | Review
Implement background images for notifications (7.04 KB, patch)
2011-07-21 20:38 UTC, Neha
needs-work Details | Review
messageTray: implement showing images in notifications (7.03 KB, patch)
2011-08-28 20:44 UTC, Marina Zhurakhinskaya
none Details | Review
Fix the layout of a notification containing an image (3.32 KB, patch)
2011-08-28 21:12 UTC, Marina Zhurakhinskaya
none Details | Review
Fix the layout of a notification containing an image (3.83 KB, patch)
2011-08-28 21:17 UTC, Marina Zhurakhinskaya
none Details | Review
Moves IMAGE_SIZE to the Notification class to be consistent with how we define ICON_SIZE for Source. (2.29 KB, patch)
2011-08-28 21:30 UTC, Marina Zhurakhinskaya
none Details | Review
messageTray: implement showing images in notification (8.33 KB, patch)
2011-08-29 02:04 UTC, Neha
none Details | Review
Make sure we only add cells to the table if they will have content (6.19 KB, patch)
2011-08-29 04:36 UTC, Marina Zhurakhinskaya
none Details | Review
Fix the layout of the notification containing an image (5.21 KB, patch)
2011-08-29 15:36 UTC, Marina Zhurakhinskaya
none Details | Review
messageTray: Implement showing images in notifications (9.94 KB, patch)
2011-08-29 17:01 UTC, Neha
committed Details | Review

Description Matt N 2010-06-08 19:33:47 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
Comment 1 Matt N 2010-06-08 21:31:34 UTC
Created attachment 163114 [details] [review]
Fix typo and clean out a comment

Already noticed a bug, this one seems to be working
Comment 2 Florian Müllner 2010-06-08 22:23:54 UTC
(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.
Comment 3 Matt N 2010-06-11 20:55:47 UTC
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.
Comment 4 Matt N 2010-06-30 23:15:59 UTC
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.
Comment 5 Dan Winship 2010-07-01 15:14:38 UTC
(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.
Comment 6 Matt N 2010-07-14 21:23:27 UTC
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.
Comment 7 Matt N 2010-07-14 21:54:07 UTC
(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.
Comment 8 Dan Winship 2010-07-16 17:58:45 UTC
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?
Comment 9 Matt N 2010-07-27 14:24:17 UTC
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.
Comment 10 Dan Winship 2010-07-27 14:52:43 UTC
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.
Comment 11 Neha 2011-06-11 05:48:51 UTC
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.
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-06-11 06:51:20 UTC
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.
Comment 13 Jonathan Matthew 2011-06-11 07:00:34 UTC
(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.
Comment 14 Neha 2011-06-11 08:22:03 UTC
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.
Comment 15 Florian Müllner 2011-06-11 08:44:51 UTC
(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)
Comment 16 Jasper St. Pierre (not reading bugmail) 2011-06-28 19:15:18 UTC
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 17 Jasper St. Pierre (not reading bugmail) 2011-06-28 19:15:23 UTC
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 18 Jasper St. Pierre (not reading bugmail) 2011-06-28 19:16:18 UTC
Comment on attachment 165920 [details] [review]
Add background image to notifications

Marking rejected to get off patch review list.
Comment 19 Neha 2011-07-11 18:02:48 UTC
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().
Comment 20 Jasper St. Pierre (not reading bugmail) 2011-07-11 18:32:13 UTC
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.
Comment 21 Jasper St. Pierre (not reading bugmail) 2011-07-11 18:33:44 UTC
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).
Comment 22 Marina Zhurakhinskaya 2011-07-12 04:49:42 UTC
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.
Comment 23 Marina Zhurakhinskaya 2011-07-12 04:51:16 UTC
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.
Comment 24 Marina Zhurakhinskaya 2011-07-12 05:01:52 UTC
Created attachment 191765 [details]
Rhythmbox notification - example 1
Comment 25 Marina Zhurakhinskaya 2011-07-12 05:04:44 UTC
Created attachment 191766 [details]
Rhythmbox notification - example 2, the text goes all the way over the image
Comment 26 Marina Zhurakhinskaya 2011-07-12 05:05:43 UTC
Created attachment 191767 [details]
Rhythmbox notification in the banner mode - should the top of the image be visible?
Comment 27 Marina Zhurakhinskaya 2011-07-12 05:26:54 UTC
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.
Comment 28 Marina Zhurakhinskaya 2011-07-12 05:30:28 UTC
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.
Comment 29 Marina Zhurakhinskaya 2011-07-12 05:34:13 UTC
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.
Comment 30 Jakub Steiner 2011-07-12 10:23:31 UTC
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
Comment 31 Florian Müllner 2011-07-12 13:59:45 UTC
(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/
Comment 32 Marina Zhurakhinskaya 2011-07-12 15:18:56 UTC
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
Comment 34 Neha 2011-07-21 18:56:24 UTC
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.
Comment 35 Neha 2011-07-21 19:05:35 UTC
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
Comment 36 Jasper St. Pierre (not reading bugmail) 2011-07-21 19:06:48 UTC
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.
Comment 37 Neha 2011-07-21 20:38:31 UTC
Created attachment 192420 [details] [review]
Implement background images for notifications

Background images are part of notifications spec. Add support for them
in the Shell.
Comment 38 Neha 2011-07-21 20:40:31 UTC
Review of attachment 192420 [details] [review]:

The image is stretched and layout needs some more tweaking
Comment 39 Dan Winship 2011-08-22 14:54:36 UTC
Reassigning; Marina is on vacation and Jasper reviewed the last version of the patch anyway.
Comment 40 Jasper St. Pierre (not reading bugmail) 2011-08-22 15:07:57 UTC
(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.
Comment 41 Neha 2011-08-22 23:24:11 UTC
(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?
Comment 42 Marina Zhurakhinskaya 2011-08-28 20:44:33 UTC
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.
Comment 43 Marina Zhurakhinskaya 2011-08-28 21:12:04 UTC
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.
Comment 44 Marina Zhurakhinskaya 2011-08-28 21:17:46 UTC
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.
Comment 45 Marina Zhurakhinskaya 2011-08-28 21:30:00 UTC
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.
Comment 46 Neha 2011-08-29 02:04:01 UTC
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.
Comment 47 Marina Zhurakhinskaya 2011-08-29 04:36:18 UTC
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.
Comment 48 Marina Zhurakhinskaya 2011-08-29 15:36:35 UTC
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.
Comment 49 Neha 2011-08-29 17:01:18 UTC
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 50 Jasper St. Pierre (not reading bugmail) 2011-08-29 17:42:48 UTC
Comment on attachment 195093 [details] [review]
messageTray: Implement showing images in notifications

Looks good
Comment 51 Marina Zhurakhinskaya 2011-08-29 17:55:34 UTC
Review of attachment 195093 [details] [review]:

Pushed it.
Comment 52 Marina Zhurakhinskaya 2011-08-29 17:55:48 UTC
Review of attachment 195093 [details] [review]:

Pushed it.