GNOME Bugzilla – Bug 704407
Add user account - move strength meter and hint below password field
Last modified: 2013-08-16 20:50:41 UTC
Created attachment 249419 [details] screenshot Right now the strength meter is at the bottom, and the strength text is still included at the right of the password field (see screenshot). Obviously having the strength text away from the meter is strange. My proposal: * Remove the strength text completely * Move the strength text below the first password field * Move the hint text below the strength meter See: https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/users/add-user-account.png This would allow the extra padding on the right of the dialog to be removed.
I'm working on...
Created attachment 249547 [details] [review] move strength meter and hint below Just for testing purposes due to resizing issues :-( Does anybody have an idea how to fix it?
Review of attachment 249547 [details] [review]: Set the local-username-hint's width-request property to a particular value (say 300). I would also do this for other hint texts to avoid resizing in some locales.
Created attachment 250023 [details] [review] move strength meter below in the accounts dialog
Created attachment 250024 [details] [review] move strength meter below in the password dialog
Review of attachment 250023 [details] [review]: ::: panels/user-accounts/data/account-dialog.ui @@ +66,3 @@ <property name="label" translatable="yes"></property> + <property name="width-request">300</property> + <property name="height-request">50</property> Why set the height request? This seems unnecessary. The dialogue growing vertically doesn't seem like a problem.
(In reply to comment #3) > Review of attachment 249547 [details] [review]: > > Set the local-username-hint's width-request property to a particular value (say > 300). I would also do this for other hint texts to avoid resizing in some > locales. I set the width-request to 300, however I am still facing with resizing (e.g. when you write invalid username with spaces) :-(
(In reply to comment #6) > Review of attachment 250023 [details] [review]: > > ::: panels/user-accounts/data/account-dialog.ui > @@ +66,3 @@ > <property name="label" translatable="yes"></property> > + <property name="width-request">300</property> > + <property name="height-request">50</property> > > Why set the height request? This seems unnecessary. The dialogue growing > vertically doesn't seem like a problem. aday don't want also height resizing of the dialog...
Created attachment 250844 [details] [review] move strength meter below in the accounts dialog Resizing problems fixed thanks to Federico.
Created attachment 250845 [details] [review] move strength meter below in the password dialog Fixed also for password dialog.
Created attachment 250878 [details] [review] move strength meter below in the accounts dialog Fixed width to be similar as in mockup, reverted removed sizegroup by mistake.
Created attachment 250879 [details] [review] move strength meter below in the password dialog Same width as in the add user dialog.
Created attachment 250930 [details] [review] move strength meter below in the password dialog Rebased to master.
Created attachment 250931 [details] [review] move strength meter below in the add user dialog Rebased to master. Other design changes discussed with aday.
Created attachment 250943 [details] [review] hint improvements when adding enterprise account Hint changes on the enterprise part of add user dialog according the mockup.
Created attachment 251093 [details] [review] move strength meter below in the password dialog Label value changed after today discussion with Allan.
Created attachment 251094 [details] [review] move strength meter below in the add user dialog Label width reduced and label value changed after today discussion with Allan.
These patches do not apply on top of patches from bug #702476. JHBUILD [stef@stef gnome-control-center]$ git bz apply https://bugzilla.gnome.org/show_bug.cgi?id=704407 Bug 704407 - Add user account - move strength meter and hint below password field 250943 - hint improvements when adding enterprise account 251093 - move strength meter below in the password dialog 251094 - move strength meter below in the add user dialog Apply? [(y)es, (n)o, (i)nteractive] y Applying: user-accounts: Hint improvements when adding enterprise accounts fatal: sha1 information is lacking or useless (panels/user-accounts/data/account-dialog.ui). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 user-accounts: Hint improvements when adding enterprise accounts The copy of the patch that failed is found in: /data/src/gnome-control-center/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/hint-improvements-when-adding-enterprise-account-wmehsP.patch
It is buildable in the following order (with patches from the depending bug): 251093 - move strength meter below in the password dialog 251094 - move strength meter below in the add user dialog 250943 - hint improvements when adding enterprise account (sorry for that, I have it in different order in different branches, so I didn't export it with correct numbers, but I should have)
Review of attachment 250943 [details] [review]: This patch also does other things, noted below. I'm not sure the below changes are warranted. I don't see explanation on the bug. If they are indeed necessary, could you separate them into other patches with appropriate explanation? ::: panels/user-accounts/data/account-dialog.ui @@ +475,3 @@ + <property name="yalign">0</property> + <property name="xalign">0</property> + <property name="label" translatable="yes">Should match the web address of your account provider.</property> This string is present both in the C and GtkBuilder file. Is that necessary? ::: panels/user-accounts/um-account-dialog.c @@ -995,3 @@ - gtk_label_set_text (GTK_LABEL (self->enterprise_hint), message); - finish_action (self); - gtk_widget_grab_focus (GTK_WIDGET (self->enterprise_password)); Why are you combining these into one? They really are separate conditions, and it's helpful to the user to be able to correct them separately. @@ -1296,3 @@ - /* Initialize enterprise domain from DHCP. */ - res_init (); Why have you taken out the res_init() line?
Review of attachment 251093 [details] [review]: This looks good. I'd like to suggest adding a few comments and notes: ::: panels/user-accounts/data/password-dialog.ui @@ +72,3 @@ <property name="visible">True</property> <property name="xalign">1</property> + <property name="label" translatable="yes">_Verify New Password</property> Is it the intent that this label is different in the new "Add User" dialog? There we just have "Verify"? Just double checking here. Certainly not a blocker for accepting the patch. ::: panels/user-accounts/um-password-dialog.c @@ -90,3 @@ - } else { - strength_hint = hint; - } Could you add a note about this in the commit message? @@ -423,3 @@ - height = gtk_widget_get_allocated_height (um->strength_indicator_label); - gtk_widget_set_size_request (label, allocation->width, height * 3); -{ Could you add a comment about the changes to the strength hint to the commit message? @@ -502,2 @@ widget = (GtkWidget *)gtk_builder_get_object (builder, "password-hint"); - g_signal_connect (widget, "size-allocate", G_CALLBACK (hint_allocate), um); A comment about why this is necessary would be helpful to later hackers looking at the code.
Review of attachment 251094 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ -157,3 @@ - spinner = self->enterprise_check_credentials ? self->spinner : self->enterprise_spinner; - gtk_widget_show (GTK_WIDGET (spinner)); - gtk_spinner_start (spinner); Should the spinner stuff be a separate patch? Or at the very least mention these changes in the commit message. @@ -291,3 @@ - } else { - strength_hint = hint; - } Again, a comment about this in the commit message makes sense.
Created attachment 251726 [details] [review] move strength meter below in the password dialog Verify hint added (to show "Passwords does not match."). Comment message improved. The label "Verify new password." is correct (discussed with designer).
Created attachment 251727 [details] [review] move strength meter below in the add user dialog dtto
Created attachment 251729 [details] [review] hint improvements when adding enterprise account Fixed by review and split to separate patches.
Created attachment 251730 [details] [review] do not prefill domain entry This feature is removed, because it is mostly unuseful and confusing. Also when wrong domain is prefilled user see error hint, but can't see the default domain hint explaining him what is it good for. It would be better to prefill one from the saved realms instead, but this will be added maybe later.
Review of attachment 251730 [details] [review]: Why are you removing res_init()? That shouldn't be dropped.
Review of attachment 251730 [details] [review]: Also, what testing did you do about this? This would make sense as a separate bug report with real background information. Case in point: Realmd is supposed to be checking that the DHCP domain is joinable before having it show up here. If not there's a bug.
Created attachment 251950 [details] [review] user-accounts: let realmd prefil the DHCP domain if it's joinable
Review of attachment 251726 [details] [review]: Looks good.
Review of attachment 251729 [details] [review]: Looks good.
Review of attachment 251727 [details] [review]: Looks good.
Review of attachment 251950 [details] [review]: Actually, this makes sense. The patch log message was wrong. We still allow realmd to prefill from DHCP if it's a joinable domain. We just don't try to do it directly, which I agree was a bad idea.
Attachment 251950 [details] pushed as 12da809 - user-accounts: let realmd prefil the DHCP domain if it's joinable