GNOME Bugzilla – Bug 697340
Decouple the account settings (and thus the account widgets) from the keyring
Last modified: 2013-07-25 13:08:05 UTC
I'm trying to isolate the account widgets and their support infrastructure in a tp-account-widgets submodule to be shared with gnome-online-accounts for its chat providers. Currently the keyring is implemented as a set of functions not backed by a real object, and they're used both in code that would belong to the above submodule and in Empathy-specific code. The keyring also has some compile-time support for Ubuntu Online Accounts, which is not currently needed in the submodule. We have the following choices: 1) move the keyring as-is in the tp-account-widgets, complete of uoa support, and use the provided functions in the other parts of Empathy which need it; 2) convert the keyring in a singleton object with virtual methods, move a base implementation in tp-account-widgets, add UOA support in a Empathy-specific subclass and explicitly pass it to AccountSettings; 3) same as 2) but leave the *_room_password() functions unchanged since they're not needed by AccountSettings: this has some issues since the following internal functions are shared between the *_password() methods and we either duplicate them or make them public from tp-account-widgets: secret_password_lookup(), lookup_item_cb(), secret_password_store(), store_password_cb(); 4) go shopping. Even if choice 4 is tempting, I'm attaching patches which implement 2. I've not tested the UOA stuff as I don't have an Ubuntu environment handy, but I'm willing to do it once the approach has been ack'ed.
Created attachment 240735 [details] [review] keyring: Make it a real singleton object Later on, this would make refactoring the account widgets easier as we could then subclass it to eg. implement the UOA support separatedly.
Created attachment 240736 [details] [review] keyring: Use virtual functions as source tags for GSimpleAsyncResult
Created attachment 240737 [details] [review] keyring: Split BaseKeyring and move the UOA support out of it
Created attachment 240738 [details] [review] keyring: Encapsulate virtual functions in public accessors
Created attachment 240739 [details] [review] account-settings: Take a BaseKeyring as construction parameter Instead of relying on the global EmpathyKeyring singleton, accept a BaseKeyring at instantiation time such that AccountSettings does no longer rely on anything provided by EmpathyKeyring.
Created attachment 240744 [details] [review] account-settings: Take a BaseKeyring as construction parameter Instead of relying on the global EmpathyKeyring singleton, accept a BaseKeyring at instantiation time such that AccountSettings does no longer rely on anything provided by EmpathyKeyring.
Review of attachment 240736 [details] [review]: ::: libempathy/empathy-keyring.c @@ +269,2 @@ simple = g_simple_async_result_new (G_OBJECT (account), callback, + user_data, klass->get_account_password_async); Actually you should use the static function itself. If you get there it means that klass->get_account_password_async == keyring_get_account_password_async, right? Likewise for all others
(In reply to comment #7) > Review of attachment 240736 [details] [review]: > > ::: libempathy/empathy-keyring.c > @@ +269,2 @@ > simple = g_simple_async_result_new (G_OBJECT (account), callback, > + user_data, klass->get_account_password_async); > > Actually you should use the static function itself. If you get there it means > that klass->get_account_password_async == keyring_get_account_password_async, > right? And _finish() functions should check for the static func as well. Subclasses must override both _async() and _finish() because they could be using GTask instead of GSimpleAsyncResult for example.
Review of attachment 240735 [details] [review]: ::: libempathy/empathy-keyring.c @@ +785,3 @@ + GAsyncReadyCallback callback, gpointer user_data) +{ + EmpathyKeyring *keyring = empathy_keyring_dup_singleton (); keyring is leaked. Likewise for all others
(In reply to comment #7) > ::: libempathy/empathy-keyring.c > @@ +269,2 @@ > simple = g_simple_async_result_new (G_OBJECT (account), callback, > + user_data, klass->get_account_password_async); > > Actually you should use the static function itself. If you get there it means > that klass->get_account_password_async == keyring_get_account_password_async, > right? Nope, later on we chain up to the base method and it must be able to create a task with the same tag. We may use empathy_base_keyring_get_account_password_async for the task tag, your call.
Created attachment 242079 [details] [review] keyring: Make it a real singleton object Later on, this would make refactoring the account widgets easier as we could then subclass it to eg. implement the UOA support separatedly.
Created attachment 242080 [details] [review] keyring: Use virtual functions as source tags for GSimpleAsyncResult
Created attachment 242081 [details] [review] keyring: Split BaseKeyring and move the UOA support out of it
Created attachment 242082 [details] [review] keyring: Encapsulate virtual functions in public accessors
Created attachment 242083 [details] [review] account-settings: Take a BaseKeyring as construction parameter Instead of relying on the global EmpathyKeyring singleton, accept a BaseKeyring at instantiation time such that AccountSettings does no longer rely on anything provided by EmpathyKeyring.
(In reply to comment #8) > > Actually you should use the static function itself. If you get there it means > > that klass->get_account_password_async == keyring_get_account_password_async, > > right? > > And _finish() functions should check for the static func as well. Subclasses > must override both _async() and _finish() because they could be using GTask > instead of GSimpleAsyncResult for example. Sure thing subclasses can override both, but I'd rather leave it non-mandatory. In fact I'm just overriding the _async() functions to try to fetch data from UOA before falling back to the base implementation.
Oh, I forgot to point out that what I've reuploaded is the updated patch stack after fixing the missing unrefs and nothing else. Xavier, do you think that my comments about the async tags make sense? Why do you think that using the pointer to the overridden method function is a bad idea?
I'm taking over this bug from Emanuele.
Created attachment 244010 [details] [review] keyring: Use virtual functions as source tags for GSimpleAsyncResult
Created attachment 244011 [details] [review] keyring: Split BaseKeyring and move the UOA support out of it
Created attachment 244012 [details] [review] keyring: Encapsulate virtual functions in public accessors
Created attachment 244013 [details] [review] account-settings: Take a BaseKeyring as construction parameter Instead of relying on the global EmpathyKeyring singleton, accept a BaseKeyring at instantiation time such that AccountSettings does no longer rely on anything provided by EmpathyKeyring.
http://cgit.collabora.com/git/user/bari/empathy.git/log/?h=keyring is the branch with this code.
About the keyring branch: I like having it a real singleton object, however the code split is useless because we'll always need an instance that speak to all supported backends in the empathy-auth-client process. When built in GOA it will just have UOA code disabled. So what I would do is simply: 1) create EmpathyKeyring singleton object 2) add an "EmpathyKeyring *self" arg to all methods 3) move that to the submodule
I had to move more code to tp-aw and, with Emanuele split, the singleton stuff doesn't work any more. I agree with Xavier, I will just get rid of the EmpathyKeyring stuff and put everything in tpaw.