[Bug 71384] improve account storage GInterface

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=71384

--- Comment #7 from Xavier Claessens <xclaesse at gmail.com> ---
(In reply to comment #2)
> Do you have an Empathy branch that updates the GOA and UOA plugins?

Not yet, but I'll surely do UOA.

> I'm trying to set up a sufficiently full jhbuild-or-something, so I might be
> able to test the GOA plugin if you can't.

IIRC GOA had issues because when I wrote it I didn't fully understand the API,
like for example it does not queue signals until _ready() is called. Maybe you
can take a look at it to check if it's all fine?

> ::: mission-control-plugins/account-storage.c
> @@ +142,5 @@
> >    return FALSE;
> >  }
> >  
> > +static gchar *
> > +default_create (const McpAccountStorage *storage,
> 
> This would have been easier to review if you added the default
> implementations in a commit where you weren't deleting anything.

Right, I failed at doing proper git commits, sorry. When refactoring I see more
stuff to change on the way and I end up with a mess to commit. But now that you
reviewed it already, is it ok with you to keep it like that?

> @@ +502,5 @@
> >   * decline to store the setting.
> >   *
> >   * The plugin is not expected to write to its long term storage
> > + * at this point. It can expect Mission Control to call
> > + * mcp_account_storage_commit_one() after a short delay.
> 
> commit_one() was always mis-named, because it can either commit one or all;
> it had that name because it was added as the replacement for commit(), which
> could only commit all.
> 
> I'd prefer one of these setups:
> 
> 1) rename your commit_one() to commit(), keep the old commit_one() semantics
> where NULL means "all of them"
> 
> 2) never call commit_one(NULL) so commit() always means "commit everything"
> and commit_one() always means "commit one"

I did (1), patch will follow.

> While you're breaking API anyway, I would be very tempted to turn
> commit[_one]() into an async/finish pair (Bug #29563), even if all its call
> sites initially just call it with a no-op callback and hope it worked.

Let's keep that out-of-scope here, but indeed bug #29563 can follow once this
lands.

> ::: src/mcd-storage.c
> @@ +96,5 @@
> > +      g_free, (GDestroyNotify) g_variant_unref);
> > +  sa->escaped_parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
> > +      g_free, g_free);
> > +  sa->secrets = g_hash_table_new_full (g_str_hash, g_str_equal,
> > +      g_free, NULL);
> 
> Now that we have SASLAuthentication and no gnome-keyring, and we're breaking
> API anyway, I think it might be time to delete the whole
> is_secret/make_secret thing. (As a separate patch, of course.)

Yay, a lot of code to delete \o/ Done in a patch following.

> @@ +1600,2 @@
> >  
> > +      done = mcp_account_storage_set (sa->storage, ma, account, key, escaped);
> 
> If we're breaking plugin API, I think we can now delete
> mcp_account_storage_set(), and say that set_attribute() and set_parameter()
> are mandatory. \o/
>
> (Which means we can drop the 'escaped' parameter to this function, and
> replace (escaped == NULL) with (variant == NULL).)

done

> @@ +2285,5 @@
> > +  g_hash_table_insert (self->accounts, g_strdup (account),
> > +      mcd_storage_account_new (plugin));
> > +
> > +  /* This will fill our parameters/attributes tables */
> > +  mcp_account_storage_get (plugin, MCP_ACCOUNT_MANAGER (self), account, NULL);
> 
> This calling convention is also pretty insane - McdStorageAccount should be
> a simple "view" of the plugin, and not have any storage of its own - but I
> think fixing that is out of scope right now. I might do a follow-up branch
> to do that before 5.17, though.

please open a new bug for that idea.

> ::: tests/twisted/mcp-account-diversion.c
> @@ +207,5 @@
> >  
> >  static gboolean
> > +_commit_one (const McpAccountStorage *self,
> > +    const McpAccountManager *am,
> > +    const gchar *account_name)
> 
> Deserves a comment "in this naive implementation, we always commit all
> accounts together" or something

done

-- 
You are receiving this mail because:
You are the QA Contact for the bug.