(In reply to comment #0)
> Right now if a pam module displays a session message we flash it by so fast the
> user won't have time to read it.
> 
> We should either add a small timeout, or add a button to continue.

How import is this message?

"Last login ..." ... don't really seem important enough to slow down login ... (continue button would be even worse).

We could stick it as a prop on the root win and show it as a notification once the session is up (assume it is still relevant there).

(I agree Last Login shouldn't be done through a pam module)

I think we need to introduce a delay before clearly for "in between" messages regardless.  Right now if the pam stack sends multiple messages we overwrite them in succession.

I'll see if i can come up with a patch.  For the last message, doing the notification thing might make sense, but i'm worried it could lead to out of context messages showing up.

Created attachment 239091 [details] [review]
gdm: don't clear user-verifier on reset automatically

Right, the common code between the login screen and
the unlock screen handles clearing the user verifier
when GDM sends a reset.

We don't actually always want to clear the messages on
reset in the unlock case, though, so doing it implicitly
is problematic.

This commit moves the clear() call from the common code
to the specific reset handlers.

Created attachment 239092 [details] [review]
unlockDialog: ignore resets after success

GDM sends a reset signal after verification succeeds
so that a user-switched login screen can prepare for
the next time it's going to be used.

The unlock screen treats resets as failures, though.
This means, on success, we're emitting "failed" and
clearing any last second messages.

This commit changes the unlock code to ignore resets from
GDM after successful verification.

Created attachment 239093 [details] [review]
loginDialog,unlockDialog: Give user time to read messages

Right now, if multiple messages come in, they just sort of
clobber each other.

This commit sets up a message queue, and introduces pauses
long enough for the user to hopefully be able to read those
messages.

i'd like the push the above 3 patches for 3.8 (provided they look okay), but we should leave the bug open to think about alternate ways to show end messages to the user, and more specifically alternate ways of showing last login to user.

I still don't like the idea of slowing down login to show a useless "last login" message that 99,99% of the users don't care about ...

sure, but the fix for that is a distro fix. That is to make sure we don't show use pam_lastlog for gdm. this is more general.

To be clear, before filing this bug I talked to Tomas Mraz in Fedora bugzilla and he agreed to take pam_lastlog out.

Review of attachment 239091 [details] [review]:

Commit message "Right, ..." -> "Right now ..." otherwise looks good.,

Review of attachment 239092 [details] [review]:

OK.

Review of attachment 239093 [details] [review]:

Looks good, but it means that we MUST kill the last login message otherwise we'd be adding ~1 second to our already not so fast login process ...
Maybe we could try to start the login process while the messages are being displayed but that's something for 3.10 if we want to do that.

::: js/gdm/util.js
@@ +30,3 @@
 
+// Give user 50ms to read each character of a PAM message
+const USER_READ_TIME = 50

Not sure about this number (means 5 seconds for 100 chars which sounds a bit too much) but ok.

@@ +191,3 @@
+        GLib.idle_add(GLib.PRIORITY_DEFAULT,
+                      Lang.bind(this, function() {
+                          this.emit('no-more-messages');

Why the idle handler and not just emitting the signal right away?

https://bugzilla.gnome.org/show_bug.cgi?id=694688

Hi,

(In reply to comment #12)
> Review of attachment 239093 [details] [review]:
> 
> Looks good, but it means that we MUST kill the last login message otherwise
> we'd be adding ~1 second to our already not so fast login process ...
Yup, I agree.  That's a distro issue, but shouldn't be a problem.

> +// Give user 50ms to read each character of a PAM message
> +const USER_READ_TIME = 50
> 
> Not sure about this number (means 5 seconds for 100 chars which sounds a bit
> too much) but ok.
I'll make the time shorter.
 
> @@ +191,3 @@
> +        GLib.idle_add(GLib.PRIORITY_DEFAULT,
> +                      Lang.bind(this, function() {
> +                          this.emit('no-more-messages');
> 
> Why the idle handler and not just emitting the signal right away?
No good reason as far as I can tell.

Attachment 239091 [details] pushed as e9584cf - gdm: don't clear user-verifier on reset automatically
Attachment 239092 [details] pushed as f72f501 - unlockDialog: ignore resets after success
Attachment 239093 [details] pushed as d097327 - loginDialog,unlockDialog: Give user time to read messages

fwiw, the downstream bug alluded to in comment 9 is https://bugzilla.redhat.com/show_bug.cgi?id=915371