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 645719 - Make sure notification has all the content when it is expanded
Make sure notification has all the content when it is expanded
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other All
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-03-26 06:31 UTC by Marina Zhurakhinskaya
Modified: 2011-03-29 02:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make sure notification has all the content when it is expanded (5.63 KB, patch)
2011-03-26 06:31 UTC, Marina Zhurakhinskaya
none Details | Review
Make sure notification has all the content when it is expanded (3.08 KB, patch)
2011-03-27 19:17 UTC, Marina Zhurakhinskaya
reviewed Details | Review
Make sure notification has all the content when it is expanded (2.88 KB, patch)
2011-03-28 20:04 UTC, Marina Zhurakhinskaya
committed Details | Review

Description Marina Zhurakhinskaya 2011-03-26 06:31:38 UTC
Waiting for the banner body to be added in the main loop when we are
showing an expanded notification was causing it to show up with an
ellipsized banner and only get the full content after it is done animating.
This was the case for notifications that are shown fully expanded the
first time they are ever shown, such as the summary notifications that
were not shown as banners because the user was in the Busy mode or was
interacting with the summary.

This patch reworks how we handle adding expanded content. It ensures that
we do so when it is needed and that we only do so once. Previously, we used
to schedule numerous content updates that each resulted in Notification
emitting the 'expanded' signal. Processing these updates was holding up
processing other things, such as the user moving the mouse away from the
notification so that the notification collapses.
Comment 1 Marina Zhurakhinskaya 2011-03-26 06:31:43 UTC
Created attachment 184268 [details] [review]
Make sure notification has all the content when it is expanded
Comment 2 Marina Zhurakhinskaya 2011-03-27 19:17:15 UTC
Created attachment 184380 [details] [review]
Make sure notification has all the content when it is expanded

Use Meta.later_add() with BEFORE_REDRAW rather than Mainloop.idle_add()
to add the banner body so that the notification body is reliably added
after the first frame is drawn. This is important for notifications that
are expanded right away, such as the summary notifications that
were not shown as banners because the user was in the Busy mode or was
interacting with the summary. Otherwise, these notifications were sometimes
shown with an ellipsized banner and were only getting full content when
they were done animating.

Only add expanded content and signal the change once. Previously, we
used to signal the change numerous times and processing this signal was
holding up processing other things, such as the user moving the mouse
away from the notification so that the notification collapses.
Comment 3 Dan Winship 2011-03-28 18:07:50 UTC
Comment on attachment 184380 [details] [review]
Make sure notification has all the content when it is expanded

>Only add expanded content and signal the change once. Previously, we
>used to signal the change numerous times

it seems like it would be better to move most of the conditionals outside of the later, and only schedule that function if it's actually going to do something. (You can test this._bannerBodyText != null to see if _addBannerBody() is going to do something.)
Comment 4 Marina Zhurakhinskaya 2011-03-28 20:04:18 UTC
Created attachment 184504 [details] [review]
Make sure notification has all the content when it is expanded

Only schedule or execute expanding content when it is necessary.
Comment 5 Dan Winship 2011-03-28 20:08:15 UTC
Comment on attachment 184504 [details] [review]
Make sure notification has all the content when it is expanded

yeah, looks good