Code review comment for lp:~unity-team/unity/phablet-pinlock

Revision history for this message
Michael Zanetti (mzanetti) wrote :

> 670 + LightDM.Greeter.authenticate("has-pin")
> Testing code?

Don't really know. Somehow this needs to be triggered so designers and testers can have a go. I'm still unsure who actually should trigger this when the screen locks... This seemed like a sensible place to me. Where would you trigger the locking?

> 715 + } else if (authenticationUser == "has-pin"){
> 716 + Q_EMIT q->showPrompt("Please enter your PIN:",
> Greeter::PromptTypeSecret);
>
> Please make that just "PIN". That's what the pam_pin module does. And
> Shell.qml should check for an exact match, not an indexOf. In the UI, you can
> check for the string and present a nicer one if needed.

Done.

> 723 - Q_EMIT q->showPrompt("Password: ", Greeter::PromptTypeSecret);
> 724 + Q_EMIT q->showPrompt("Please enter your password:",
> Greeter::PromptTypeSecret);
>
> PAM will likely just give us "Password:", so we should keep our mock close to
> that. Plus, you'll have to change some of the qmltests that check for
> "Password" being passed back. You can, however, check in the UI if the string
> is "Password" and present something nicer. It's not translated from PAM.
> (Which reminds me, we should pass it through gettext in LoginList.qml)
>
> 732 - authenticated = (response == "password");
> 733 + if (authenticationUser == "has-password") {
> 734 + authenticated = (response == "password");
>
> Actually, not only has-password has a password. All the users are assumed to
> have "password" as their password unless otherwise specified. (And no-
> password users never get to the handleRespond() function) So please just
> check for the pin user and fallback to checking for "password".

Done.

> 751 + { "has-pin", "Has PIN", 0, 0, false, false, 0, 0 },
>
> I see you added the user to the "full" mock lightdm. I think it would be more
> useful to add a new mock liblightdm that only has the pin user. That way we
> can get a more phone-like experience.

No. Because I need also the password user for the tests. This code does not only handle PINs but also passphrases by now. Don't think its useful to have another two single-user backends for those use cases when it can be handled by the full backend easily.

« Back to merge proposal