Created attachment 227102 [details] [review]
express,winxp: Make setup look more like design

This patch makes the express installation setup UI looks more like the
latest UI design mockup:

https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install4.png

The vertical positioning of username/password isn't quite right with this patch applied, the mockup shows very little vertical spacing between the entries, with your patch I get more vertical spacing between username and password than between password and product key. The user avatar is also much bigger with your patch than in the mockup. And I'm not sure we should show an avatar at all if we are using the default one?

(NB: there's a duplicate of this bug which was filed some time ago)

(In reply to comment #2)
> The vertical positioning of username/password isn't quite right with this patch
> applied, the mockup shows very little vertical spacing between the entries,
> with your patch I get more vertical spacing between username and password than
> between password and product key. The user avatar is also much bigger with your
> patch than in the mockup.

I'll try to fix these, first 2 should be trivial.

> And I'm not sure we should show an avatar at all if
> we are using the default one?

According to the mockups, we should.

(In reply to comment #3)
> (NB: there's a duplicate of this bug which was filed some time ago)

Yeah, I remembered but I failed to find that bug. :(

Created attachment 227204 [details] [review]
express,winxp: Make setup look more like design

This one fixes the row spacing of labels and entries. I couldn't figure how to tell GtkImage to take a particular number of pixels (setting "pixel-size" prop does not seem to have any effect).

Created attachment 227238 [details]
Review page

The vertical spacing between 'user' and 'password' is still far too much compared to the mockup https://raw.github.com/gnome-design-team/gnome-mockups/master/boxes/boxes-install4.png

Oops, ignore my previous comment, things work much better with your patch actually applied ;)

Either of the change below changed the size of the default icon (but it was far too small)

diff --git a/src/unattended-installer.vala b/src/unattended-installer.vala
index 2f13da9..e3c7c99 100644
--- a/src/unattended-installer.vala
+++ b/src/unattended-installer.vala
@@ -226,7 +226,8 @@ protected virtual void setup_ui () {
         var avatar = new Gtk.Image.from_icon_name ("avatar-default", 0);
         avatar.halign = Gtk.Align.END;
         avatar.valign = Gtk.Align.CENTER;
-        avatar.pixel_size = 128;
+        //avatar.icon_size = Gtk.IconSize.SMALL_TOOLBAR;
+        avatar.pixel_size = 16;
         setup_hbox.pack_start (avatar, false, false);
         avatar.show_all ();
         fetch_user_avatar.begin (avatar);
[

The current patch looks bad on win7, the avatar is not grouped at all with the username/password.
I think you need to have a grid with 2 columns, icon on the left, the rest on the right, and to make sure the avatar spans the 2 rows corresponding to username/passord, and only these 2 rows. Dunno how convenient it is to achieve :(

(In reply to comment #9)
> The current patch looks bad on win7, the avatar is not grouped at all with the
> username/password.
> I think you need to have a grid with 2 columns, icon on the left, the rest on
> the right, and to make sure the avatar spans the 2 rows corresponding to
> username/passord, and only these 2 rows. Dunno how convenient it is to achieve
> :(

I already tried that but failed to make gtk image decide its size based on the adjust columns and I was getting more space between username and password. I'll give it another shot though. I'm actually surprised that setting pixel_size' works for you cause it didn't for me.

Created attachment 227802 [details] [review]
express,winxp: Make setup look more like design

This version should work. I might want to divide it into multiple patches but first I'd want it to be tested and that it looks better for others too.

Screenshots:

http://static.fi/~zeenix/tmp/boxes-express-xp.png
http://static.fi/~zeenix/tmp/boxes-express-win7.png

Two things:

1. I notice now that I didn't manage to keep same distance between avatar and label on its right the same for both cases. I'll investigate that..

2. I asked Jakub on IRC about avatar and 'Express.." label overlapping vertically:

<zeenix> jimmac: hey, the "Express install" label doesn't overlap with the avatar in the vertical direction here, does it? https://raw.github.com/gnome-design-team/gnome-mockups/master/boxes/boxes-install4.png
<jimmac> zeenix, spanning two columns might be good for german "Bittesindssiesohilflichundwatersiehierundwirinstallieriendiesesding"

Created attachment 227803 [details] [review]
express,winxp: Make setup look more like design

This version should also fix issue#1 in my previous comment.

Review of attachment 227803 [details] [review]:

::: src/unattended-installer.vala
@@ +9,2 @@
 private abstract class Boxes.UnattendedInstaller: InstallerMedia {
+    protected const int SETUP_GRID_N_ROWS = 3;

Its not horrible as is, but I would perhaps make this a non-const and increment it each time i added a row to setup_grid in the code. That way it can be used multiple times in the class inheritance

(In reply to comment #14)
> Review of attachment 227803 [details] [review]:
> 
> ::: src/unattended-installer.vala
> @@ +9,2 @@
>  private abstract class Boxes.UnattendedInstaller: InstallerMedia {
> +    protected const int SETUP_GRID_N_ROWS = 3;
> 
> Its not horrible as is, but I would perhaps make this a non-const and increment
> it each time i added a row to setup_grid in the code. That way it can be used
> multiple times in the class inheritance

Yeah. I wonder why GtkGrid doesn't provided num-rows and num-columns props like GtkTable does.

Created attachment 227832 [details] [review]
express,winxp: Make setup look more like design

Replaced the constant with a field as per last review comment.

Review of attachment 227832 [details] [review]:

looks good to me

::: src/winxp-installer.vala
@@ +113,2 @@
         key_entry.get_style_context ().add_class ("boxes-product-key-entry");
+        setup_grid.attach (key_entry, 2, setup_grid_n_rows, 1, 1);

Technically you should increment it here too.

Attachment 227832 [details] pushed as e43ffea - express,winxp: Make setup look more like design

*** Bug 681755 has been marked as a duplicate of this bug. ***