Code review comment for lp:~mterry/unity8/greeter-refactor

> """
> QtObject {
> id: d
> }
> """
>
> The intention is good, but right now it's just dead code.

I tend to use QtObject 'd' blocks like that as barriers between the "public" code and the implementation code. So they are useful as a visual separator, even if empty. But alright, removed.

> In tst_Greeter.qml [and usersmodel.cpp, Greeter.qml, and CoverPage.qml]
>
> """
> * Copyright 2013 Canonical Ltd.
> """

Fixed.

> """
> property bool ready: greeterBackground.source == "" || greeterBackground.status == Image.Ready || greeterBackground.status == Image.Error
> """
>
> This line is too long.

Fixed by removing it. It's not actually used anywhere now with this refactor.

> tst_Greeter.tst_launcherOffsetAnimation is not really checking that the value got animated back to zero.

Fair enough, used your version. Thanks!

> Now that the lightdm mock libs are unified we no longer need separate test files for different lightdm mock modes.

Fair enough, used your version. And added a line for "single-pin", though I didn't take the further step of consolidating tst_Shell.qml and tst_ShellWithPin.qml. That can be cleanup for some future branch.

> Shouldn't you also set LightDM.Greeter.mockMode to "full" as well? I'm not
> seing it being done anywhere in this file.
>
> Hmm, maybe it doesn't matter as LightDM.Greeter doesn't seem to be used by WideView anyway.

That's why I didn't have it. The views should never need the LightDM.Greeter object, they should get everything from the Greeter.qml class.

> Please at least add an inline comment saying what this boolean is about, like:
>
> """
> d.startUnlock(false /* toTheRight */);
> """

Done.

> """
> loader.item.authenticated(false);
> """
>
> Please name it like a function, instead of like a boolean property.

Fair, especially since LightDM.Greeter is already a property that exists in and around this code.

I've changed it to notifyAuthenticationFailed and notifyAuthenticationSucceeded (I don't like re-using the on* prefix because that same pattern is used for signals).

> I'm getting some test failures:
> [snip]

Fixed. I got overzealous in my consolidation of app-switching code in r1493.

=================

OK, that should be all your comments addressed.