GNOME Bugzilla – Bug 776906
Miscellaneous fixes and improvements to add_temporary_account
Last modified: 2017-01-10 14:35:39 UTC
I don't like how we use a callback with the GTask (earlier GSimpleAsyncResult) in add_temporary_account. add_temporary_account is a fire-and-forget function, and the caller doesn't pass any callback. It contains a chain of asynchronous calls and we need a way to carry the context (ie. the GoaIdentityService and the GoaIdentity) across those calls. There are so many ways to do it: (a) a struct (b) g_object_set_data on a GObject (c) abusing GSimpleAsyncResult:source_tag, or (d) GTask:task_data Attaching a separate callback to the GTask to simply to pass some context seems needlessly convoluted. I also came across a few minor issues.
Created attachment 342970 [details] [review] identity: Consolidate the exit path
Created attachment 342971 [details] [review] identity: Fix typo in add_temporary_account
Created attachment 342972 [details] [review] identity: Simplify the add_temporary_account code
Created attachment 342973 [details] [review] identity: Hold a ref on the identity while adding a temporary account
Review of attachment 342970 [details] [review]: ok
Review of attachment 342971 [details] [review]: good clean up/fix.
Review of attachment 342972 [details] [review]: i actually think the old way is clearer, but it's subjective, so whatever. if you like this better I don't care if you push it. ::: src/goaidentity/goaidentityservice.c @@ +780,1 @@ on_account_added (GoaManager *manager, if you're going to make this function specific to temporary accounts (which is fine, since that's all it's currently used for), then I'd rename it to have temporary somewhere in its name.
Review of attachment 342973 [details] [review]: looks right
Comment on attachment 342970 [details] [review] identity: Consolidate the exit path Pushed to master.
Comment on attachment 342971 [details] [review] identity: Fix typo in add_temporary_account Pushed to master.
(In reply to Ray Strode [halfline] from comment #7) > Review of attachment 342972 [details] [review] [review]: > > i actually think the old way is clearer, but it's subjective, so whatever. > if you like this better I don't care if you push it. Ok. :) > ::: src/goaidentity/goaidentityservice.c > @@ +780,1 @@ > on_account_added (GoaManager *manager, > > if you're going to make this function specific to temporary accounts (which > is fine, since that's all it's currently used for), then I'd rename it to > have temporary somewhere in its name. Ok, I will do that.
Created attachment 343015 [details] [review] identity: Rename on_account_added to on_temporary_account_added