Created attachment 234103 [details] [review]
add user account dialog redesgin

There are changes for non-enterprise users (adding password options) as in mockup. Changes for enterprise users I'll send later.

Review of attachment 234103 [details] [review]:

::: panels/user-accounts/data/account-dialog.ui
@@ +34,3 @@
             <property name="border_width">6</property>
             <property name="margin_bottom">6</property>
+            <property name="width_request">550</property>

I don't think hardcoding a width here is right. Why is this necessary ?

@@ +287,3 @@
+                        </style>
+                        <attributes>
+                          <attribute name="scale" value="0.84"/>

In other panels, I have used 0.8 for 'small print'. We should be consistent here. Its probably worth looking over the other panels to compare.

@@ +507,3 @@
             <child>
               <object class="GtkButton" id="button2">
+                <property name="label" translatable="yes">_Enrole</property>

That should be _Enroll
And should probably have a translator hint

@@ +561,3 @@
                 <property name="yalign">0</property>
                 <property name="label" translatable="yes">In order to use enterprise logins, this computer needs to be
+enrolled in the domain. Please have your network Administrator type their domain password here.</property>

I think administrator should not be capitalized here.

::: panels/user-accounts/pw-utils.c
@@ +121,1 @@
 

uppercase and lowercase in one word (each)

Created attachment 234229 [details] [review]
add user account dialog redesgin

Attached fixed patch as per reviewer and designer comments. Validation is also slightly changed.

Created attachment 234230 [details]
add user dialog screenshot

(In reply to comment #2)
> Review of attachment 234103 [details] [review]:
> 
> ::: panels/user-accounts/data/account-dialog.ui
> @@ +34,3 @@
>              <property name="border_width">6</property>
>              <property name="margin_bottom">6</property>
> +            <property name="width_request">550</property>
> 
> I don't think hardcoding a width here is right. Why is this necessary ?

Actually it isn't necessary, however there is another ugly hardcoded value for password hint, because of updating hint changes its width (dialog width):

<property name="max_width_chars">25</property>

Do you have better idea how to solve this?

> @@ +287,3 @@
> +                        </style>
> +                        <attributes>
> +                          <attribute name="scale" value="0.84"/>
> 
> In other panels, I have used 0.8 for 'small print'. We should be consistent
> here. Its probably worth looking over the other panels to compare.

Other panels are using values:

panels/keyboard/gnome-keyboard-panel.ui: 0.82999999999999996
panels/mouse/gnome-mouse-properties.ui: 0.82999999999999996
panels/power/power.ui: 0.82999999999999996
panels/universal-access/uap.ui: 0.82999999999999996
panels/wacom/wacom-stylus-page.ui: 0.82999999999999996
panels/user-accounts/data/password-dialog.ui: 0.83333333333329995

So I have changed value to 0.82999999999999996 to be consistent across major of panels.

Created attachment 235419 [details] [review]
enterprise part redesign of add user account dialog

Redesign for enterprise users according slightly changed mockup.

Created attachment 235420 [details]
screenshot for enterprise part

Created attachment 235521 [details] [review]
enterprise part redesign of add user account dialog

Fix segmentation fault during domain checking caused by dialog canceling. 

Domain checking only while editing credentials or after click to "Add" button.

(In reply to comment #5)
> (In reply to comment #2)
> > Review of attachment 234103 [details] [review] [details]:
> > 
> > ::: panels/user-accounts/data/account-dialog.ui
> > @@ +34,3 @@
> >              <property name="border_width">6</property>
> >              <property name="margin_bottom">6</property>
> > +            <property name="width_request">550</property>
> > 
> > I don't think hardcoding a width here is right. Why is this necessary ?
> 
> Actually it isn't necessary, however there is another ugly hardcoded value for
> password hint, because of updating hint changes its width (dialog width):
> 
> <property name="max_width_chars">25</property>
> 
> Do you have better idea how to solve this?

Hardcoding the width in characters is less bad than hardcoding a number of pixels, since it will work with font and font size changes.

Created attachment 235525 [details] [review]
replace stop sign by warning sign in add user dialog

Can you repost the entire series ? The first patch (add user account dialog redesign) doesn't apply to git master.

Created attachment 236036 [details] [review]
add user account dialog redesgin

Created attachment 236037 [details] [review]
enterprise part redesign of add user account dialog

Created attachment 236039 [details] [review]
replace stop sign by warning sign in add user dialog

Ohdrej has very kindly done a screencast for me so I can review this. Some points based on the video:

 * We need to make sure that the dialog mirrors what is presented in initial setup.
 * Tooltips should be avoided.
  * For the user name, it would be better to have a hint text below the field, like in initial setup. This can update if there are errors: "The username should consist of lower and upper case letters from a-z and should not include spaces."
 * The dialog appears to change height when the password is entered - it shouldn't - the height should always be the same.

 * Enterprise login:
  * The progress indication for checking the domain only appears to start once the login name is being entered.
  * We should think about adding a check mark next to the domain once it has been validated.
  * Error for a non-discovered domain ("No such domain or realm found") could be better. "Domain not found" could be better.

 * The user list in the panel seems to change width after a user is added.

(In reply to comment #15)
> Ohdrej has very kindly done a screencast for me so I can review this. Some
> points based on the video:
> 
>  * We need to make sure that the dialog mirrors what is presented in initial
> setup.
>  * Tooltips should be avoided.
>   * For the user name, it would be better to have a hint text below the field,
> like in initial setup. This can update if there are errors: "The username
> should consist of lower and upper case letters from a-z and should not include
> spaces."
>  * The dialog appears to change height when the password is entered - it
> shouldn't - the height should always be the same.

Account dialog in g-i-s has static labels and using tooltips, so this is contradictory. Problem with labels I'll discuss next.

>  * Enterprise login:
>   * The progress indication for checking the domain only appears to start once
> the login name is being entered.

Checking is started by typing login, typing password, or submitting dialog if the domain hasn't been checked. 

>   * We should think about adding a check mark next to the domain once it has
> been validated.

We can add it, however I think it isn't necessary.

>   * Error for a non-discovered domain ("No such domain or realm found") could
> be better. "Domain not found" could be better.

I'll fix the error messages in realm manager.
 
>  * The user list in the panel seems to change width after a user is added.

That's probably another bug.

There are several things which was discussed on #gnome-hackers today. Firstly there are problems with updating multiline labels for passwords and login hints, because labels changes its size and also dialog size during updating. We need these updating labels, because password is checking using libpwquality which is returning different error messages. 

<Jasper> oholy, have all of the labels in a GdStack and switch between them, maybe?
<oholy> Jasper: I have been thinging about, but problem is, that we have very much messages from libpwquality library...
<Jasper> oholy, it's impossible without hardcoding something

Secondly there are dicussed libpwquality useful. Rules for changing password are too complicated and password changing is flustrating. Probably we can't simply remove libpwquality checking and make own checking rules.

<Jasper> oholy, note that I don't think the strings from libpwquality are in any way usable, though
<aday> i don't see why we need libpwquality messages here
<aday> we probably only want 2 or 3 messages
<Jasper> mclasen_afk, it's impossible to know how big you want a label if you don't know all the possible strings...
<oholy> aday: I think we need more messages then 2 or 3: same or similar, palindrom, contains username or name or forbidden words or dictionary words... these messages are important, because without them user will be confused...

...

<ebassi> aday: last time, Marta decided to change her password because it was too simple; she tried, and got rejected by the UI. it was fairly frustrating, so she gave up

...

<aday> Jasper, oholy, so looking at this, i think we're maybe making it too complicated
<aday> the only requirement i think we should have for a password is a minimum number of characters
<aday> and we can just have a single hint that doesn't update - "Try to use at least 8 different characters. Mix upper and lower case and use a number or two."
<Jasper> I know mclasen added libpwquality for a reason...
<Jasper> I think the security team wanted it in.
<Jasper> So I think we should talk to them about it.

...

<oholy> aday:  when we remove libpwquality from checking password, we probably should remove strength indicator also...
<aday> the strength indicator is fine
<aday> provided that it guides and doesn't punish
<oholy> yes, but it depends on libpwquality and when user type "password09770WRR.//;" it says "Not good enought", but user don't know why....

So it's necessary to find solution for mentioned problems and probably change mockup accordingly, also we should to change password dialog, because both panels share same codes and make changes only in one of them is complicated.

Created attachment 237048 [details] [review]
add user account dialog redesign

Username hint added. Username and password hints have 3 lines space allocated, so dialog doesn't change its height. Strength hint fixed also.

Created attachment 237049 [details] [review]
enterprise part redesign of add user account dialog

Check mark added. Common error messages fixed.

Created attachment 237051 [details]
add user dialog redesign

Refreshed screenshot.

Review of attachment 237048 [details] [review]:

::: panels/user-accounts/data/account-dialog.ui
@@ +72,3 @@
+                    </style>
+                    <attributes>
+                      <attribute name="scale" value="0.82999999999999996"/>

weird number.

::: panels/user-accounts/um-account-dialog.c
@@ +64,3 @@
         GtkSpinner *spinner;
 
+        /* Mode areas widgets */

I don't understand what this means. The original comment was clearer to me.

@@ +313,3 @@
+                valid_password = strength > 0 && strcmp (password, verify) == 0;
+        }
+        else {

closing curly brace on the same line as the else

::: panels/user-accounts/um-utils.c
@@ +524,3 @@
                 }
                 else {
+                        *tip = g_strdup (_("The username should consist of lower and upper case letters from a-z, digits and any of characters '.', '-' and '_'"));

"should only" consists. Otherwise it makes people think that the username must include all those different characters.

Review of attachment 237049 [details] [review]:

::: panels/user-accounts/um-account-dialog.c
@@ +825,3 @@
+        const gchar *message;
+
+        if (g_cancellable_is_cancelled (self->cancellable)) {

That's not the correct way to check for cancellation.
Before you call um_realm_login_finish(), you shouldn't access any members of the user dialog, so:
- UmAccountDialog *self = UM_ACCOUNT_DIALOG (user_data);
  that needs to happen later

@@ +832,2 @@
         um_realm_login_finish (result, &creds, &error);
         if (error == NULL) {

if (!um_realm_login (result, &creds, &error)) {
  if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
   return;
}

@@ +905,3 @@
+        gchar *message;
+
+        if (g_cancellable_is_cancelled (self->cancellable)) {

Same thing here.

Thanks for review.

(In reply to comment #22)
> Review of attachment 237048 [details] [review]:
> 
> ::: panels/user-accounts/data/account-dialog.ui
> @@ +72,3 @@
> +                    </style>
> +                    <attributes>
> +                      <attribute name="scale" value="0.82999999999999996"/>
> 
> weird number.

Please look upper at comment 5. Yes, it's probably wrongly rounded, so I'll fix it on 0.83. 

(In reply to comment #23)
> Review of attachment 237049 [details] [review]:
> 
> ::: panels/user-accounts/um-account-dialog.c
> @@ +825,3 @@
> +        const gchar *message;
> +
> +        if (g_cancellable_is_cancelled (self->cancellable)) {
> 
> That's not the correct way to check for cancellation.
> Before you call um_realm_login_finish(), you shouldn't access any members of
> the user dialog, so:
> - UmAccountDialog *self = UM_ACCOUNT_DIALOG (user_data);
>   that needs to happen later

I'll investigate it more, but I haven't got a better idea currently, how to avoid of critical errors when user cancel form submitting. 

I'll fix rest of the mentioned things also.

Created attachment 237171 [details] [review]
add user account dialog redesign

Fixed patch according reviewer comments.

Created attachment 237172 [details] [review]
enterprise part redesign of add user account dialog

Created attachment 239568 [details] [review]
userfriendly password hints

There are finally user-friendly password hints followed by necessary changes of other patches.

Created attachment 239569 [details] [review]
add user account dialog redesign

Created attachment 239570 [details] [review]
enterprise part redesign of add user account dialog

Thanks to Ondrej for helping me to get this running. In general this looks excellent. A few small pieces of feedback:

 * The text that is displayed about the strength bar suffers from being overly critical, eg: "Strength: Not good enough". I would stick to Strength: Low/Good/High, etc.

 * With a single character in the password field, the hint changes to "Try to avoid reordering words." It's confusing! Ideally we'd only update the hint once there are more characters.

 * When the hint updates to be two lines, the window height changes.

 * I also see some rearrangement of the UI when deleting the contents of the first password entry field.

 * Once the password is acceptable, I think we should revert to displaying the default hint. It makes it look like the password isn't good enough, when it is.

 * It might be a good idea to use the hint text to indicate when the passwords don't match.

(In reply to comment #30)
> Thanks to Ondrej for helping me to get this running. In general this looks
> excellent. A few small pieces of feedback:
> 
>  * The text that is displayed about the strength bar suffers from being overly
> critical, eg: "Strength: Not good enough". I would stick to Strength:
> Low/Good/High, etc.

Can you specify it more? Do you want just use "Strength: Low/Good/High" instead of "Week/Fair/Strong"? What we should to show instead of "Not good enough"?

>  * With a single character in the password field, the hint changes to "Try to
> avoid reordering words." It's confusing! Ideally we'd only update the hint once
> there are more characters.

I'll fix it.

>  * When the hint updates to be two lines, the window height changes.
> 
>  * I also see some rearrangement of the UI when deleting the contents of the
> first password entry field.

Unfortunately I don't face up any size changes recently. Same questions as in Bug 695450#c4...?

>  * Once the password is acceptable, I think we should revert to displaying the
> default hint. It makes it look like the password isn't good enough, when it is.

Actually default hint is displayed, however default hint isn't displayed at startup. I'll fix it.

>  * It might be a good idea to use the hint text to indicate when the passwords
> don't match.

That's great idea, I'll fix it.

(In reply to comment #31)
> (In reply to comment #30)
> > Thanks to Ondrej for helping me to get this running. In general this looks
> > excellent. A few small pieces of feedback:
> > 
> >  * The text that is displayed about the strength bar suffers from being overly
> > critical, eg: "Strength: Not good enough". I would stick to Strength:
> > Low/Good/High, etc.
> 
> Can you specify it more? Do you want just use "Strength: Low/Good/High" instead
> of "Week/Fair/Strong"? What we should to show instead of "Not good enough"?

I would just say "Strength: Low", "Strength: Medium", "Strength: Good", "Strength: High", or something along those lines. The main thing is to avoid "not good enough", since that isn't a very nice thing to say to someone.

...
> >  * When the hint updates to be two lines, the window height changes.
> > 
> >  * I also see some rearrangement of the UI when deleting the contents of the
> > first password entry field.
> 
> Unfortunately I don't face up any size changes recently. Same questions as in
> Bug 695450#c4...?

I only observed changes in height here. The issue I pointed out in bug 695450 was only about width changes.

Created attachment 241247 [details] [review]
userfriendly password hints

Hint change from Not good enought/Week/Fair/Good/Strong to Strenght: Week/Strenght: Low/Strenght: Medium/Strenght: Good/Strenght: High.

Created attachment 241248 [details] [review]
add user account dialog redesign

- Fix for size changes. 
- Hint when passwords don't match.
- Hint updates with timeout.

Can you check the resizing issue again, please?

Created attachment 241249 [details] [review]
enterprise part redesign of add user account dialog

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

I rebased the patches onto master with a few minor changes and also squashed
the two redesign patches. Any further issues can be filed as new bugs.

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