GNOME Bugzilla – Bug 686936
Initial experience confusion
Last modified: 2016-03-31 13:59:21 UTC
The first time you run boxes, it opens with the new box dialog. There are a few issues with this: * Once you have created your first box there is a back button, but it isn't clear where back leads to * Once you get back to the boxes overview, it is unclear where you are or what it is for My suggestion would be to show the empty overview grid on first run, with a suggestion to create a box. We do something similar for the other apps: * https://raw.github.com/gnome-design-team/gnome-mockups/master/documents/documents-empty.png * https://raw.github.com/gnome-design-team/gnome-mockups/master/contacts/png/no-contacts.png
https://raw.github.com/gnome-design-team/gnome-mockups/master/boxes/boxes-empty.png
that icon is barely visible - maybe it should be lighter gray, same as the text ?
Created attachment 234268 [details] [review] Add initial experience greeting Introducing EmptyBoxes, a new actor/widget that is shown when there is no boxes in the collection view. This is as per the design: https://raw.github.com/gnome-design-team/gnome-mockups/master/boxes/boxes-empty.png Issues: * The 'application-x-appliance-symbolic' icon just landed in git master of gnome-icon-theme-symbolic so maybe we want to keep a private copy of that to not bump our requirement.
Created attachment 234269 [details] [review] No need to launch wizard on first time anymore Showing wizard automatically doesn't make sense anymore when we show the initial greeting message in the main view.
I think at this point we should just rely on 3.8 stuff.
Review of attachment 234268 [details] [review]: ::: src/empty-boxes.vala @@ +29,3 @@ + var label = new Gtk.Label ("<b><span size=\"large\">" + + _("No boxes found") + + "</span></b>"); Why are you using two labels here? Just put in a newline between the two? @@ +65,3 @@ + private void update_visibility () { + var visible = (ui_state == UIState.COLLECTION || ui_state == UIState.NONE) && + App.app.collection.items.length == 0; Isn't there a risk this will be temporarily shown during startup? Should we not wait until the startup has reached the "ready" state? My patch in bug 692306 has some helpers for this.
Review of attachment 234269 [details] [review]: Looks good, but it conflicts with patches in bug 692306
Review of attachment 234268 [details] [review]: ::: src/empty-boxes.vala @@ +29,3 @@ + var label = new Gtk.Label ("<b><span size=\"large\">" + + _("No boxes found") + + "</span></b>"); Mainly because thats what gnome-document's code does and I based this code on that. Do newlines guarantee proper alignment against the image? @@ +65,3 @@ + private void update_visibility () { + var visible = (ui_state == UIState.COLLECTION || ui_state == UIState.NONE) && + App.app.collection.items.length == 0; No, since this function is only called on App.app.ready, App.app.collection.item_added and App.app.collection.item_removed. Actually its very easy to test since currently libvirtd takes a while to autostart.
Review of attachment 234269 [details] [review]: Meh, I guess one of us will have to rebase then. :)
Review of attachment 234269 [details] [review]: too late for you sucker!!! :)
Review of attachment 234268 [details] [review]: ::: src/empty-boxes.vala @@ +29,3 @@ + var label = new Gtk.Label ("<b><span size=\"large\">" + + _("No boxes found") + + "</span></b>"); Newline cause alignment according to the label align mode as a new paragraph. Should be ok. @@ +35,3 @@ + labels_grid.add (label); + + label = new Gtk.Label ("Create one using the button on the top left."); Need _() here. @@ +65,3 @@ + private void update_visibility () { + var visible = (ui_state == UIState.COLLECTION || ui_state == UIState.NONE) && + App.app.collection.items.length == 0; cool
Created attachment 234649 [details] [review] Add initial experience greeting v2: Rebase & mark a string for translation.
Created attachment 234650 [details] [review] No need to launch wizard on first time anymore v2: Rebased.
Review of attachment 234649 [details] [review]: ack
Review of attachment 234650 [details] [review]: ack
Attachment 234649 [details] pushed as ad7dd98 - Add initial experience greeting Attachment 234650 [details] pushed as 8796e6f - No need to launch wizard on first time anymore
This seems to be racy, the libvirt connection is done asynchronously, and the 'no VM' screen can be briefly seen during startup if connecting to libvirt is a bit longer than usual.
(In reply to comment #18) > This seems to be racy, the libvirt connection is done asynchronously, and the > 'no VM' screen can be briefly seen during startup if connecting to libvirt is a > bit longer than usual. Yeah, I told Alex about this yesterday. The first patches here didn't have this issue but then it regressed after I rebased against Alex' latest patches. Should have done more testing after the rebase. :( I'll fix soon.
Created attachment 234986 [details] [review] empty-boxes: Don't show thyself before App is ready
Created attachment 234988 [details] [review] app: Put EmptyBoxes actor below all its siblings This fixes the issue of user being unable to click on notificationbar button(s) when EmptyBoxes is displayed.
Review of attachment 234986 [details] [review]: ::: src/empty-boxes.vala @@ +64,3 @@ private void update_visibility () { + App.app.call_when_ready (() => { + var visible = ui_state == UIState.COLLECTION && App.app.collection.items.length == 0; Am I right in thinking that the change fixing the bug is this one, and that the rest of this patch is only code cleanup?
Review of attachment 234988 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Review of attachment 234986 [details] [review]: ::: src/empty-boxes.vala @@ +64,3 @@ private void update_visibility () { + App.app.call_when_ready (() => { + var visible = ui_state == UIState.COLLECTION && App.app.collection.items.length == 0; Dont see any code-cleanup. The only other change is removal of connection to 'App.app.ready' and that becomes irrelevant with this change.
Review of attachment 234986 [details] [review]: ::: src/empty-boxes.vala @@ +64,3 @@ private void update_visibility () { + App.app.call_when_ready (() => { + var visible = ui_state == UIState.COLLECTION && App.app.collection.items.length == 0; Most of the change in this patch is switching from App.app.ready to call_when_ready, which does not seem related to this bugfix, the bugfix seems to be only the 'var visible = ...' change
Review of attachment 234986 [details] [review]: ::: src/empty-boxes.vala @@ +64,3 @@ private void update_visibility () { + App.app.call_when_ready (() => { + var visible = ui_state == UIState.COLLECTION && App.app.collection.items.length == 0; I'm only removing 'ui_state == UIState.NONE' from that line and that change *alone* does not fix this bug. I'm only remove that check because I realized its redundant, at least after this fix.
Review of attachment 234986 [details] [review]: ::: src/empty-boxes.vala @@ +64,3 @@ private void update_visibility () { + App.app.call_when_ready (() => { + var visible = ui_state == UIState.COLLECTION && App.app.collection.items.length == 0; Ah yeah, rereading the patch, I see what it does now, a bit indirect. don't be afraid of writing longer commit logs ;)
Attachment 234986 [details] pushed as cf4d1a4 - empty-boxes: Don't show thyself before App is ready Attachment 234988 [details] pushed as 721208d - app: Put EmptyBoxes actor below all its siblings
*** Bug 693834 has been marked as a duplicate of this bug. ***