GNOME Bugzilla – Bug 687113
Better login process indication
Last modified: 2012-11-20 23:29:41 UTC
We need to do a better job of indicating login process. This can sometimes take a few seconds (particularly if you get your password wrong): we need to give better feedback of what's going on. In particular: * The login button should be insensitive (we already do this) * The password field should be insensitive * "Login as another user..." should be insensitive * If authorisation takes more than about half a second, we should show a spinner next to the login button: https://dl.dropbox.com/u/5031519/gnome-shell/login/3-progress.png
Just to clarify the password field is already insensitive too, its styling is wrong though.
Created attachment 227600 [details] [review] First try Some notes: - the sliced image is loaded twice since we use reuse AnimatedIcon from panel, but it's ok - "box-shadow: none" does not work, so I overwrote the definition with a "inset 0px 0px" - the buttons-set signal is not used ATM? I duplicate the signal emission but it could be either removed or we could add a #buttonLayoutFinished method or something - ModalDialog#makeButton is full of side-effects, and without them the method is not useful, maybe addButton(buttonInfo, layoutInfo) would be better as convenience API (defaults to medium-y, etc.) since the point of makeButton is to create a button to add it. Well, the unlock dialog is the first to add actors manually, so I do not know... Jasper, is it what you wanted? - for the moment UnlockDialog#_setWorking could be called inside _updateSensitivity, or I could merge everything in _setWorking (with a potential better name)
Review of attachment 227600 [details] [review]: ::: js/ui/modalDialog.js @@ +154,3 @@ + if (isDefault && !key) { + this._actionKeys[Clutter.KEY_KP_Enter] = action; + this._actionKeys[Clutter.KEY_ISO_Enter] = action; I wasn't thinking this through. This will have to be an "addButton" method, or an "addButton"/"makeButton" pair or something. A constructor like I was thinking shouldn't modify any state like this.
Created attachment 227601 [details] [review] Second try Fixed the entry look after Giovanni's run dialog enhancements and removed the 'buttons-set' signal emission.
Created attachment 227603 [details] [review] Third patch, addButton-powered Ok, we were on the same line then.
Review of attachment 227603 [details] [review]: ::: js/ui/modalDialog.js @@ +166,3 @@ + this._actionKeys[key] = action; + + layoutInfo = Params.parse(layoutInfo, { expand: false, This seems unnecessary to me. @@ +178,3 @@ + + getButtonLayout: function() { + return this._buttonLayout; I'd prefer a rename to this.buttonLayout. ::: js/ui/unlockDialog.js @@ +180,3 @@ + default: true }; + + this.getButtonLayout().visible = true; Yeah, I find this a bit awkward. @@ +181,3 @@ + + this.getButtonLayout().visible = true; + this.addButton(cancelButtonInfo, { expand: true, I don't like having the info split out like this. @@ +234,3 @@ + this._workSpinner.actor.show(); + + Tweener.addTween(this._workSpinner.actor, Just have it appear directly. The fade-out seems a bit extravagant when we already have the thing spinning. (So, this entire function would go away and be replaced with this._workSpinner.actor.visible = ...) @@ +245,3 @@ + time: WORK_SPINNER_ANIMATION_TIME, + transition: 'linear', + onComplete: this._workSpinner.actor.hide I'm not sure this will work correctly.
> Just have it appear directly. The fade-out seems a bit extravagant when we > already have the thing spinning. I am a big fan of the fade. In fact, I'd even go an extra step and make it scale with an overshoot for the fade in, as I've done in the boot process animation -- http://jimmac.fedorapeople.org/gnome3/login.webm (it's a little hard to catch in firefox as it starts right at the beginning when Ffox is still showing its buffering spinner
Created attachment 227631 [details] [review] An other one After much fiddling with the animation, jimmac said "maybe the fade is enough". Other comments are taken in account, except one. I tested the patch before submitting, so "onComplete: this._workSpinner.actor.hide" is working fine. Jasper, I made a mistake while testing the example lambda call you gave me yesterday. hide is a function and the scope is not used so we do not care which one it is: it is just a simple function reference (maybe I missed something)? You'd prefer I add "onCompleteScope: this" or switch to "Lang.bind(this, this._workSpinner.actor.hide)"?
For reference: - fading: http://zzrough.free.fr/files/public/gnome-shell-687113-Better-login-process-indication/0001-Login-better-process-indication-1.0-fade.webm - scale+overshoot: http://zzrough.free.fr/files/public/gnome-shell-687113-Better-login-process-indication/0001-Login-better-process-indication-scale-overshoot.webm Jasper: I also added a check in AnimatedIcon to guard against cumulating timeouts (this was the original reason for the spinner.visible != working check). I still need the check in _setWorking to avoid a glitch as working(true) might be called multiple times.
(In reply to comment #8) > Created an attachment (id=227631) [details] [review] > An other one > > After much fiddling with the animation, jimmac said "maybe the fade is enough". > > Other comments are taken in account, except one. I tested the patch before > submitting, so "onComplete: this._workSpinner.actor.hide" is working fine. > Jasper, I made a mistake while testing the example lambda call you gave me > yesterday. hide is a function and the scope is not used so we do not care which > one it is: it is just a simple function reference (maybe I missed something)? > > You'd prefer I add "onCompleteScope: this" or switch to "Lang.bind(this, > this._workSpinner.actor.hide)"? Neither of these are correct. this._workSpinner.actor.hide needs to be called on this._workSpinner.actor, not this. We typically use: onComplete: function() { this._workSpinner.actor.hide(); }, onCompleteScope: this, Although we're not consistent. Sometimes we use Lang.bind(this, function() { ... }); instead.
Actually, you know what? Don't bother hiding the actor at all. Just keep it at 0 opacity. It will be fine.
Review of attachment 227631 [details] [review]: ::: data/theme/gnome-shell.css @@ +396,3 @@ } +.modal-dialog StEntry:insensitive { Is this badly rebased? I think it's .modal-dialog StEntry:insensitive already in master. ::: js/ui/modalDialog.js @@ -116,3 @@ }, - setButtons: function(buttons) { Keep this as buttonInfos. ::: js/ui/unlockDialog.js @@ +180,3 @@ + default: true }; + + this.getButtonLayout().visible = true; getButtonLayout() still icky. @@ +240,3 @@ + transition: 'linear' + }); + } else Style issue: if one branch has braces, all need to have them.
Created attachment 227645 [details] [review] Fixed patch Yes, I meant on the this._workSpinner.actor scope, not this. But it is still unused... Ok, I'll follow the more verbose style from now on. Isn't it a problem not to hide it? the timeout will keep on running animation will keep on running. It will not draing my battery faster? Anyway, the actor is not hidden anymore with this version. Sorry for the last patch, it was the same as the 2nd one + only the css changes, so indeed you certainly did not find anything you commented fixed. :-/ About the latest review: - no, it still not ".modal-dialog StEntry:insensitive" on master - for "Keep this as buttonInfos.", I think you reviewed the wrong column? It's what I changed to, or did you mean to keep the name "buttons", then?
Review of attachment 227645 [details] [review]: Code looks fine, I'd just like things split out a bit more. ::: js/ui/modalDialog.js @@ +94,3 @@ y_align: St.Align.START }); + this.buttonLayout = new St.BoxLayout({ style_class: 'modal-dialog-button-box', I'd prefer all these changes to modalDialog in a separate patch, please. ::: js/ui/panel.js @@ +100,2 @@ _onVisibleNotify: function() { + if (this.actor.visible && this._timeoutId > 0) I'd prefer this in a separate patch as well.
Created attachment 227817 [details] [review] panel: do not cumulate animations in AnimatedIcon
Created attachment 227818 [details] [review] modalDialog: give more control over the layout
Jasper, I made the same mistake as in #687112: I only changed the unlock dialog and not the login one, so the 3td patch will need more work.
Review of attachment 227817 [details] [review]: "accumulate" The fix is correct, but when would this happen, where we'd repeatedly be setting the visible property to "true", without setting it to "false" first?
Created attachment 227836 [details] [review] panel: do not accumulate icon animations Yes.
Created attachment 228015 [details] [review] modalDialog: give more control over the layout Make the button layout public for callers to be able to have more control over like adding custom widgets. Also, a method addButton is added as convenience for the most frequent usage. This patch fixes a dummy regression where the button was not set in the buttonInfo object.
Review of attachment 228015 [details] [review]: ::: js/ui/modalDialog.js @@ -116,3 @@ }, - setButtons: function(buttons) { Gah, I typed this wrong last time. Keep this as "buttons" @@ +125,3 @@ + let buttonInfo = buttonInfos[i]; + + let xAlignment; This style fixup belongs in a separate patch.
Comment on attachment 227836 [details] [review] panel: do not accumulate icon animations This is obsolete since bug 687583 is fixed.
(In reply to comment #17) > Jasper, I made the same mistake as in #687112: I only changed the unlock dialog > and not the login one, so the 3td patch will need more work. I prefer to attach my updated patches anyway so they are not lost, hence the next attachments.
Created attachment 228121 [details] [review] modalDialog: give more control over the layout Make the button layout public for callers to be able to have more control over like adding custom widgets. Also, the clearButtons and addButton methods are added as convenience for the most frequent usage. Note: I added clearButtons because I need that in the LoginDialog and it makes sense to have a way to clear them properly.
Created attachment 228140 [details] [review] Login: better process indication We need to do a better job of indicating login process. This can sometimes take a few seconds (particularly if you get your password wrong): we need to give better feedback of what's going on. In particular: - The password field insensitive look must match the mockup - "Login as another user..." should be insensitive - If authorisation takes more than about half a second, we should show a spinner next to the login button
This doesn't apply for me.
Allan, I'll rebase once bug 687848 is fixed. Basically this could have been committed without the login part since I suppose the "shared widget" will be based on the unlock dialog as it seems more "correct".
Don't wait on that for now.
Comment on attachment 228140 [details] [review] Login: better process indication I've split this patch, see upcoming comment about the patch series.
Created attachment 229311 [details] [review] modalDialog: give more control over the layout Make the button layout public for callers to be able to have more control over like adding custom widgets. Also, the clearButtons and addButton methods are added as convenience for the most frequent usage.
Created attachment 229312 [details] [review] modalDialog: do not launch disabled button actions The "Sign In" button of the login dialog has its look disabled when the entry is empty, but it can still be triggered by the Enter key. This fixes the modal dialog so it does not trigger the action of an insensitive button, and also means we do not need to connect to the "activate" signal of the entry anymore.
Created attachment 229313 [details] [review] Login: sensitivity fixes The login dialog had these issues: - the entry was not really disabled, you could still edit text - the sensitivity state was not reset on verification failure - the session list was not disabled The unlock dialog had these issues: - "Login as another user..." was not insensitive - redundant password char setting, overwriting the one given by the question The entry insensitive style was also wrong.
Created attachment 229314 [details] [review] Login: add a spinner for better process indication We need to do a better job of indicating login process. This can sometimes take a few seconds (particularly if you get your password wrong): we need to give better feedback of what's going on. This adds a spinner next to the login button if the authorization takes some time.
(In reply to comment #28) > Don't wait on that for now. My bad, I did not understand that when you told me about sharing the widgets. I really need to read between the lines better :) I've reworked the patch series after rebasing: - to fix all those little issues I talked about in the login dialog (entry still editable when log in, still look disabled after one try) - to also fix the modal dialog where the sign in button could still be activated when disabled (did not spot that one before) The last patch "better process indication" was becoming quite big because it needs some moves in login dialog, so I split it in two: - one to fix all the sensitivity bugs (including the style) - one to add the spinner I hope this will ease reviewing.
Review of attachment 229311 [details] [review]: Sure; I'm fine with this.
Review of attachment 229312 [details] [review]: I'd rather have a buttonKeys that mapped to the buttonInfo, so that way we wouldn't need two maps, but only one.
Review of attachment 229313 [details] [review]: Seems fine to me.
Review of attachment 229314 [details] [review]: Seems fine to me.
Created attachment 229323 [details] [review] modalDialog: do not launch disabled button actions The "Sign In" button of the login dialog has its look disabled when the entry is empty, but it can still be triggered by the Enter key. This fixes the modal dialog so it does not trigger the action of an insensitive button, and also means we do not need to connect to the "activate" signal of the entry anymore.
Seems that there's a patch here that needs reviewing...
Review of attachment 229323 [details] [review]: Sure, looks fine.
I guess I didn't hit the "Publish" button hard enough when I reviewed it the first time.
Attachment 229311 [details] pushed as c10e4c3 - modalDialog: give more control over the layout Attachment 229313 [details] pushed as c3cab28 - Login: sensitivity fixes Attachment 229314 [details] pushed as 59a7fdd - Login: add a spinner for better process indication Attachment 229323 [details] pushed as 4c55a6f - modalDialog: do not launch disabled button actions
Yaaaay!
The UnlockDialog patches are wrong. The isPassword and password_char bits were there to allow typing the password after a failure but before the first question for the new attempt came. Without that code, in complex PAM setups you can end up with a visible password.
Created attachment 229517 [details] [review] unlockDialog: set password char on verif. failure Commit c3cab28 removed bits setting the password char that was used to allow typing the password after a failure but before the first question for the new attempt came. Without that code, in complex PAM setups you can end up with a visible password.
Sorry for that, I knew I should have let this code intact. I saw differences between the login and the unlock dialogs, but this should have been kept for bug 687848 as there are many implementation differences (the handling of the first question for instance). The unlock dialog seems to be more polished in this domain.
Review of attachment 229517 [details] [review]: Yes
Attachment 229517 [details] pushed as 393c238 - unlockDialog: set password char on verif. failure