Code review comment for lp:~rodrigo-moya/ubuntuone-client/use-sso-in-u1-prefs

Revision history for this message
Rodrigo Moya (rodrigo-moya) wrote :

> It looks like test_login_check in the tests is skipped at the moment, but it
> should be updated to comply with the changes here too. I'd also avoid using
> globals for the oauth_token and oauth_consumer variables, and store them as
> attributes in the dialog (and pass them on to the other classes and methods
> that might need them).
>
test_login_check still depends on the real DBus service to be run, not sure how we can make it work without depending on the tests in ubuntu-sso-client. If there's a way, I'm ok with doing it, but not in this branch, which is needed for the new release to be done today or tomorrow at the latest.

> I'm still confused about the changes for the dialog creation though. Why did
> you change it to not create the dialog when starting up? Shouldn't the dialog
> be created and shown, and its window id passed over to ubuntu_sso for setting
> as the parent?
>
well, what was the point of having a dialog with all the widgets showing temporary data ('Unknown', etc)? So, now, we create the dialog associated to the LoginHandler, and show it when we get the credentials or when calling _present

> The dialog creation was how it was, in order to show the dialog to the user as
> soon as possible, so that they know things are happening. Is there a good
> reason to not do that now?
>
if the credentials are in the keyring, the dialog will show up inmediately, if not, they will see the SSO login/register screen

« Back to merge proposal