GNOME Bugzilla – Bug 624900
make the summary notification animation nicer
Last modified: 2010-12-17 15:50:45 UTC
We'd like the summary notification to fade in slightly above the message tray when the source is clicked. Similar to what the user menu does. Ideally, it would have an arrow too, but it might be a bit tricky since the source can move around when the user hovers over other sources or moves outside of the summary area. Just replacing the black background that extends to the bottom would be a good start. In the future, we'd like these items to have a "genie" effect that the popup shown over the Download item on this page has: http://www.panic.com/coda So, for the message tray summary, the notification would be moving a bit up as it fades in and a bit further up as it fades out. For the user menu, the menu would be moving a bit down as it fades in, and a bit further down as it fades out.
Created attachment 170156 [details] [review] [boxpointer] add animateDisappear/animateAppear methods
Created attachment 170157 [details] [review] [MessageTray] use boxpointer for summary notification
Created attachment 170524 [details] [review] add animateDisappear/animateAppear methods Maxim's patch rebased to latest on master.
Review of attachment 170524 [details] [review]: Looks great! Only two very minor comments code-wise. Jon, do you want to try this out for the panel menus? I like the animation when we are showing or hiding a single menu, but I think it looks a bit jittery when we are changing which menu is showing when the user moves the mouse from one item in the panel to another. Any ideas on how it should animate in that case? ::: js/ui/boxpointer.js @@ +68,3 @@ + } + + this.actor.opacity = 0; It would be a bit more logical to set opacity to 0 before calling this.actor.show() . @@ +80,3 @@ + let y = this.actor.y; + let ox = this.actor.x; + let oy = this.actor.y; originalX and originalY would be better names
Review of attachment 170157 [details] [review]: This doesn't quite work for me, as the summary notification constantly flickers. _allocate() in BoxPointer seems to be continually called when the summary notification is showing. ::: data/theme/gnome-shell.css @@ +847,3 @@ } +.summary-notification-bin { I suggest renaming this to .summary-notification-boxpointer . That way the name will be similar to .popup-menu-boxpointer . ::: js/ui/messageTray.js @@ +865,3 @@ this._summaryBin.opacity = 0; + this._summaryNotificationBin = new BoxPointer.BoxPointer(St.Side.BOTTOM, this._summaryNotificationBoxPointer is a better name. It is strange to be calling .actor and .bin on something called a Bin. @@ +1479,3 @@ }, + _adjustNotificationBinPosition: function(actor) { I'm not sure why this function is needed. Is it for adjusting the position of the item's arrow when the summary item it's for is collapsed or moved around? Is this something that can be implemented with a more generic function in BoxPointer? I think that would be preferable. If that's not possible, it at least requires a comment about the behavior it is implementing. @@ +1486,3 @@ + let arrowOrigin = arrowBase / 2; + if (x + this._summaryNotificationBin.actor.width > global.screen_width) { + let dx = x + this._summaryNotificationBin.actor.width - global.screen_width; deltaX @@ +1497,2 @@ _onSummaryNotificationExpanded: function() { + this._itemActor = this._clickedSummaryItem.actor; If we absolutely need to store the summaryItem for which the notification is being shown, you should declare that variable in _init() and call it more descriptively, e.g. this._showingNotificationSummaryItem However, this variable seems unnecessary. We could instead add _unsetClickedSummaryItem() function which would disconnect this._itemAllocationChangedId and set this._clickedSummaryItem to null. We would then call this function from everywhere where this._clickedSummaryItem is being set to null or changed. We are always hiding the notification for this item when that happens, so it's not necessary to make adjustments based on the item's position during that time. @@ +1498,3 @@ + this._itemActor = this._clickedSummaryItem.actor; + this._itemAllocationChangedId = this._itemActor.connect('allocation-changed', + Lang.bind(this, this._adjustNotificationBinPosition)); this._summaryItemAllocationChangedId; should also be first declared in _init() if we in fact need to track this here as opposed to in BoxPointer.
Created attachment 171222 [details] [review] [MessageTray] use boxpointer for summary notification > I'm not sure why this function is needed. Is it for adjusting the position of > the item's arrow when the summary item it's for is collapsed or moved around? yes, it is. > Is this something that can be implemented with a more generic function in BoxPointer? It is much harder to do.
Created attachment 172111 [details] [review] [MessageTray] use boxpointer for summary notification fix flickering with very long message body.
Review of attachment 172111 [details] [review]: This works well (except for the case with chat messages described below). The way the arrow moves accordingly when the item gets expanded and collapsed is really nice :). The suggested changes below are all straight-forward to implement. ::: js/ui/messageTray.js @@ +1494,3 @@ }, + _adjustNotificationBoxPointerPosition: function() { I think adjustBoxPointerPosition(sourcePosition) belongs in boxpointer.js . You can get the notification's prefferred width and height there with this.actor.get_preferred_width/height() It's ok if you add a comment that this is only first implemented for St.Side.BOTTOM case and implement other cases later. @@ +1495,3 @@ + _adjustNotificationBoxPointerPosition: function() { + // position of arrowhead should be equal position of _clickedSummaryItem.actor Here is the comment wording suggestion: // The position of the arrow origin should be the same as the position of this._clickedSummaryItem.actor However, I think it would actually look much better if the arrow points to the center of the icon, not its left-most edge. @@ +1499,3 @@ + return; + let [hasArrowBase, arrowBase] = this._summaryNotificationBoxPointer.actor.get_theme_node().get_length('-arrow-base', false); + arrowBase = hasArrowBase ? arrowBase : 0; 0 doesn't seem to be a good default. This should use the same variable names and pattern as _drawBorder() in boxpointer.js , which is [found, base] = themeNode.get_length('-arrow-base', false); and then 'base' being used without a fallback. @@ +1502,3 @@ + let [itemX, itemY] = this._clickedSummaryItem.actor.get_transformed_position(); + let x = itemX - arrowBase / 2; + let arrowOrigin = arrowBase / 2; Should use Math.floor() If you define arrowOrigin first, you can then use it when assigning the value to x :). @@ +1528,3 @@ _onSummaryNotificationExpanded: function() { + if (this._summaryNotificationState == State.SHOWN) + return; _onSummaryNotificationExpanded() actually only gets called when the summary notification is shown. It is used when the summary notification is updated, such is when there are new chat messages. So right now the position of the notification doesn't get adjusted when there is a new message, but removing this check fixes the problem. However, then notification disappears and appears again when a new message is added because animateAppear() is called. What actually makes most sense, is to move all this code to the end of _showSummaryNotification() . The first time the code here gets called right now is when 'expand' is emitted in this._summaryNotification.expand(false) call anyway, which is the last call in _showSummaryNotification() . Then you can just have the code that tweens this._summaryNotificationBoxPointer.actor.y to the new value here. @@ +1538,3 @@ + this._adjustNotificationBoxPointerPosition(); + this._summaryNotificationBoxPointer.animateAppear(); + this._summaryNotificationState = State.SHOWN; State.SHOWN should only be set when animateAppear() is complete. So animateAppear() should take an onComplete callback similar to how it's done in animateDisappear() . State.SHOWING should be set here (probably before the call to animateAppear() ). @@ +1552,2 @@ + this._summaryNotification.ungrabFocus(); + this._summaryNotificationState = State.HIDDEN; State.HIDDEN should be set in this._hideSummaryNotificationCompleted() ; State.HIDING should be set here.
Created attachment 172404 [details] [review] add animateDisappear/animateAppear methods
Created attachment 172405 [details] [review] boxpointer: add setPosition method
Created attachment 172406 [details] [review] [MessageTray] use boxpointer for summary notification
Review of attachment 172404 [details] [review]: Looks good!
Review of attachment 172405 [details] [review]: Good to commit once you address the comment below. ::: js/ui/boxpointer.js @@ +348,3 @@ + // Actually set the position + this.actor.x = x; + this.actor.y = y; transform_stage_point() doesn't seem to produce integers. Should we use Math.floor() on the coordinates?
Review of attachment 172406 [details] [review]: The code structure looks great! I realized there are some problems with style (I missed them on the initial review). I pointed out some in the comment below. The other problem is that the notification is no longer transparent and the focused chat glow doesn't show up. Is this something related to how the boxpointer is drawn? Can you try to fix it? ::: data/theme/gnome-shell.css @@ +876,2 @@ /* message-tray.height + notification.padding-bottom */ padding-bottom: 44px; The 'padding-bottom: 44px;' is no longer needed - this was used to make the notification content start above the message tray when it was extending all the way behind it. 'padding-bottom: 12px' looks good for both one-line and multi-line notifications, such as the ones that have actions. Actually, ideally we should use 8px for one-line notifications and 12px or 16px for multi-line ones, but 'multi-line-notification' class name doesn't seem to have any effect at the moment. We should also add 'border-radius: 9px;' to match the -arrow-border-radius in .summary-notification-boxpointer . Otherwise we end up with non-matching or non-rounded corners that are defined in #notification. Btw, -arrow-border-radius, -arrow-background-color, -arrow-border-width, -arrow-border-color are really confusing names. I don't think they should have 'arrow' in them. The only ones that need to have 'arrow' is -arrow-base and -arrow-rise. ::: js/ui/messageTray.js @@ +1527,2 @@ _onSummaryNotificationExpanded: function() { + this._adjustNotificationBoxPointerPosition(); (This comment doesn't require any change.) I was wondering if we could use the tween below, which is what we had before. However, it makes things look worth because the bottom of the notification first moves down before the whole notification moves up. (We had this problem before too.) So just readjusting the 'y' without tweening makes most sense. I think it's reasonable to call this._adjustNotificationBoxPointerPosition(), but otherwise we could just set it in place this._summaryNotificationBoxPointer.actor.y = this._clickedSummaryItem.actor.y - this._summaryNotificationBoxPointer.actor.height; The tween that I tried: this._tween(this._summaryNotificationBoxPointer.actor, '_summaryNotificationState', State.SHOWN, { y: this._clickedSummaryItem.actor.y - this._summaryNotificationBoxPointer.actor.height, time: ANIMATION_TIME, transition: 'easeOutQuad' });
Created attachment 172589 [details] [review] [MessageTray] use boxpointer for summary notification > problem is that the notification is no longer transparent fixed > the focused chat glow doesn't show up. It work as expected With small change in boxpointer. @@ -50,7 +50,7 @@ BoxPointer.prototype = { if (!found) rise = 0; - this.actor.opacity = 0; + this.actor.opacity = 255; this.actor.show(); switch (this._arrowSide) { @@ -68,7 +68,7 @@ BoxPointer.prototype = { break; } - Tweener.addTween(this.actor, { opacity: 255, + Tweener.addTween(this.actor, { //opacity: 255, x: x, y: y, transition: "linear", I am not sure, is it Clutter bug or StThemeNodeDrawing bug.
Review of attachment 172589 [details] [review]: The border-radius change still needs a small adjustment as described in the comment below. I think this patch is good to commit with the small changes mentioned below. We'll ask Florian to look into the glow problem, since Dan and Owen believe that shadows are his territory :). I'll add more details in a separate comment and once these patches land we can move it out to a separate bug. ::: data/theme/gnome-shell.css @@ +853,2 @@ font-size: 16px; border-radius: 5px 5px 0px 0px; There is a duplicate assignment of border-radius here. We want to keep the border-radius for the two bottom corners at 0px because we don't want them to be rounded. So it's best to just add 'border-radius: 9px;' to .summary-nitification-boxpointer #notification @@ +878,2 @@ /* message-tray.height + notification.padding-bottom */ + padding-bottom: 12px; The comment is no longer applicable, so you should just remove it. ::: js/ui/messageTray.js @@ +1486,3 @@ this._summaryNotification.grabFocus(true); + this._summaryNotificationBoxPointer.actor.opacity = 0; Can you move this line just below right before this._summaryNotificationBoxPointer.actor.show() so that it's clear why we are setting opacity to 0 here.
(In reply to comment #15) > > the focused chat glow doesn't show up. > It work as expected With small change in boxpointer. > @@ -50,7 +50,7 @@ BoxPointer.prototype = { > if (!found) > rise = 0; > > - this.actor.opacity = 0; > + this.actor.opacity = 255; > this.actor.show(); > > switch (this._arrowSide) { > @@ -68,7 +68,7 @@ BoxPointer.prototype = { > break; > } > > - Tweener.addTween(this.actor, { opacity: 255, > + Tweener.addTween(this.actor, { //opacity: 255, > x: x, > y: y, > transition: "linear", > I am not sure, is it Clutter bug or StThemeNodeDrawing bug. Florian, could you please look into this problem? Basically, you can reproduce it by applying the three patches from this bug and then getting a chat message, and looking at the notification in the summary view. The text entry box will be missing the glow, but if you remove the tweening for the opacity value and just set it to 255 from the start, the glow will be there. The glow will also be there if you move this._summaryNotification.grabFocus(true); call into animateAppear() onComplete callback. Here is the css for the focused chat entry box: .chat-response:focus { border: 1px solid #3a3a3a; color: #545454; background-color: #e8e8e8; caret-color: #545454; -st-shadow: 0px 0px 6px 2px rgba(255,255,255,0.9); } The actor being tweened is a St.Bin that contains Shell.GenericContainer that contains St.Bin that contains the notification that eventually contains the chat entry box.
Created attachment 175046 [details] [review] shadows: Copy material when updating parameters for painting According to the clutter cookbook, materials can not be assumed to be modifiable more than once after creation, so operate on a copy rather than the transition's material. The glow is still occasionally missing with that patch, but it sure helps a lot.
Review of attachment 175046 [details] [review]: Hmm: void cogl_set_source_color (const CoglColor *color) { CoglPipeline *pipeline; _COGL_GET_CONTEXT (ctx, NO_RETVAL); if (cogl_color_get_alpha_byte (color) == 0xff) { cogl_pipeline_set_color (ctx->opaque_color_pipeline, color); pipeline = ctx->opaque_color_pipeline; } else { CoglColor premultiplied = *color; cogl_color_premultiply (&premultiplied); cogl_pipeline_set_color (ctx->blended_color_pipeline, &premultiplied); pipeline = ctx->blended_color_pipeline; } cogl_set_source (pipeline); } (pipeline is the new name for material in Clutter 1.5)
Comment on attachment 175046 [details] [review] shadows: Copy material when updating parameters for painting Clutter commit 3729e47ef appears to have fixed the issue.