[Bug 52231] Storage plugins should be able to create new accounts

[bugzilla-daemon at freedesktop.org]

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

--- Comment #8 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-07-19 16:05:32 PDT ---
> + if (iface->create == NULL)
> + {
> + g_set_error (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
> + "This storage does not implement create function");
> + return NULL;
> + }

This can break existing plugins. You can tell by the fact that you had to
change the default pseudo-plugin to accommodate it...

In Comment #3 I suggested giving McpAccountStorage a default implementation of
->create (set up in the interface's base_init) which calls
mcp_account_manager_get_unique_name, and tries to set the manager and protocol
keys. Then you could move responsibility for setting those keys into ->create,
removing it from _mcd_account_manager_create_account.

In existing plugins, this will work if and only if the "set" method works, i.e.
if and only if the plugin was willing to make the account exist - that seems
exactly what we want. In particular, you won't need any changes to the default
pseudo-plugin any more.

Conveniently, it also means ->create will never be NULL.

> + * Create an account in long term storage.

Nothing is created in long term storage until you commit(), which the default
implementation doesn't. Please document whether implementations are expected to
commit changes to long-term storage (by inspection, "no").

The more I read the McpAccountStorage interface, the more I want to replace it
with something nicer... but it'll have to do for now.

(In reply to comment #1)
>   it should be documented/specified [whether] after a
>   successful call, the "list" vfunc will include that account

It turns out that the "list" vfunc is documented to only be called once, during
startup, so this never becomes significant. (Yay, non-state-recoverable
interfaces.)

>   it should be documented/specified whether a call to "commit"
>   is implied

Still needs doing. Also, please document whether the "created" signal is
emitted (by inspection: "no").

> I'd like a regression test, ideally via
> a test plugin that does something vaguely realistic.

Still true.

(In reply to comment #7)
> Not the reason why GOA have names like "goa_<internal id>" is because we can't
> save settings on a GOA account.

I think that's a GOA design flaw, but presumably one we're stuck with.

> > Identifiers containing a double-underscore are currently safe, so you could use
> > goa__234. Or, implement mcp_account_manager_account_exists (you can factor it
> > out of get_unique_name) and document that plugins that generate their own IDs
> > are responsible for using that to avoid collisions.
> 
> I think you're looking too far for the name collision, if the user wants to
> shoot himself he has easier ways than that :-) 

I suppose my point is: using prefixes for namespacing is only useful if
*everyone* uses prefixes for namespacing. Right now, MC doesn't. I suppose we
could make it use a prefix for new accounts in future, like
mc_smcv_40example_2ecom0 for an account "smcv at example.com", if necessary...

> But indeed I didn't though about it when writing the GOA plugin, and IMO it's
> not worth changing names and breaking all stored logs.

If the GOA plugin has to remain buggy to keep stored logs valid, that might be
a necessary evil, but in that case, please make sure to document a way that new
plugins (that don't have the stored-logs problem) can guarantee not to collide,
and use it in any new plugins (including the one in the regression test).

A prefix containing double-underscore would be sufficient.

In Telepathy 1.0, perhaps we should relax the definition of an account "unique
name" (object-path tail) from "exactly 3 components" to "at least 3
components". Then MC's own storage could use gabble/jabber/smcv_40example_2ecom
and the GOA plugin could use gabble/jabber/goa/_123 or something.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.