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

Revision history for this message
dobey (dobey) 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).

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?

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?

review: Needs Fixing

« Back to merge proposal