[Bug 71384] improve account storage GInterface

[bugzilla-daemon at freedesktop.org]

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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |john.brooks at jollamobile.com

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
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 at 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", ""))

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