GNOME Bugzilla – Bug 776157
Improve notifications' interaction
Last modified: 2017-02-18 15:20:37 UTC
See the patches below
Created attachment 342049 [details] [review] window: Use a push/pop model for loading notifications The current loading notification is handled by a private function that is accessed outside the Window object. This is not only a poor design decision, as the notification object is an implementation detail of the window, and external objects can't rely on that. Fix that by hiding the loading notifications management behind a push/pop pair of calls.
Created attachment 342050 [details] [review] window: Use one notification widget for loading Instead of instantiating the notification widget every time we start a new loading notification, put a static one and only show and hide it. This patch also makes the loading notification a GtkRevealer widget.
Created attachment 342051 [details] [review] window: Improve the playlist remove notification Following the previous patch, this patch turns the playlist notification into a GtkRevealer, and reorganize the code to better expose that. Again, the notification was made an implementation detail and the method was renamed to be public.
Created attachment 342052 [details] [review] project: Remove GdNotification from compiled libgd list We completely wiped out all the traces of GdNotification, we can safely drop it now.
Review of attachment 342049 [details] [review]: minor adjustments ::: gnomemusic/window.py @@ +82,3 @@ self.set_icon_name('gnome-music') self.notification_handler = None + self.loading_counter = 0 self._loading_counter @@ +525,3 @@ + """ Increases the counter of loading notification triggers running. If + there is no notification is visible, the loading notification is + started. block comments and docstrings have line length at 72 chars (nitpicking i know)
Review of attachment 342050 [details] [review]: lgtm ::: gnomemusic/window.py @@ +112,3 @@ + def _setup_loading_notification(self): + self.loading_notification = Gtk.Revealer(halign=Gtk.Align.CENTER, + valign=Gtk.Align.START) self._loading_notification
Review of attachment 342051 [details] [review]: Why not stay in the terminology and push_playlist_notification? ::: gnomemusic/window.py @@ +32,2 @@ import gi gi.require_version('Gd', '1.0') you can remove these 2 lines with Gd gone I did notice however that at least 1 other module doesn't import it correctly itself (which was hidden by this one). @@ +142,3 @@ + @log + def _setup_playlist_notification(self): + self.playlist_notification_timeout_id = 0 self._playlist_notification_timeout_id @@ +143,3 @@ + def _setup_playlist_notification(self): + self.playlist_notification_timeout_id = 0 + self.playlist_notification = Gtk.Revealer(halign=Gtk.Align.CENTER, self._playlist_notification
Review of attachment 342052 [details] [review]: lgtm
Awesome patches. I don't think we should overthink at this time and the replacement of libgd is the important aspect. Eventually we should maybe make a little notification class to get the code burden out of the window.py . One small feature request I have is for the notifications (especially loading) to have a little grace time (0-100/200ms) where they can be pushed but don't get revealed yet. My feeling is that a lot of loading times are too fast they result in a reveal and immediate hide.
Created attachment 342349 [details] [review] window: Use a push/pop model for loading notifications Fixed.
Created attachment 342350 [details] [review] window: Use one notification widget for loading Fixed.
Created attachment 342351 [details] [review] window: Improve the playlist remove notification I didn't use the push/pop model because the Playlists notification doesn't use it. We don't push playlists removal notification, just show them.
Created attachment 342356 [details] [review] window: Show loading notification after a small delay When loading e.g. an artists, the notification can show and hide very quickly, causing a very bad impression since we didn't really spend any time loading it. Fix that by setting a small timeout of 500ms before showing the notification.
Pushed with some minor stylistic modifications & cleanups. Thanks for the patches! Attachment 342349 [details] pushed as b8da571 - window: Use a push/pop model for loading notifications Attachment 342350 [details] pushed as 1f7a335 - window: Use one notification widget for loading Attachment 342351 [details] pushed as b1c9ce4 - window: Improve the playlist remove notification Attachment 342356 [details] pushed as 35f0393 - window: Show loading notification after a small delay
*** Bug 754404 has been marked as a duplicate of this bug. ***