GNOME Bugzilla – Bug 659158
Abrt: WHOA BABY
Last modified: 2012-03-15 21:01:57 UTC
Created attachment 196647 [details] screenshot Yikes
I believe Marina is working with the abrt guys to get this fixed.
Oh, I posted a patch for abrt which should fix this yesterday. Maybe there's something to fix on the shell side too though. https://fedorahosted.org/abrt/ticket/335
Created attachment 198585 [details] [review] tests: Add a test for scaling multitextured materials https://bugzilla.gnome.org/show_bug.cgi?id=659166
Created attachment 198586 [details] [review] notificationDaemon: Use the natural size for embedded images Instead of hardcoding an image size of 125x125, use the natural image size. whoops, uploaded the wrong patch by mistake. If we want to downscale album art for Rhythmbox, we have a few options: * Set a hardcoded maximum size of 125x125 * Update the notification spec to add new hints to tell us that we should resize the image client-side. * Load the album art in Rhythmbox, scale there, and shuttle the raw pixbuf over. Your choice.
Created attachment 198639 [details] [review] notificationDaemon: Use the natural size for embedded images Instead of hardcoding an image size of 125x125, use the natural image size. -- Fixed the "padding" around images.
Created attachment 198640 [details] [review] messageTray: Add banner body when expanding if we haven't already In some cases, we never add the banner body when expanding. Ensure that we add it when expanding to make sure the user never misses part of a message. -- This fixes the banner body not appearing in the "Download Complete" notification. I remember seeing a bug for it, but couldn't find it. Attaching it here.
Created attachment 198688 [details] [review] StTable: Port allocation changes from MxTable to St MxTable has changed to have much better support for spanning elements since we forked it for St. Port these changes over. -- This removes a lot of the hacks, helps an St/Mx merge, and makes row-span/col-span work correctly. Note that this is after the changes I submitted to Mx here: https://github.com/clutter-project/mx/pull/9
Created attachment 198689 [details] [review] messageTray: Use the natural size for images
Created attachment 198690 [details] [review] messageTray: Add banner body when expanding if we haven't already In some cases, we never add the banner body when expanding. Ensure that we add it when expanding to make sure the user never misses part of a message. Removed an accidentally added '_delegate' line that I used for debugging. It should probably go in another patch.
Review of attachment 198689 [details] [review]: Accidentally uploaded this one. Will upload a new one in a bit.
Created attachment 198692 [details] [review] StTable: Port allocation changes from MxTable to St MxTable has changed to have much better support for spanning elements since we forked it for St. Port these changes over. -- Fixed a build issue
Created attachment 198693 [details] [review] messageTray: Use the natural size for images OK. Here's the "patch". It doesn't get the aspect ratio right. This is a hard problem: * We have a table with a fixed width, 35em. We can't show any image at full scale -- the table would "overflow" then. * ClutterTexture (the image) always allocates a minimum size of 0x0, meaning that the table will try to compress it the rowspan if it won't fit. Using a Shell.GenericContainer wrapper to set the natural and min width sizes will set them both to the raw size, leading to the overflow above. * ClutterTexture will try to return its full texture size, so the table allocates that much for the width of the texture cell, but gets "beaten" by collapsing the row-span here. We have a few different layouts: ________________________________________ / \ | Q Title banner.... | ------------------------------------------ ________________________________________ / \ | Q Title | | | | banner body banner body | ------------------------------------------ ________________________________________ / \ | Q Title | | | | /=-=\ This is the banner body | | |IMG| | | |IMG| | | \=-=/ | ------------------------------------------ ________________________________________ / \ | Q Title | | | | /=-=\ This is the banner body | | |IMG| | | |IMG| | | \=-=/ << || >> | ------------------------------------------ The easiest thing to do in the short term is to stick the "body" (image, banner body, action area) into two BoxLayouts instead of using a table for this. The banner body and action area are all created at separate times, so a naïve replacement won't work.
For reference, I tested this with something like: $ sleep 5 & kill -SIGSEGV %1
For those unable to parse my message in Comment 12, I'm saying that the Notification code has too many moving parts: we add and remove and show and hide and create and destroy too many things. I'm suggesting that we try to reduce the amount of mechanical failures by looking at all the states and parts from the top down, making each state explicit in the code, and removing a lot of the silly interactions that happen between states. See also: bug #661615 , bug #661236 , bug #661077 .
Created attachment 199171 [details] Wrong-looking abrt notification with this patches This is not the right way for the abrt notification to look either. These patches don't fix the main problem, which is that a lot of applications (including abrt and firefox) use notify_notification_set_icon_from_pixbuf() which in turn calls notify_notification_set_image_from_pixbuf() which in turn sets image-data for notification daemons claiming to implement the 1.2 spec (like us) in libnotify/libnotify/notification.c . This is how we end up with a default icon and an image which should really be an icon. There are two alternative ways to proceed. 1) Use image-data for an icon in _iconForNotificationData() if icon is not specified, and don't use it for an image in this case. This would make the gnome-shell notification daemon backward-compatible with all the notify_notification_set_icon_from_pixbuf() calls that are still around. Then, applications will be encouraged to move to specify app_icon parameter in there calls to Notify() and drop notify_notification_set_icon_from_pixbuf() calls. If they want to have both the icon and image displayed, they would need to specify both. We would also implement using the icon associated with the application for the notification if app_icon was not specified. Google code search shows that currently there are 5 usages of notify_notification_set_image_from_pixbuf(), none of which are in core GNOME apps, and 100+ usages of the "deprecated" notify_notification_set_icon_from_pixbuf(). So by potentially using image-data as an icon if an icon is not set, we would not be breaking anything. This way assumes that we do not want to support specifying icons with pixbufs in the long term, but only want to support specifying them with URIs or names of icons that are part of the theme. Which is what the current spec provides for. 2) Reinstate notify_notification_set_icon_from_pixbuf() and make it set icon_data in libnotify. Then handle icon_data for setting icons in the notification daemon. This way assumes that we want to keep supporting specifying icons with pixbufs. It seems to be more pragmatic to me, as it seems like there are many applications now that use that way of setting an icon. I'll implement one of the ways tomorrow. Input from Jon on whether supporting icon_data in the spec makes sense would help inform which way to implement it.
(In reply to comment #15) > Created an attachment (id=199171) [details] > Wrong-looking abrt notification with this patches Oh, whoops, I forgot to amend a patch. Setting x_fill = false; on the cell the icon is in should fix this.
Created attachment 199247 [details] [review] notificationDaemon: use "image-data" hint for the icon and only display an image for Rhythmbox notifications Historically, when applications set "image-data" they expect it to show up as an icon. For now, we only know of Rhythmbox as a use-case for a large image in a notification, so make that explicit.
Created attachment 199249 [details] Properly looking abrt notification screenshot
Created attachment 199250 [details] Properly looking download complete notification screenshot
Created attachment 199251 [details] Properly looking Rhythmbox track notification screenshot
Review of attachment 199247 [details] [review]: This is an ugly, dirty hack. If this goes in, we need to fix it properly for 3.4.
Created attachment 199253 [details] [review] notificationDaemon: use "image-data" hint for the icon and only display an image for Rhythmbox notifications Historically, when applications set "image-data" they expect it to show up as an icon. For now, we only know of Rhythmbox as a use-case for a large image in a notification, so make that explicit. Just a small update that adds a comment in the code.
Created attachment 199262 [details] [review] notificationDaemon: only display a large image if icon is also specified Historically, when applications set "image-data" they expect it to show up as an icon. So we display it as such if an icon is not specified with an "app_icon" argument to Notify(). We only display a large image specified with "image-data" or "image-path" if an icon is also specified. This is an alternative fix to hardcoding Rhythmbox. The abrt, Firefox download complete, and Rhythmbox notifications look the same. There are no known applications that specify both "app_icon" argument and ("image-data" or "image-path"), but don't want an additional image to be displayed. This approach is better, as it will allow other applications such as Banshee to also display large images in notifications. https://bugzilla.gnome.org/show_bug.cgi?id=659768
Review of attachment 199262 [details] [review]: Remove the duplicate URLs in the commit message. ::: js/ui/notificationDaemon.js @@ +120,3 @@ icon_type: St.IconType.FULLCOLOR, icon_size: size }); + } else if (hints['image-data']) { What about image-path with no icon? We just ignore it?
Created attachment 199277 [details] [review] notificationDaemon: only display a large image if icon is also specified You are right. We should use "image-path" for an icon if an "app_icon" is not passed in, since we would not be using it for a large image anyway. I didn't have it before because I'm not sure if that ever happens and because we didn't use it at all before the patch to support large images.
Review of attachment 199277 [details] [review]: ::: js/ui/notificationDaemon.js @@ +111,3 @@ + // If an icon is not specified, we use 'image-data' or 'image-path' hint for an icon + // and don't show a large image. A comment in here explaining why would be nice: // libnotify sets the image hint when using 'set_icon_from_pixbuf' // A lot of API users assumed this would do the right thing, meaning // to set an icon when they should have used the 'app_icon' parameter. // In this case, it's unlikely that an application would have both // 'app_icon' parameter and the 'image-data' hint, so correct their // mistake. In the case where we have both, show a large image instead.
Comment on attachment 199277 [details] [review] notificationDaemon: only display a large image if icon is also specified Added the suggested comment.
Review of attachment 198690 [details] [review]: Good catch! The right thing to do though, at least to be consistent with the current code, is to put this._addBannerBody() in setImage(). For example, after this._table.add_style_class_name('notification-with-image') line. That's most consistent because the other places where we call this._addBannerBody() are when we find out that the notification will have some content, such as in _createScrollArea() and in setActionArea(), as well as when we find out that the banner doesn't fit in the first line. I don't think it requires any comment in the code when placed there. I've tested it that way and it works fine. A good commit message for this patch would be: messageTray: add banner body when setting an image for the notification We always place the banner in the body if the notification has additional content.
Comment on attachment 198690 [details] [review] messageTray: Add banner body when expanding if we haven't already An updated version was committed.
The reported problem is fixed, but let's keep this bug open since it still has some patches attached that need a review.
Review of attachment 198692 [details] [review]: The changes to st-table code don't work for regular notifications. Try libnotify/tests/test-basic and you'll see that things don't get wrapped correctly and get too much height. Inline, are some comments about the code itself as is. In theory, you should also be able to remove "notification-with-image" css class once everything else works. Perhaps it makes sense to move out this patch to a different bug and close this one. ::: js/ui/messageTray.js @@ -464,3 @@ this._table.add(this._bannerBox, { row: 0, col: 1, - col_span: 2, col_span: 2 is still needed here. ::: src/st/st-table.c @@ +63,3 @@ +{ + guint expand : 1; + guint shrink : 1; Looks like it can be removed (you removed it in MX too). @@ +339,3 @@ + col->min_size = MAX (col->min_size, w_min); + col->final_size = col->pref_size = MAX (col->pref_size, w_pref); + col->expand = MAX (col->expand, meta->x_expand); col->expand || meta->x_expand be easier on the reader, but ok to keep same as in MX. @@ +548,3 @@ + { + columns[i].final_size = + columns[i].pref_size + (extra_width / priv->n_cols); This will never be reached, because if columns[i].expand is true n_expand will be greater than 0. Should this be the following, similar to step (3) above? if (columns[i].expand) columns[i].final_size = columns[i].pref_size + (extra_width / n_expand); else if (!n_expand) columns[i].final_size = columns[i].pref_size + (extra_width / priv->n_cols); else columns[i].final_size = columns[i].pref_size; @@ +561,3 @@ + columns[i].final_size++; + i++; + remaining--; This should be if (columns[i].expand || !n_expand) { columns[i].final_size++; remaining--; } i++; @@ +595,3 @@ ClutterActor *child; + DimensionData *row; + gfloat c_min, c_pref; Perhaps h_min, h_pref to be consistent with the names in st_table_calculate_col_widths() . @@ +632,3 @@ + StTableChild *meta; + ClutterActor *child; + gfloat c_min, c_pref; h_min, h_pref @@ +669,3 @@ + } + + /* If this actor is visible, then all the rows is spans are visible */ s/is spans/it spans ; Maybe make consistent with the corresponding comment in st_table_calculate_col_widths() . @@ +682,3 @@ + /* 1) If the minimum height of the rows spanned is less than the minimum + * height of the child that is spanning them, then we must increase the + * minimum height of the rows spanned. The way these comments appear now, they duplicate shorter comments for (2) and (3) that are already inlined. It'd be best to drop the enumeration and inline the longer comments, and then just say "See st_table_calculate_row_heights() for more comments" in st_table_calculate_col_widths() or duplicate the comments. @@ +842,3 @@ + { + rows[i].final_size = + rows[i].pref_size + (extra_height / priv->n_rows); See corresponding comment in st_table_calculate_col_widths() . @@ +855,3 @@ + rows[i].final_size++; + i++; + remaining--; See corresponding comment in st_table_calculate_col_widths() .
Created attachment 203413 [details] [review] st-table: Port allocation changes from MxTable to St MxTable has changed to have much better support for spanning elements since we forked it for St. Port these changes over. I didn't change the c_pref to h_pref, because I didn't understand what you meant. It's 'c_pref' in both cases. Do you want me to change it to 'r_pref' in the case of rows?
Created attachment 203414 [details] [review] messageTray: Set the bannerBox to expand Because we have better support for spanning and expanding elements now, we can remove the StBin to force bannerBox to expand, and just expand the banner box naturally. Yes, now that we have better support for spanning and expanding elements, we can remove the expanding bin and col_span and just rely on St.Table to do the right thing. I've tested this a bit more now, and it works.
Created attachment 203415 [details] [review] messageTray: Make the scroll area and action area expand, too Without this, we end up distributing the excess remaining area among all children, which makes the icon on the left stretch.
These changes didn't work out as well as I hoped they would. I may bring them back some day, but not in this bug.