GNOME Bugzilla – Bug 644737
PolicyKit authentication dialog updates
Last modified: 2011-03-14 18:20:37 UTC
Jon asked me to make the PolicyKit dialogs look more like the mockups at https://live.gnome.org/GnomeShell/Design/Whiteboards/AuthorizationDialog
Created attachment 183355 [details] [review] Proposed patch Here's a patch to do this. And here are some screenshots of the dialog with the patch applied http://people.freedesktop.org/~david/gnome-shell-polkit-update-1.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-2.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-3.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-4.png (note: the "Swipe was too short, try again" is red in -4 and not yellow as in the mockup simply because the pam_fprintd.so pam module emits an error message and not an info message. So if you want this changed, you need to bug the fprint folks.)
Thanks for doing this. Some comments: Info messages should be in normal text color and error messages should be in yellow. Could we also have a bit more space between the key icon and the heading text as in the mockups (use the full color mockups for spacing)? The vertical space between sections isn't quite enough. It makes it look squishy and hard to read. Can we increase the space as in the mockups? "Authentication failure" would be better as "Sorry, that didn't work. Please try again." as in the mockups. As you probably already know, the text doesn't really seem to match the mockups very well in some cases.
(In reply to comment #2) > Thanks for doing this. Some comments: > > Info messages should be in normal text color and error messages should be in > yellow. Oh, I see. Done. > Could we also have a bit more space between the key icon and the heading text > as in the mockups (use the full color mockups for spacing)? Done. But not sure how to get the size from the mockup so I just guessed - feel free to finetune the CSS yourself when this lands. > The vertical space between sections isn't quite enough. It makes it look > squishy and hard to read. Can we increase the space as in the mockups? Done. Same disclaimer as above. > "Authentication failure" would be better as "Sorry, that didn't work. Please > try again." as in the mockups. We get this string from PAM so you need to talk to the maintainers of the PAM modules, sorry. > As you probably already know, the text doesn't really seem to match the mockups > very well in some cases. I'm not exactly sure what you mean here (please be more specific), but if it's about the message text, it comes directly from the application performing the action (Mechanism). As I said on IRC there will be a way to make this look closer to the mockup.
Created attachment 183356 [details] [review] Updated patch http://people.freedesktop.org/~david/gnome-shell-polkit-update-take-two-1.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-take-two-2.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-take-two-3.png http://people.freedesktop.org/~david/gnome-shell-polkit-update-take-two-4.png
(In reply to comment #3) > I'm not exactly sure what you mean here (please be more specific), but if it's > about the message text, it comes directly from the application performing the > action (Mechanism). As I said on IRC there will be a way to make this look > closer to the mockup. For the record, this is what I said <davidz> mccann: and the next polkit release will allow mechanisms to provide something like "$(application) requires Administrator privileges to install new software" where the shell can replace $(application) so it will read "Software Update requires Administrator privileges to install new software" or "An Unknown Application requires Administrator privileges to install new software" ... which should improve the text in the dialogs somewhat but it requires a little bit of work to do correctly
(In reply to comment #3) > > "Authentication failure" would be better as "Sorry, that didn't work. Please > > try again." as in the mockups. > > We get this string from PAM so you need to talk to the maintainers of the PAM > modules, sorry. I looked into this and it's actually incorrect. The string "Authentication Failure" stems from pam_strerror() when pam_authenticate() fails. Which I now believe is wrong to show to the end-user (might be fine in a log though). This requires a fix in polkit. So I'll update the patch so it shows "Sorry, that didn't work" when authentication fails AND no other error or info message is being shown... then the right will happen once there's an updated polkit version.
(In reply to comment #6) > (In reply to comment #3) > > > "Authentication failure" would be better as "Sorry, that didn't work. Please > > > try again." as in the mockups. > > > > We get this string from PAM so you need to talk to the maintainers of the PAM > > modules, sorry. > > I looked into this and it's actually incorrect. The string "Authentication > Failure" stems from pam_strerror() when pam_authenticate() fails. Which I now > believe is wrong to show to the end-user (might be fine in a log though). This > requires a fix in polkit. > > So I'll update the patch so it shows "Sorry, that didn't work" when > authentication fails AND no other error or info message is being shown... then > the right will happen once there's an updated polkit version. Achieved with this patch + if (!gainedAuthorization) { + /* Unless we are showing an existing error message from the PAM + * module (the PAM module could be reporting the authentication + * error providing authentication-method specific information), + * show "Sorry, that didn't work. Please try again." + */ + if (!this._errorMessageLabel.visible) { + this._errorMessageLabel.set_text(_('Sorry, that didn\'t work. Please try again.')); + this._errorMessageLabel.show(); + this._infoMessageLabel.hide(); + this._nullMessageLabel.hide(); + } + } and this polkit patch http://cgit.freedesktop.org/PolicyKit/commit/?id=bd8e8eaf320d6a9a30361f0523d3a032e56e6143 which is now on master, things work as expected http://people.freedesktop.org/~david/gnome-shell-polkit-sorry-dave.png I'll update this patch with the change above.
Created attachment 183362 [details] [review] Take three
Created attachment 183367 [details] [review] Take four Minor changes after some extensive testing - Don't show "Sorry that didn't work" if the dialog is dismissed (it was visible for a very short period) - Don't cancel a completion authentication session (it caused "Sorry that didn't work" to be shown for a very short period) - Add some spacing (requested by Jon) I'm going to commit this as Jon seemed happy about it on IRC. We can tweak stuff post-landing.
Created attachment 183368 [details] [review] Take five Fix spelling and factual errors in the commit message.
Committed here: http://git.gnome.org/browse/gnome-shell/commit/?id=9b55de1c6b0bb0acb2738ec36ba83f7c7e3aeef6