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 773882 - No way to know when Games looks for games
No way to know when Games looks for games
Status: RESOLVED FIXED
Product: gnome-games
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Games maintainers
GNOME Games maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-03 10:07 UTC by Adrien Plazas
Modified: 2017-01-31 10:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Show a spinner notification when games are being loaded. (9.76 KB, patch)
2017-01-27 21:09 UTC, Abhinav Singh
none Details | Review
Show a spinner notification when games are being loaded. (8.08 KB, patch)
2017-01-30 14:06 UTC, Abhinav Singh
none Details | Review
Show a spinner notification when loading games (7.92 KB, patch)
2017-01-30 16:00 UTC, Abhinav Singh
none Details | Review
Show a spinner notification when loading games (8.62 KB, patch)
2017-01-31 09:14 UTC, Abhinav Singh
none Details | Review
Show a spinner notification when loading games (8.44 KB, patch)
2017-01-31 09:54 UTC, Abhinav Singh
committed Details | Review

Description Adrien Plazas 2016-11-03 10:07:04 UTC
There is no way to know when Games i looking for games.

Maybe adding a spinner to the headerbar could do the trick?
Comment 1 Abhinav Singh 2017-01-27 21:09:46 UTC
Created attachment 344440 [details] [review]
Show a spinner notification when games are being loaded.

Added an overlayed revealable notification over collection box.
Comment 2 Adrien Plazas 2017-01-30 08:09:22 UTC
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.
Comment 3 Abhinav Singh 2017-01-30 14:06:18 UTC
Created attachment 344551 [details] [review]
Show a spinner notification when games are being loaded.

Added an overlayed revealable notification over collection icon view box.
Comment 4 Adrien Plazas 2017-01-30 14:37:50 UTC
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).
Comment 5 Adrien Plazas 2017-01-30 15:01:15 UTC
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.
Comment 6 Abhinav Singh 2017-01-30 16:00:33 UTC
Created attachment 344563 [details] [review]
Show a spinner notification when loading games
Comment 7 Adrien Plazas 2017-01-31 07:23:10 UTC
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.
Comment 8 Adrien Plazas 2017-01-31 07:24:17 UTC
Review of attachment 344551 [details] [review]:

Please mark deprecated patches as such. :)
Comment 9 Abhinav Singh 2017-01-31 09:14:12 UTC
Created attachment 344633 [details] [review]
Show a spinner notification when loading games
Comment 10 Adrien Plazas 2017-01-31 09:22:52 UTC
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
Comment 11 Abhinav Singh 2017-01-31 09:54:22 UTC
Created attachment 344636 [details] [review]
Show a spinner notification when loading games
Comment 12 Adrien Plazas 2017-01-31 10:05:12 UTC
Review of attachment 344636 [details] [review]:

LGTM
Comment 13 Adrien Plazas 2017-01-31 10:08:10 UTC
Review of attachment 344636 [details] [review]:

LGTM