GNOME Bugzilla – Bug 773882
No way to know when Games looks for games
Last modified: 2017-01-31 10:08:48 UTC
There is no way to know when Games i looking for games. Maybe adding a spinner to the headerbar could do the trick?
Created attachment 344440 [details] [review] Show a spinner notification when games are being loaded. Added an overlayed revealable notification over collection box.
Review of attachment 344440 [details] [review]: It looks great! I have a nitpick though: the way the UI is splitted in Games is that way: - each page has its own header bar and content box - a page's headerbar is the toplevel widget to show as the headerbar of the window - a page's content box is the toplevel widget to show as the content of the window It's not written in HACKING and it probably should. :) With that in mind, the widget called CollectionBox should be the toplevel one.
Created attachment 344551 [details] [review] Show a spinner notification when games are being loaded. Added an overlayed revealable notification over collection icon view box.
Review of attachment 344551 [details] [review]: What would happen if the window is created after the timeout but before all games are loaded? I suspect the notification would never show up. Maybe we should ensure the window is created before or we should store the state in the application… the first one sound simpler and cleaner to me. Looks good otherwise! ::: src/ui/application-window.vala @@ +57,3 @@ } + public bool loading_notification { get; set; } Other setters are before the getters, let's do the same here! ::: src/ui/application.vala @@ +140,3 @@ + + if (loading_notification_timeout_id != null) + Source.remove (loading_notification_timeout_id); You should make the ID null here as you just made it invalid. ::: src/ui/collection-box.vala @@ +7,3 @@ public ListModel collection { construct set; get; } public bool search_mode { set; get; } + public bool loading_notification { get; set; } Other setters are before the getters, let's do the same here! @@ +30,3 @@ BindingFlags.BIDIRECTIONAL); + loading_notification_binding = bind_property ("loading-notification", + loading_notification_revealer, "reveal-child", BindingFlags.DEFAULT); It looks like alignment is wrong here, put the 3 first params on the previous line and align the last one correctly (even if it breaks the length rules).
Review of attachment 344551 [details] [review]: I forgot to comment the commit message: - we don't use final punctuation for the shortlog (the first line), - we avoid preterit for actions done by the patch itself (i.e. "Add" rather than "Added"). - the first line should be 52 chars long max when possible, in this case "ui: Show a spinner notification when loading games" would work better.
Created attachment 344563 [details] [review] Show a spinner notification when loading games
Review of attachment 344563 [details] [review]: Almost there! :) ::: data/ui/collection-box.ui @@ +38,3 @@ + <property name="valign">start</property> + <child> + <object class="GtkGrid"> Why did you choose a GtkGrid if there is only one line? It looks like it would be simpler with a GtkBox. What motivated your choice? @@ +57,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">Loading</property> You need to add this file to POTFILES.in.
Review of attachment 344551 [details] [review]: Please mark deprecated patches as such. :)
Created attachment 344633 [details] [review] Show a spinner notification when loading games
Review of attachment 344633 [details] [review]: Besides the positions it's ready to be commited. :) ::: data/ui/collection-box.ui @@ +51,3 @@ + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">0</property> Do GtkBoxes have a position packaging property? @@ +63,3 @@ + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">1</property> Ditto @@ +81,3 @@ + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">2</property> Ditto
Created attachment 344636 [details] [review] Show a spinner notification when loading games
Review of attachment 344636 [details] [review]: LGTM