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 751668 - Spinner & shutdown emblem collision
Spinner & shutdown emblem collision
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on: 730258
Blocks:
 
 
Reported: 2015-06-29 17:48 UTC by Zeeshan Ali
Modified: 2016-09-20 08:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot of the issue (75.53 KB, image/png)
2015-06-29 17:48 UTC, Zeeshan Ali
  Details
libvirt-machine: Add 'creation_state' prop (2.65 KB, patch)
2015-06-30 08:33 UTC, Adrien Plazas
rejected Details | Review
collection-view: Use machine creation state (3.65 KB, patch)
2015-06-30 08:33 UTC, Adrien Plazas
needs-work Details | Review
collection-view: Add get_thumbnail() (1.55 KB, patch)
2015-06-30 13:55 UTC, Adrien Plazas
accepted-commit_now Details | Review
collection-view: Draw empty frame on construction (2.85 KB, patch)
2015-06-30 13:55 UTC, Adrien Plazas
needs-work Details | Review
machine-thumbnailer: Add construction thumbnail (2.31 KB, patch)
2015-07-01 14:43 UTC, Adrien Plazas
needs-work Details | Review
machine-thumbnailer: Diff. thumb for under construct box (2.37 KB, patch)
2015-07-02 06:41 UTC, Adrien Plazas
needs-work Details | Review
machine-thumbnailer: Diff. thumb for under construct box (2.30 KB, patch)
2015-07-02 14:55 UTC, Adrien Plazas
needs-work Details | Review
machine-thumbnailer: Diff. thumb for under construct box (2.22 KB, patch)
2015-07-06 11:30 UTC, Adrien Plazas
accepted-commit_now Details | Review
machine-thumbnailer: Diff. thumb for under construct box (2.22 KB, patch)
2015-07-07 20:22 UTC, Adrien Plazas
committed Details | Review

Description Zeeshan Ali 2015-06-29 17:48:10 UTC
Created attachment 306312 [details]
Screenshot of the issue

In the fix for bug#730258, we forgot about the spinner of importing VM. They overlap each other.
Comment 1 Adrien Plazas 2015-06-30 08:33:06 UTC
Created attachment 306372 [details] [review]
libvirt-machine: Add 'creation_state' prop

Add the 'creation_state' prop to tell the current creation state of a
Libvirt machine.

This will be needed in the next commit to check if a machine is being
imported and need to have a special thumbnail.
Comment 2 Adrien Plazas 2015-06-30 08:33:10 UTC
Created attachment 306373 [details] [review]
collection-view: Use machine creation state

Use the machine's creation state (if any) to check if the machine is
importing and its thumbnail shouldn't display the 'system-shutdown'
emblem.

This is needed to avoid conflict between the spinner notifying that the
machine is importing and the 'system-shutdown' emblem notifying that the
machine is stopped.
Comment 3 Zeeshan Ali 2015-06-30 12:37:55 UTC
Review of attachment 306372 [details] [review]:

We already have a property for this: Machine.under_construction
Comment 4 Zeeshan Ali 2015-06-30 12:46:05 UTC
Review of attachment 306373 [details] [review]:

* Shortlog could be more specific.

* 'notifying' -> 'showing' or 'reporting'.

::: src/collection-view.vala
@@ +85,3 @@
         Machine machine = item as Machine;
         return_if_fail (machine != null);
+        var pixbuf = get_thumbnail (machine);

Not an issue introduced by this patch but seems we paint empty or stopped thumbnail even for the cases where it won't be used (which would hopefully be the case most of the time).

@@ +95,3 @@
 
+    private Gdk.Pixbuf get_thumbnail (Machine machine) {
+        if (is_stopped (machine))

You are moving the logic of which thumbnail to use to this new method. I think this is a separate change.

@@ +123,3 @@
+
+    private Gdk.Pixbuf paint_stopped_machine_thumbnail () {
+        return add_centered_emblem_icon (paint_empty_machine_thumbnail (), "system-shutdown-symbolic", BIG_EMBLEM_SIZE);

don't see why we'd want to change to function call in args from use of var.

@@ +252,3 @@
             store.set (iter, ModelColumns.INFO, machine.info);
             queue_draw ();
+            update_screenshot (iter);

Why is this needed now?
Comment 5 Adrien Plazas 2015-06-30 13:55:17 UTC
Created attachment 306410 [details] [review]
collection-view: Add get_thumbnail()

This is needed to keep the thumbnail's selection clean as it is expected
to become more complex in the next commit.
Comment 6 Adrien Plazas 2015-06-30 13:55:22 UTC
Created attachment 306411 [details] [review]
collection-view: Draw empty frame on construction

Check if the machine is under construction to check if the machine is
importing and its thumbnail shouldn't display the 'system-shutdown'
emblem.

This is needed to avoid conflict between the spinner notifying that the
machine is importing and the 'system-shutdown' emblem showing that the
machine is stopped.
Comment 7 Zeeshan Ali 2015-06-30 18:13:23 UTC
Review of attachment 306410 [details] [review]:

ack
Comment 8 Zeeshan Ali 2015-06-30 18:21:16 UTC
Review of attachment 306411 [details] [review]:

* the shortlog totally confused me. How about "Empty frame for under-construction machines".
* under-construction isn't just for importing case (as description says) but is also for under installation etc. It's just that the underlying issue only realizes for under-import VMs since their machines remain stopped during the import.
* you missed some comments from previous review, at least about "notifiying".

::: src/collection-view.vala
@@ +97,3 @@
+        if (is_stopped (machine))
+            // If the machine is stopped but importing, it will draw a spinner itself, so we don't need to show the
+            // 'system-shutdown' emblem.

Same comment here: under_construction doesn't just imply import.

@@ +253,3 @@
             setup_activity (iter, machine);
             queue_draw ();
+            update_screenshot (iter);

why is this needed, again?
Comment 9 Adrien Plazas 2015-07-01 14:43:35 UTC
Created attachment 306526 [details] [review]
machine-thumbnailer: Add construction thumbnail

Add get_construction_thumbnail() and check if the machine is under
construction to draw an apropriate thumbnail.

This is needed to avoid conflict between the spinner showing that the
machine is under construction and the 'system-shutdown' emblem showing
that the machine is stopped.
Comment 10 Adrien Plazas 2015-07-01 14:44:18 UTC
This last patch must be applied on top of the ones submitted here: https://bugzilla.gnome.org/show_bug.cgi?id=730258
Comment 11 Zeeshan Ali 2015-07-01 15:39:39 UTC
(In reply to Adrien Plazas from comment #10)
> This last patch must be applied on top of the ones submitted here:
> https://bugzilla.gnome.org/show_bug.cgi?id=730258

Ok, let's set deps then. :)
Comment 12 Zeeshan Ali 2015-07-01 16:08:31 UTC
Review of attachment 306526 [details] [review]:

* "Add construction thumbnail" -> "Diff. thumb for under construct box".

* Summarize the change, not explain code in English. :) It's *very* easy to make that mistake, so don't feel bad about it.

* Could you please add ", which happens in case of imports where machine remains in stopped state during the import" at the end?

::: src/machine-thumbnailer.vala
@@ +23,3 @@
 
+            if (is_stopped (machine))
+                // If the machine is stopped but being constructed, it can still be under construction.

comment is basically saying "if x then it can mean x" so not useful.

@@ +96,3 @@
     }
 
+    private static Gdk.Pixbuf get_construction_thumbnail () {

get_under_construction_thumbnail or get_under_construct_thumbnail
Comment 13 Adrien Plazas 2015-07-02 06:40:46 UTC
Review of attachment 306526 [details] [review]:

* "Add construction thumbnail" -> "Diff. thumb for under construct box".

"machine-thumbnailer: Diff. thumb for under construct box" is too long. =/
Comment 14 Adrien Plazas 2015-07-02 06:41:51 UTC
Created attachment 306582 [details] [review]
machine-thumbnailer: Diff. thumb for under construct box

Add get_construction_thumbnail() to draw an appropriate thumbnail for
under construction boxes.

This is needed to avoid conflict between the spinner showing that the
machine is under construction and the 'system-shutdown' emblem showing
that the machine is stopped, which happens in case of imports where
machine remains in stopped state during the import.
Comment 15 Zeeshan Ali 2015-07-02 13:56:25 UTC
Review of attachment 306582 [details] [review]:

Shortlog isn't very long, just a bit longer than recommended 50 chars and important bits are within 50 chars.

* get_under_construction_thumbnail() is private so it's a detail that need not be mentioned in the log.

* Good otherwise.

::: src/machine-thumbnailer.vala
@@ +60,3 @@
+
+        if (is_stopped (machine))
+            // If the machine is stopped, check if it still is under construction.

"If the machine is stopped" part is very obvious from the condition just above. Actually the rest is too so this would be overdocumentation.
Comment 16 Adrien Plazas 2015-07-02 14:55:31 UTC
Created attachment 306633 [details] [review]
machine-thumbnailer: Diff. thumb for under construct box

This is needed to avoid conflict between the spinner showing that the
machine is under construction and the 'system-shutdown' emblem showing
that the machine is stopped, which happens in case of imports where
machine remains in stopped state during the import.
Comment 17 Zeeshan Ali 2015-07-04 15:24:12 UTC
Review of attachment 306633 [details] [review]:

almost there :)

::: src/machine-thumbnailer.vala
@@ +43,3 @@
+
+        if (machine.is_stopped)
+            // If the machine is stopped, check if it still is under construction.

you missed the comment on this one I think.
Comment 18 Adrien Plazas 2015-07-06 11:30:27 UTC
Created attachment 306913 [details] [review]
machine-thumbnailer: Diff. thumb for under construct box

This is needed to avoid conflict between the spinner showing that the
machine is under construction and the 'system-shutdown' emblem showing
that the machine is stopped, which happens in case of imports where
machine remains in stopped state during the import.
Comment 19 Zeeshan Ali 2015-07-06 13:22:36 UTC
Review of attachment 306913 [details] [review]:

ack
Comment 20 Adrien Plazas 2015-07-07 20:22:35 UTC
Created attachment 307039 [details] [review]
machine-thumbnailer: Diff. thumb for under construct box

This is needed to avoid conflict between the spinner showing that the
machine is under construction and the 'system-shutdown' emblem showing
that the machine is stopped, which happens in case of imports where
machine remains in stopped state during the import.
Comment 21 Zeeshan Ali 2015-07-07 21:44:48 UTC
Review of attachment 307039 [details] [review]:

I'm guessing it's just a rebase so still ack.
Comment 22 Zeeshan Ali 2015-07-07 21:50:41 UTC
Attachment 307039 [details] pushed as 4752a38 - machine-thumbnailer: Diff. thumb for under construct box