Context:

<xclaesse> smcv, Hmm, I have an issue with MC storage plugins: the -default plugin writes a .account for accounts that comes from -ring plugin
<xclaesse> then when I restart MC, the -default storage see that .account and create the account before -ring plugin
<smcv> xclaesse: patches to make the account API more competent welcome
<smcv> xclaesse: the underlying bug is https://bugs.freedesktop.org/show_bug.cgi?id=27727
<xclaesse> smcv, before making patches, do you agree that if an account comes from a plugin, the -default should not store it, right?
<smcv> xclaesse: yes, and while we're breaking plugin ABI is a great time to implement that
<xclaesse> smcv, if I understand correctly, mcd_storage_commit() does a commit in all storages plugins, and each storage plugin is supposed to know if it is repsonsible for that account, and do nothing if they aren't, right?
<smcv> xclaesse: yes
<xclaesse> ah, I understand
<smcv> xclaesse: however, feel free to change the system to
: MC remembers which single plugin is responsible for each account, and only commits to that one
<xclaesse> I've set MCP_ACCOUNT_STORAGE_PLUGIN_PRIO_READONLY on the ring plugin
<xclaesse> but that's lower than MCP_ACCOUNT_STORAGE_PLUGIN_PRIO_DEFAULT
<smcv> ah, the priority system is probably a trap
<xclaesse> I should rather make -ring priority higher and just claim that it commited
<smcv> it was designed when "accounts get split between separate account and keyring backends" was thought to be a feature
<smcv> each account being in exactly one backend would be a more sensible layout
<smcv> MC should create new accounts by going through backends in descending order and saying "can you?"
<smcv> and for everything else it should save each account in the single backend that it knows owns that one
<xclaesse> foreach (storage) if (mcp_account_storage_owns(storage, account)) {mcp_account_storage_commit(storage, account); break;}
<xclaesse> smcv, that's the correct way to do it now? ^
<smcv> looks sane
<smcv> might involve patching ring's and empathy's plugins so they actually implement _owns
<xclaesse> so the priority's only purpose now should be to pick a storage to create accounts, right?
<smcv> yeah, I think so
<xclaesse> ok, I'll check if I can cook a patch
<smcv> hmm, in fact, _owns might not even be necessary, if McdStorage stores "this account belongs to" in each account
<smcv> in general, any simplification here is good
<xclaesse> hash table account-name -> storage
<xclaesse> makes sense
<xclaesse> if we have that, why do we have owns at all ?
<smcv> well, we already have a hash table account name -> McdStorageAccount
<smcv> but McdStorageAccount doesn't point to the plugin, because bees
<smcv> or something
<smcv> oh yeah I remember
<smcv> it's for backwards compat
<smcv> search for "FIXME: This is rather subtle, and relies on the fact that accounts"
<smcv> it's because create() didn't originally exist
<smcv> xclaesse: ^ but now that we've broken API, we can just say "in the new API, if you don't implement create, MC will never create accounts in your plugin, deal with it"
<smcv> you might want to review https://bugs.freedesktop.org/show_bug.cgi?id=71230
<xclaesse> smcv, ok, I'll check that, so if I add the storage in that struct, I can remove iface->owns altogether, right?
<smcv> I think so

Comment on attachment 88886 [details] [review]
Simplify a bit storage API

Review of attachment 88886 [details] [review]:
-----------------------------------------------------------------

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

Reviewing your version of the -ring plugin (Cc John since most of this is probably how it already works):

> G_DEFINE_TYPE_WITH_CODE (McpAccountManagerRing, mcp_account_manager_ring,

This should be called RingAccountStorage or something. It implements McpAccountStorage, not McpAccountManager; and it isn't part of libmission-control-plugins, so it shouldn't have a Mcp prefix.

>   account_name = mcp_account_manager_get_unique_name (self->priv->am,
>       "ring", "tel", "account");

This means that if you have more than one modem, the accounts' names are assigned at random, making the account object path uncorrelatable with anything useful.

Does oFono let you attach arbitrary data to modems? If it does, you could tag the modems with their object path tails and display names.

I realize this is necessary if you want the first account to be ring/tel/account0 for compatibility or something.

>   PARAM ("always_dispatch", "true");

John: do you need this, or is it cargo-cult? I never liked this special case, so if we can get rid of it, I'd be in favour; or if you need one of its (possibly several) effects, I'd like to pin down which one(s) you need, and document why.

>       g_variant_get (parameters, "(&o@a{sv})", &path, NULL);
>       add_modem (self, path);

Assuming the a{sv} contains properties, it'd be nice to be able to pass it to add_modem().


>   /* It does not use self->priv->proxy_manager here because self could have been
> * disposed, in which case we'll get a cancelled error. */
>   proxy = g_dbus_proxy_new_finish (result, &error);

!!!

Passing around objects that might have been disposed: generally bad news. I would recommend either making the user_data a TpWeakRef or GWeakRef or something (to make the "is it dead?" explicit), or taking a ref when you start the call and unreffing it in the callback.

>   return TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_PARAMETERS |
>       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_ENABLED |
>       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_PRESENCE |
>       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_SERVICE;

I think this suggests how we can supersede McdAccount::always-on:

* if we CANNOT_SET_ENABLED, then Enabled is forcibly set to what the
  storage backend says it is (instead of TRUE, as always-on does)

* if we CANNOT_SET_PRESENCE, then AutomaticPresence and ConnectAutomatically
  are forcibly set to what the storage backend says they are,
  and RequestedPresence is forcibly set to
  (ConnectAutomatically ? AutomaticPresence : (OFFLINE, "offline", ""))

(In reply to comment #3)
> >   account_name = mcp_account_manager_get_unique_name (self->priv->am,
> >       "ring", "tel", "account");
> 
> This means that if you have more than one modem, the accounts' names are
> assigned at random, making the account object path uncorrelatable with
> anything useful.
> 
> Does oFono let you attach arbitrary data to modems? If it does, you could
> tag the modems with their object path tails and display names.
> 
> I realize this is necessary if you want the first account to be
> ring/tel/account0 for compatibility or something.

Reasonable point. Don't worry about the compatibility case; nemo is the only one that cares, and we can fix all of that software to handle it properly, or patch the name back to what it was. Ofono at least provides object paths for modems, which could probably map easily to account names.

> >   PARAM ("always_dispatch", "true");
> 
> John: do you need this, or is it cargo-cult? I never liked this special
> case, so if we can get rid of it, I'd be in favour; or if you need one of
> its (possibly several) effects, I'd like to pin down which one(s) you need,
> and document why.

Yes. I use the connectivity management in MC, and the ring account must always be connected regardless of connectivity. From a quick glance, the name is bad, but I don't see that it has any other effects?

> >   return TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_PARAMETERS |
> >       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_ENABLED |
> >       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_PRESENCE |
> >       TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_SERVICE;
> 
> I think this suggests how we can supersede McdAccount::always-on:
> 
> * if we CANNOT_SET_ENABLED, then Enabled is forcibly set to what the
>   storage backend says it is (instead of TRUE, as always-on does)
> 
> * if we CANNOT_SET_PRESENCE, then AutomaticPresence and ConnectAutomatically
>   are forcibly set to what the storage backend says they are,
>   and RequestedPresence is forcibly set to
>   (ConnectAutomatically ? AutomaticPresence : (OFFLINE, "offline", ""))

If the behavior change isn't a problem elsewhere, that would be a good solution.

(In reply to comment #4)
> > Does oFono let you attach arbitrary data to modems? If it does, you could
> > tag the modems with their object path tails and display names.
> > 
> > I realize this is necessary if you want the first account to be
> > ring/tel/account0 for compatibility or something.
> 
> Reasonable point. Don't worry about the compatibility case; nemo is the only
> one that cares, and we can fix all of that software to handle it properly,
> or patch the name back to what it was. Ofono at least provides object paths
> for modems, which could probably map easily to account names.

Excellent. If oFono's modem object paths have the same properties as Telepathy account paths (adding a new one "usually" produces a new unique path; keeping an old one "usually" keeps its old path) then we can use those; if not, perhaps we can use some properties of the modem, like the name of the hardware or something?

> I use the connectivity management in MC, and the ring account must
> always be connected regardless of connectivity. From a quick glance, the
> name is bad, but I don't see that it has any other effects?

I opened Bug #71389 to discuss this.

(In reply to comment #4)
> (In reply to comment #3)
> > I think this suggests how we can supersede McdAccount::always-on:
> 
> If the behavior change isn't a problem elsewhere, that would be a good
> solution.

Bug #71390

(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

Branch updated at http://cgit.collabora.com/git/user/xclaesse/telepathy-mission-control.git/log/?h=account-storage

(In reply to comment #3)
> Reviewing your version of the -ring plugin (Cc John since most of this is
> probably how it already works):
> 
> > G_DEFINE_TYPE_WITH_CODE (McpAccountManagerRing, mcp_account_manager_ring,
> 
> This should be called RingAccountStorage or something. It implements
> McpAccountStorage, not McpAccountManager; and it isn't part of
> libmission-control-plugins, so it shouldn't have a Mcp prefix.

GOA and UOA plugins does the same, but I agree it should be fixed.

Not done yet.

> >   account_name = mcp_account_manager_get_unique_name (self->priv->am,
> >       "ring", "tel", "account");
> 
> I realize this is necessary if you want the first account to be
> ring/tel/account0 for compatibility or something.

Indeed I did that for backward compatibility because Jolla harcode ring/tel/account0 in some places (Nokia did the same) but we could fix that upstream and jolla can trivially patch back their package if needed.

Not done yet.

> >   PARAM ("always_dispatch", "true");

Removed, MC now does the right thing.

> >   /* It does not use self->priv->proxy_manager here because self could have been
> > * disposed, in which case we'll get a cancelled error. */
> >   proxy = g_dbus_proxy_new_finish (result, &error);
> 
> !!!
> 
> Passing around objects that might have been disposed: generally bad news. I
> would recommend either making the user_data a TpWeakRef or GWeakRef or
> something (to make the "is it dead?" explicit), or taking a ref when you
> start the call and unreffing it in the callback.

Fixed by taking a strong ref.

I did lots of other changes in my branch. The account is enabled iff the modem is powered.

The tp-ring branch: http://cgit.collabora.com/git/user/xclaesse/telepathy-ring.git/log/?h=multi-modem

(In reply to comment #9)
> (In reply to comment #3)
> > >   account_name = mcp_account_manager_get_unique_name (self->priv->am,
> > >       "ring", "tel", "account");
> > 
> > I realize this is necessary if you want the first account to be
> > ring/tel/account0 for compatibility or something.
> 
> Indeed I did that for backward compatibility because Jolla harcode
> ring/tel/account0 in some places (Nokia did the same) but we could fix that
> upstream and jolla can trivially patch back their package if needed.
> 
> Not done yet.

Done now.

(In reply to comment #7)
> > 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?

Yes, it's OK now, but be careful in future :-)

(I often rebase refactoring branches several times before publishing.)

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

OK.

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

Sure.

> _set_attribute() and _set_parameter() are not mandatory for writable storage
> plugins.

not -> now

(Please be careful about that typo, it reverses the meaning of the sentence...)

"Remove mcp_account_storage_set()"

>   g_hash_table_remove (sa->parameters, parameter);
> - g_hash_table_remove (sa->escaped_parameters, parameter);

No, I think we still need that removal. If a parameter is stored keyfile-escaped in an existing account, and we change its value (including "to the same value but with a known type this time"), we should delete the keyfile-escaped form.

For instance, we might go from

{
  ...
  Parameters = <@a{sv} {}>,
  EscapedParameters = <@a{ss} {'account': 'me@example.com', 'port': '42', 'require-ssl': '1'}>,
}

to

{
  ...
  Parameters = <@a{sv} {'account': <'me@example.com'>, 'port': <uint32 42>, 'require-ssl': <True>}>,
  EscapedParameters = <@a{ss} {}>,
}

+static gboolean
+_set_parameter (McpAccountStorage *self,
+ McpAccountManager *am,
+ const gchar *account,
+ const gchar *parameter,
+ GVariant *val,
+ McpParameterFlags flags)
+{
+ return _set (self, am, account, parameter, val, flags);
+}

To be general and backwards-compatible, this needs to use g_strdup_printf ("param-%s", parameter) as the key.

MC account-storage branch looks good other than that.

> + PARAM ("param-modem", path);

Use the modem's object path as the StorageIdentifier, maybe?

+ g_hash_table_insert (modem->attributes,
+ "org.freedesktop.Telepathy.Account.Interface.Addressing.URISchemes",
+ g_variant_ref_sink (g_variant_new_strv (&uri_scheme, 1)));

My understanding had been that URISchemes is for "secondary" URI schemes, and that a telephony UI should use this pseudocode:

    if account.protocol_name == "tel" or account.use_for("tel"):
        add_button("Call using %s" % account, account)

Otherwise, a telephony account without "tel" in URISchemes would be completely useless...

+ variant = g_hash_table_lookup (modem->properties, "Powered");
+ if (variant != NULL)
+ return g_variant_get_boolean (variant);

Don't be remotely crashable:

if (variant != NULL && g_variant_classify (variant) == G_VARIANT_CLASS_BOOLEAN)

or equivalent.

Or you could use tp_vardict_get_boolean().

+ if (g_str_equal (property, "Powered"))
+ {
+ mcp_account_storage_emit_toggled ((McpAccountStorage *) self,
+ modem->account_name, g_variant_get_boolean (value));
+ }

Likewise.

(In reply to comment #16)
> > + PARAM ("param-modem", path);
> 
> Use the modem's object path as the StorageIdentifier, maybe?

done.

> + g_hash_table_insert (modem->attributes,
> + "org.freedesktop.Telepathy.Account.Interface.Addressing.URISchemes",
> + g_variant_ref_sink (g_variant_new_strv (&uri_scheme, 1)));
> 
> My understanding had been that URISchemes is for "secondary" URI schemes,
> and that a telephony UI should use this pseudocode:
> 
>     if account.protocol_name == "tel" or account.use_for("tel"):
>         add_button("Call using %s" % account, account)
> 
> Otherwise, a telephony account without "tel" in URISchemes would be
> completely useless...

ok, removed it.

> + variant = g_hash_table_lookup (modem->properties, "Powered");
> + if (variant != NULL)
> + return g_variant_get_boolean (variant);
> 
> Don't be remotely crashable:
> 
> if (variant != NULL && g_variant_classify (variant) ==
> G_VARIANT_CLASS_BOOLEAN)
> 
> or equivalent.
> 
> Or you could use tp_vardict_get_boolean().
> 
> + if (g_str_equal (property, "Powered"))
> + {
> + mcp_account_storage_emit_toggled ((McpAccountStorage *) self,
> + modem->account_name, g_variant_get_boolean (value));
> + }
> 
> Likewise.

It needs to be for for all g_variant_get then for being really pedantic. Done.

Sorry, I reverted your MC patches, because the tests now fail, and I wanted to get through the simpler changes before dealing with things like whether it's OK to delete an account that was already deleted.

There's a long patch series on <https://bugs.freedesktop.org/show_bug.cgi?id=27727>. I think it covers everything I reverted.

(In reply to comment #0)
> This removes mcp_account_storage_commit() because it is redundant with
> mcp_account_storage_commit_one (plugin, am, NULL);

This happened.

> This removes mcp_account_storage_owns() because an account is now
> owned by one and only one storage plugin and MC now keeps track of
> which storage plugin each account comes from.

So did this.

> Finally this adds default implementation on most iface methods to
> make read-only plugins easier to implement. Only _get() and _list()
> and mandatory.

This expanded a bit, because I wanted to make the plugin API more sensible while we were breaking API anyway. Mandatory methods are now:

* list
* get_attribute
* get_parameter
* list_typed_parameters
* list_untyped_parameters

and optional ones:

* delete_async, delete_finish
* commit
* get_identifier
* get_additional_info
* get_restrictions
* create
* set_attribute
* set_parameter
* get_flags

(In reply to comment #7)
> (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'd be happy to review this, but I can't test it.

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

It wasn't, except maybe in practice it was, because I don't think we actually re-entered the main loop between list() and ready()...

... but in any case, I decided ready was stupid, and removed the need for it (Bug #74581). So now the GOA plugin is correct :-)

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

I don't think we need to do this as part of the API break. We can easily make the default implementation of commit_async() be "just call commit()". Both of the callers of mcp_account_storage_commit() ignore what it returns anyway, and mcd_storage_commit() returns void :'-(

> > This calling convention is also pretty insane - McdStorageAccount should be
> > a simple "view" of the plugin, and not have any storage of its own

I fixed this.

> Sorry, I reverted your MC patches, because the tests now fail
...
> There's a long patch series on
> <https://bugs.freedesktop.org/show_bug.cgi?id=27727>. I think it covers
> everything I reverted.

All merged.

The things I would be tempted to change before releasing this API as 5.17.0 are:

* If get_attribute, get_parameter return NULL, should they raise a GError?
  There are two reasons they can fail: either that key isn't stored,
  or that key's stored value is not something they can coerce into @type.
  I think perhaps they should raise a GError: "not stored" should not
  invalidate an account (unless the parameter is mandatory) but "cannot coerce"
  maybe should?

* The same for set_attribute, set_parameter. I think these should be
  documented to raise an error of the caller's choice, typically a TP_ERROR.

* commit should be async, but it's easy to add a commit_async() later,
  with a default implementation in terms of commit(), so we can skip this
  if it turns out to be Hard™.

* Either list should be async, or there should be some sort of async-init
  that happens first; but in the worst case, plugins can implement list()
  as "start the async list process, return NULL, and when we get
  results back, signal them as new accounts" like the GOA plugin does.
  Unless we consider it to be important for callers to be able to know
  when all plugins are ready and we have as many accounts as we're going
  to get?

(In reply to comment #21)
> * If get_attribute, get_parameter return NULL, should they raise a GError?

We can always add a get_attribute_failable later, if we really need to, so this isn't really a blocker.

> * The same for set_attribute, set_parameter. I think these should be
>   documented to raise an error of the caller's choice, typically a TP_ERROR.

mcd_storage_set_(attribute|parameter) currently just return a boolean for "did it change?", so... whatever. Again, if necessary, we can add a parallel thing later.

> * Either list should be async, or there should be some sort of async-init
>   that happens first

Wasn't actually very hard.

Created attachment 93543 [details] [review]
If account storage plugins are GAsyncInitable, initialize  them before list()

This means they don't have to be prepared to block during list().

(In reply to comment #23)
> If account storage plugins are GAsyncInitable, initialize  them before list()

In principle the TestDBusAccountPlugin is not actually quite right here - its init_async should be idempotent (first call does the actual initialization, subsequent calls while initializing wait for the first call and then have the same result that the first call did, subsequent calls while already initialized return the same result again).

I can't help thinking there should be some sort of mixin for this.

Created attachment 93548 [details] [review]
TestDBusAccountPlugin: remove leftover code from events  queue

Created attachment 93549 [details] [review]
Let account storage plugins load asynchronously

This means they don't have to be prepared to block during list().

(tests/twisted/mcp-account-diversion.c still loads its data at the last
possible moment, in list(), to confirm that the no-op default implementation
of list_async and list_finish works as intended.)

---

Life's too short to implement and test idempotent g_asyncable_init_async() for this... and if we provide our own method here, we can have MC guarantee that it will only call it once.

Comment on attachment 93548 [details] [review]
TestDBusAccountPlugin: remove leftover code from events  queue

Review of attachment 93548 [details] [review]:
-----------------------------------------------------------------

++

Comment on attachment 93549 [details] [review]
Let account storage plugins load asynchronously

Review of attachment 93549 [details] [review]:
-----------------------------------------------------------------

::: src/mcd-storage.c
@@ +2386,5 @@
> +    gpointer user_data)
> +{
> +  GList *store;
> +  GTask *task = g_task_new (self, NULL, callback, user_data);
> +  gsize remaining = 1;

I'd add a comment explaining why you start at '1' instead of 0, it's not that clear to me.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-mission-control/issues/75.