[Bug 71384] improve account storage GInterface

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Comment on attachment 88886
  --> https://bugs.freedesktop.org/attachment.cgi?id=88886
Simplify a bit storage API

Review of attachment 88886:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=71384&attachment=88886)
-----------------------------------------------------------------

Do you have an Empathy branch that updates the GOA and UOA plugins?

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.

::: 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.

@@ +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"

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.

::: 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.)

@@ +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).)

@@ +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.

::: 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

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