GNOME Bugzilla – Bug 624584
Message tray look update
Last modified: 2010-11-25 04:56:03 UTC
The attached patch contains some design improvements that Jon suggested for the message tray. I'll file more of the changes we discussed later.
Created attachment 166045 [details] [review] Update the look of various message tray components The following changes make message tray look more polished: - all notifications are now same width (no min and max width) - there is an equal padding all around the notification (which resulted in the increased tray height) - action buttons are now nicely styled - chat text box glows when focused (nevermind the unfocused look because we'll soon always have it focused when it is showing) - chat text has padding on the left and rounded background - summary source buttons no longer have a border This applies to the current master branch (i.e. no need for the changes from the message tray design update bug).
*** Bug 624371 has been marked as a duplicate of this bug. ***
Overall a huge improvement. Details matter so thanks for the effort here. A few things I found in brief testing. * Not able to scroll while the entry box is focused in chats * Similar to the above something funky seemed to happen when I tried to use printscreen while in the entry box. Seemed like a grab got stuck there or something. * Descenders are clipped in the entry box (eg. type gyp) * The summary icons don't extend to the bottom right corner of the screen so if I slam my mouse into the lower right corner I have to left/up up to hover over them. Maybe this is a feature? * scrollbar left padding is still missing * the unfocused style of the entry box makes it look focused. One easy option is to just copy the style of the overview find box when in non-white mode. * The style of icon buttons still stinks we didn't address that one it seems. On that note, I'm really confused by when action buttons are using icons. Some of the tests in libnotify seem to get icons added even though they don't ask for them. And the choic of icons seems really strange - and in some cases is way too mysterious.
Created attachment 166046 [details] screenshot of icon buttons It isn't clear what the rightmost button does at all. The hover states for icon buttons aren't in sync with the text ones (corner radius, color, padding). The icons stretch the text buttons vertically too much I think. I'm not sure mixing icon buttons and text is really great in general. And certainly not in an ABA pattern.
We probably also need to make the icon in the banner and expanded modes look more like a button especially when hovering. I couldn't seem to find any css for that in order to play with it. Noticed that the ends of the scrollbars in chats are not round. Also, let's try the following for the chats: index 020045d..926e81d 100644 --- a/data/theme/gnome-shell.css +++ b/data/theme/gnome-shell.css @@ -874,8 +874,8 @@ StTooltip { .chat-received { background-gradient-direction: horizontal; - background-gradient-start: #606060; - background-gradient-end: #000000; + background-gradient-start: rgba(255, 255, 255, 0.2); + background-gradient-end: rgba(0, 0, 0, 0.0); padding-left: 4px; border-radius: 4px; @@ -883,8 +883,8 @@ StTooltip { .chat-sent { background-gradient-direction: horizontal; - background-gradient-start: #000000; - background-gradient-end: #606060; + background-gradient-start: rgba(0, 0, 0, 0.0); + background-gradient-end: rgba(255, 255, 255, 0.2); padding-left: 4px; border-radius: 4px;
Created attachment 166251 [details] [review] Update the look of various message tray components The following changes make message tray look more polished: - all notifications are now same width (no min and max width) - there is an equal padding all around the notification (which resulted in the increased tray height) - action buttons are now nicely styled - chat text box glows when focused (nevermind the unfocused look because we'll soon always have it focused when it is showing) - chat text has padding on the left and rounded background - summary source buttons no longer have a border This patch adds more padding to the chat text box, gives it a grey background when unfocused, and changes the background of received and sent chat lines. Jon, transitioning from (255, 255, 255, 0.2) to (0, 0, 0, 0) makes it drop all the way to black, which means that the background outline disappears about half-way. This seemed quite disorienting to me. Wouldn't (255, 255, 255, 0) or something like that make more sense? I made it go from (255, 255, 255, 0.12) to (255, 255, 255, 0.02) in the current patch, but let me know what you like. Dan or Florian, could you please give this patch a quick look-over if you think it needs a code review. Thanks :)!
Created attachment 166252 [details] [review] Only use icon buttons for notification actions for Rhythmbox We currently show a random icon if the action id happens to match the name of some Gtk icon. This is wrong. We'll need to use a hint to indicate that the notification is using 'x-gnome-icon-buttons' capability and only use the icons if it does and if we have images for all the action-ids. This is a quick work-around for now. Jon, this is the patch I had locally to avoid using random icons for action buttons. Dan and Florian, I think this patch is reasonable to apply until we have a proper fix. It's just wrong the way it is now (see Jon's screenshot of how test-multi-actions from libnotify shows up).
(In reply to comment #6) > Dan or Florian, could you please give this patch a quick look-over if you think > it needs a code review. Thanks :)! I don't think it does.
(In reply to comment #7) > Dan and Florian, I think this patch is reasonable to apply until we have a > proper fix. It's just wrong the way it is now (see Jon's screenshot of how > test-multi-actions from libnotify shows up). I don't like it - I don't mind having a workaround for now, but adding a parameter to addButton() suggests a per-action setting, while I would prefer it either per-source or per-notification (the latter makes more sense?). The proper fix will probably just add a new hint to libnotify - in my opinion a property on the source/notification object reflects that better than a parameter to addButton().
(In reply to comment #3) > Overall a huge improvement. Details matter so thanks for the effort here. > > A few things I found in brief testing. > > * Not able to scroll while the entry box is focused in chats I had the same problem with the old code, but it is gone with the new focus grabbing code. > * Similar to the above something funky seemed to happen when I tried to use > printscreen while in the entry box. Seemed like a grab got stuck there or > something. Print Screen works here with the new code. > * Descenders are clipped in the entry box (eg. type gyp) I didn't have this problem, but I increased the padding inside the text box from 2px to 4px in any case. > * The summary icons don't extend to the bottom right corner of the screen so > if I slam my mouse into the lower right corner I have to left/up up to hover > over them. Maybe this is a feature? Possibly a feature. Let's change it later if we decide that it's not :). > * scrollbar left padding is still missing Slightly more involved, so filed a separate bug about it. https://bugzilla.gnome.org/show_bug.cgi?id=624893 > * the unfocused style of the entry box makes it look focused. One easy option > is to just copy the style of the overview find box when in non-white mode. Fixed this. > * The style of icon buttons still stinks we didn't address that one it seems. > On that note, I'm really confused by when action buttons are using icons. Some > of the tests in libnotify seem to get icons added even though they don't ask > for them. And the choic of icons seems really strange - and in some cases is > way too mysterious. Yeah, this is not supposed to be happening. We should not be using icons for buttons for applications that didn't ask for it or if we don't have all the images. Added a patch with a temporary fix.
Created attachment 166255 [details] screenshot of clipping
Created attachment 166258 [details] [review] Only use icon buttons for notification actions for Rhythmbox We currently show a random icon if the action id happens to match the name of some Gtk icon. This is wrong. We'll need to use a hint to indicate that the notification is using 'x-gnome-icon-buttons' capability and only use the icons if it does and if we have images for all the action-ids. This is a quick work-around for now. I think it makes sense to set it on the notification because, in theory, you could have notifications with and without icon buttons from the same source.
Comment on attachment 166258 [details] [review] Only use icon buttons for notification actions for Rhythmbox imho it would be better to just make icon buttons be explicitly opt-in via some other hint
Bug 633510 is about adding an "action-icons" hint to the notifications to use icons for actions.
Created attachment 173547 [details] [review] use action-icons hint to decide whether to create icon buttons
Review of attachment 173547 [details] [review]: Looks good! Would be good to also reflect the fact that the server capability name for supporting action icons was updated in the patch in the commit message. The comments below are very minor. This is good to commit once you fix them up. ::: js/ui/messageTray.js @@ +99,3 @@ this.urgent = false; this.expanded = false; + this.useActionIcons = false; It should be a private variable, which is indicated by a leading underscore. So this._useActionIcons . The reason this.urgent and this.expanded are public is that they are used in the MessageTray class, but this won't be the case for this._useActionIcons . I know my original patch was making it a public variable, but I don't think there is a reason for it. @@ +362,3 @@ + setUseActionIcons: function(use_icons) { + this.useActionIcons = use_icons; The JavaScript convention is to use lowerCamelCase, so the variable name should be useIcons ::: js/ui/notificationDaemon.js @@ +296,2 @@ if (actions.length) { + notification.setUseActionIcons(hints['action-icons'] == true); This comment possibly requires no changes :). We are referring to other hints as hints.hint_name , but because 'action-icons' has a dash, you have to refer to it the way you do, hints['action-icons'] Also, if you wanted to set a default for the value, you could add "'actions-icons': false" in a second argument in this call hints = Params.parse(hints, { urgency: Urgency.NORMAL }, true); in Notify() . Again, because of the "-" you need to use quotes, otherwise it gets interpreted as a minus sign. I don't know if it is meaningful to add a default for this hint.
Thanks for the review. I've pushed an updated patch (commit 9585c82). I decided not to add a default for the action-icons hint, since it already effectively defaults to false. Is there any more work to do on this bug?
Thanks! Nope, nothing else to do for this bug.