GNOME Bugzilla – Bug 623970
long summary should wrap in expanded notifications
Last modified: 2010-08-31 17:57:02 UTC
Testing the message tray with test-basic from libnotify/tests showed that we truncate a long summary even in the expanded view. We should wrap it instead. To get the libnotify tests, check out the stable branch of libnotify from git: git clone git://git.gnome.org/libnotify git checkout -b 0.5 origin/0.5
Created attachment 165695 [details] [review] Changed line wrap and ellipsize mode on popIn and popOut events To get my feet wet in the gnome-shell codebase I tried getting long titles to wrap when expanded. It works for the test-base test in libnotify.
Review of attachment 165695 [details] [review]: Thank you for coming up with this patch! It is correct overall, but there are some caveats that need to be addressed. First, for the commit message, we try to follow the guidelines in this e-mail: http://lists.cairographics.org/archives/cairo/2008-September/015092.html That is we put "what" as a subject line, "why" as the body, and then optionally add "details" in the body. So the commit message for this patch can be Subject: Fully show long titles when a notification is expanded Body: We used to keep long titles ellipsized when a notification was expanded, which was a bug. We now show them fully by line wrapping them. There are also two trailing whitespaces in your patch. See http://live.gnome.org/GnomeShell/Development/WorkingWithPatches#trailing-whitespaces for ideas on how to remove them :). Btw, this page might have some other info you'll find useful. Functional changes: 1) The notification icon should be aligned to the top of the notification, not centered vertically in the first row as it happens now. This can be achieved by adding 'y_align: St.Align.START' property when adding this._icon to this.actor 2) We should still wrap the long title if there is no content (this.actor.row_count == 1). You can add this test locally to libnotify/tests/test-basic.c I think we can fix this by adding this._titleTruncatedInBannerMode variable, setting it to true if (titleBox.x2 + this._spacing > availWidth) in this._bannerBoxAllocate() and checking it in this.popOut() and this.popIn() . Be sure to return the right boolean from the function in all cases and not tween this._bannerLabel if it's not there. 3) We should ellipsize this._titleLabel only once we are done with the animation related to popIn. Otherwise, it disappears abruptly. You can see it well if you set ANIMATION_TIME to 5 instead of 0.2 . (This can also be done on the fly by using St.set_slow_down_factor(factor) in Looking Glass - see http://live.gnome.org/GnomeShell/LookingGlass for more details.) Moving the ellipsization adjustment code to a separate this._unwrapTitle() function and calling it after the ANIMATION_TIME timeout should do the trick. ::: js/ui/messageTray.js @@ +341,3 @@ return false; + // Remove the ellips from the title label and make it wrap so it will ellipses @@ +358,3 @@ return false; + + // Remove line wrap and change ellipsation mode so long titles will get ellipsization
Created attachment 167997 [details] [review] Fully show long titles when a notification is expanded We used to keep long titles ellipsized when a notification was expanded, which was a bug. We now show them fully by line wrapping them. Based on the original patch from Steven Van Bael.
Comment on attachment 167997 [details] [review] Fully show long titles when a notification is expanded > popIn: function() { >- if (this.actor.row_count <= 1) >+ if (this._titleTruncatedInBannerMode) >+ Mainloop.timeout_add(ANIMATION_TIME * 1000, Lang.bind(this, this._unwrapTitle)); you shouldn't really mix addTween and timeout_add; it would be better to call _unwrapTitle from a completion handler on the tween. the patch otherwise looks good, but it seems like you might want to solve the problem we were talking about yesterday at the same time?
Created attachment 168975 [details] [review] Remove Notification::popIn() and calls to it We never revert the notification from the expanded mode to the banner mode. Calling popIn() when hiding the notification only resulted in the notification banner reappearing while the notification was being hidden. This is visible when the ANIMATION_TIME is increased.
Created attachment 168981 [details] [review] Don't show the banner when hiding the notification or showing it in the summary mode The banner should not be appearing briefly when we are hiding the notification. For that, we should only restore the opacity of the banner in popInCompleted() when we are done hiding the notification. We do need to restore the opacity in case the notification is updated and is shown in the banner mode again. The banner should not be appearing briefly when we are showing the notification in the summary mode. For that, we should not use the animation time to fade out the banner in popOut() for summary notifications. These two problems were particularly visible when the ANIMATION_TIME was increased.
Created attachment 168982 [details] [review] Use 'customContent' flag instead of 'bannerBody' We used 'bannerBody' flag to differentiate the case when we move the banner to the body when the notification is expanded from the one when we don't do that and only use the custom content set for the notification, as is the case for Telepathy notifications. We also always cleared the content of the notification on update when bannerBody was set to true. Flag named 'customContent' reflects the use case for it more clearly. The comments that accompany it were also updated and improved. We now always add the banner text as the first element in the expanded notification unless 'customContent' flag is set to true. If the 'body' parameter is specified, we use it in addition to the banner text. The earlier version of the code had a bug that resulted in the 'body' parameter not being set only in the case when the 'bannerBody' was set to true and the banner text had newlines in it.
Created attachment 168984 [details] [review] Fully show long titles when a notification is expanded We used to keep long titles ellipsized when a notification was expanded, which was a bug. We now show them fully by line wrapping them. Based on the original patch from Steven Van Bael.
Created attachment 168986 [details] [review] Fully show long titles when a notification is expanded We used to keep long titles ellipsized when a notification was expanded, which was a bug. We now show them fully by line wrapping them. Based on the original patch from Steven Van Bael.
Comment on attachment 168982 [details] [review] Use 'customContent' flag instead of 'bannerBody' >+// If params includes a 'customContent' parameter with the value %true, >+// then @banner will not be shown in the body except that you didn't explain that part yet. I think you should put the default (banner moves into the body) behavior first, and then after that explain that you can use customContent to turn that behavior off. >- params = Params.parse(params, { bannerBody: this._bannerBody, >+ params = Params.parse(params, { customContent: this._customContent, This was a lame hack to make people not need to re-specify "bannerBody: true" in every call to update. Now that the "normal" behavior is the default, we can just remove the hack, and have it be "customContent: false", and telepathyClient (the only user of customContent:true) can just be sure to re-set it every time it calls update(). >+ this._customContent = params.customContent; This needs to happen after the cleanup (where the _bannerBody assignment happens now), because you want to do the cleanup based on the *old* values of the parameters, not the new values. >- notification.update(summary, body, { icon: iconActor, >- clear: true }); >+ notification.update(summary, body, { icon: iconActor }); I think "clear: true" is still wanted?
Created attachment 169122 [details] [review] Use 'customContent' flag instead of 'bannerBody' We used 'bannerBody' flag to differentiate the case when we move the banner to the body when the notification is expanded from the one when we don't do that and only use the custom content set for the notification, as is the case for Telepathy notifications. We also always cleared the content of the notification on update when bannerBody was set to true. Flag named 'customContent' reflects the use case for it more clearly. The comments that accompany it were also updated and improved. We now always add the banner text as the first element in the expanded notification unless 'customContent' flag is set to true. If the 'body' parameter is specified, we use it in addition to the banner text. The earlier version of the code had a bug that resulted in the 'body' parameter not being set only in the case when the 'bannerBody' was set to true and the banner text had newlines in it.
*** Bug 628241 has been marked as a duplicate of this bug. ***