GNOME Bugzilla – Bug 751668
Spinner & shutdown emblem collision
Last modified: 2016-09-20 08:15:55 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.
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.
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.
Review of attachment 306372 [details] [review]: We already have a property for this: Machine.under_construction
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?
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.
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.
Review of attachment 306410 [details] [review]: ack
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?
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.
This last patch must be applied on top of the ones submitted here: https://bugzilla.gnome.org/show_bug.cgi?id=730258
(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. :)
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
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. =/
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.
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.
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.
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.
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.
Review of attachment 306913 [details] [review]: ack
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.
Review of attachment 307039 [details] [review]: I'm guessing it's just a rebase so still ack.
Attachment 307039 [details] pushed as 4752a38 - machine-thumbnailer: Diff. thumb for under construct box