Branch implementing this support:

http://git.collabora.co.uk/?p=user/danni/telepathy-mission-control.git;a=shortlog;h=refs/heads/external-account-support

This branch does not use the External property to enforce any policy, i.e. calling 'Set' does not prevent the property from being set. This is currently left up to the plugins to handle. The rationale behind this is that some properties may still be settable (e.g. Enabled). An (ass) containing mutable property names could possibly be added as another property, so that user interfaces know what properties to still display, but this significantly increases the complexity for user interfaces.

(In reply to comment #1)
> This branch does not use the External property to enforce any policy, i.e.
> calling 'Set' does not prevent the property from being set. This is currently
> left up to the plugins to handle.

See also the "always-on" property on McdAccount, which has the following semantics if TRUE:

* cannot be disabled
* ConnectAutomatically cannot be false
* RequestedPresence cannot be offline

(This is exactly what you want for the singleton cellular account on an N900, which is brought into existence by osso-mission-control.)

In practice, I don't think External's effects are going to be the same for all accounts it's applied to:

* a Facebook Chat (XMPP) or other social-network-attached account can still be put online/offline and probably even disabled (the UI widget would ideally be labelled "disable Facebook Chat" to indicate that it only affects MC); it doesn't make sense to change its 'account' or 'password' parameters, but other Parameters (proxy server?) are likely to make sense to change

* a cellular account probably can't be taken offline or disabled, but its Parameters (e.g. SMS centre and SMS validity time on N900) can be altered freely

Perhaps we want a read-only A.I.External.AlwaysOn property to indicate the "always-on" semantics?

(In reply to comment #1)
> An (ass) containing mutable
> property names could possibly be added as another property, so that user
> interfaces know what properties to still display, but this significantly
> increases the complexity for user interfaces.

If we go this way then I think Parameters should be a special case, and have its own list.

When proposing Account API please be completely clear about the difference between Parameters (which is itself a property) and properties :-)

Here is a new API + an implementation. Ready for review!

http://git.collabora.co.uk/?p=user/eitan/telepathy-mission-control.git;a=shortlog;h=refs/heads/account-storage

> Added mcd_account_manager_get_storage_plugin and have each account borrow...

The accounts should take and release a real reference, on general principles. Let's not add any more hidden assumptions about objects' life-cycles.

> +++ b/mission-control-plugins/account-storage.h
...
> +#include <libmcclient/mc-enums.h>

libmcclient is going away. Don't use it for anything.

For the moment, just replace the mcStorageRestrictionFlags with a guint. I don't think we should be merging this branch until the spec is declared stable and present in telepathy-glib, anyway; the spec isn't very elaborate and you've just done an implementation, so that shouldn't take too long to happen.

> +#define PLUGIN_PROVIDER "org.freedesktop.Telepathy.Account.DefaultStorage"

I think this should be either "org.freedesktop.Telepathy.MissionControl5.DefaultStorage" or the empty string.

@@ -101,6 +101,8 @@ struct _McpAccountStorageIface
> +  const gchar *provider;
> +  mcStorageRestrictionFlags restrictions;

Perhaps restrictions should be a get_restrictions() method, so you can have accounts of varying restrictedness within one plugin?

> +GValue *
> +mcp_account_storage_get_identifier (const McpAccountStorage *storage,
> +    const gchar *account)

As implemented, all existing storage implementations will segfault (NULL pointer dereference) when this method is called. If iface->get_identifier is NULL, you need to do some default implementation; otherwise it's an ABI break.

I think the default implementation should be to return @account, and the default (keyfile + gnome-keyring) plugin shouldn't override that default implementation.

As implemented, this method needs to document how the returned GValue is allocated; saying "slice-allocated as if via tp_g_value_slice_new()" would be OK. However, that'll be problematic if/when we do language bindings for Mcp (the GType for GValue is g_malloc'd, not slice-allocated). You can sidestep the issue by changing the signature to be:

void mcp_account_storage_get_identifier (const McpAccountStorage *, const gchar *, GValue *)

and documenting that the GValue is initially zero-filled. This makes some code later on work better, too!

> +/**
> + * mcp_account_storage_get_additional_info:

Again, if iface->get_additional_info is NULL, you need to do something sensible. Returning an empty hash table would make sense.

It would also be helpful to plugin authors if you catch iface->get_additional_info *returning* NULL, and turn *that* into an empty hash table as well.

Again, the default plugin should leave this unset, and get an empty hash table as a result.

The ownership of the returned hash table should be documented: I think it's intended to be "caller unrefs"?

> +const gchar *
> +mcp_account_storage_provider (const McpAccountStorage *storage)

This needs to do something sane for NULL, too. I'd suggest returning either the empty string, or G_OBJECT_TYPE_NAME (storage).

> +/**
> + * mcp_account_storage_restrictions:

Again, think about what happens if this is unimplemented; 0 is probably the right answer anyway, though.

> +void mcp_account_storage_iface_implement_get_identifier (
> +void mcp_account_storage_iface_implement_get_additional_info (

void \n ... for definitions, please. Ideally, these should both be documented.

In the SSO plugin:

> -    mission-control-plugins \
>      libmcclient \
> +    mission-control-plugins \

In principle this should have been part of the previous commit rather than the one it appears in, but in any case, don't do that. mcp appears before libmcclient partially so that people won't be tempted to make mcp depend on libmcclient :-P

> +static GHashTable *
> +_get_additional_info (const McpAccountStorage *self,
> +    const gchar *account)
> +{
> +  AgAccountId account_id = 0;

It's not clear to me that *all* the account info from libaccounts should be echoed onto D-Bus; it'd be better to have some actually useful subset. I'd be inclined to leave this method unimplemented for the moment, and implement one later if needed?

In McdAccount:

> +static void
> +get_storage_specific_info (TpSvcDBusProperties *self,
> +    const gchar *name, GValue *value)

This leaks the GHashTable. Use g_value_take_boxed to transfer ownership.

> +static void
> +get_storage_identifier (TpSvcDBusProperties *self,
> +    const gchar *name, GValue *value)

This leaks @identifier. Also, you're doing the type wrong: a D-Bus property of type 'v' is a GValue containing a GValue. You should do something like this:

    GValue identifier = { 0 };

    mcp_account_storage_get_identifier (priv->storage_plugin, priv->unique_name, &identifier);
    g_value_init (value, G_TYPE_VALUE);
    g_value_set_boxed (value, &identifier);
    g_value_unset (&identifier);

Miscellaneous documentation nits
================================

> + * Get the storage-specific identifier for this account. The type is variant,
> + * hence the GValue.
> + *
> + * Returns: a #GValue that uniquely identifies the account.

This should say that the GValue can have any type accepted by dbus-glib (which is a subset of the type system). I'd have documented it as:

Return the storage-specific identifier for this account, typically a string or integer.

Returns: a #GValue whose type can be sent over D-Bus by dbus-glib

> + * Get additional fields and their values that are stored for this account
> + * that are not Telepathy parameters.
> + *
> + * Returns: a #GHashtable mapping of #gchar keys and #GValue values.

Return additional storage-specific information about this account, which is made available on D-Bus but not otherwise interpreted by Mission Control.

tp_asv_new() can be used to construct a suitable mapping.

Returns: a #GHashTable with string keys and #GValue values

> + * Returns: a const #gchar* : the plugin's provider, a DBus namespaced name for
> + * this plugin.

Saying "a const #gchar* :" here is not useful; the type already says that. Also, it's spelled D-Bus.

(In reply to comment #5)
> > Added mcd_account_manager_get_storage_plugin and have each account borrow...
> 
> The accounts should take and release a real reference, on general principles.
> Let's not add any more hidden assumptions about objects' life-cycles.

7eb53dc

> 
> > +++ b/mission-control-plugins/account-storage.h
> ...
> > +#include <libmcclient/mc-enums.h>
> 
> libmcclient is going away. Don't use it for anything.
> 
> For the moment, just replace the mcStorageRestrictionFlags with a guint. I
> don't think we should be merging this branch until the spec is declared stable
> and present in telepathy-glib, anyway; the spec isn't very elaborate and you've
> just done an implementation, so that shouldn't take too long to happen.

959fe5c

> 
> > +#define PLUGIN_PROVIDER "org.freedesktop.Telepathy.Account.DefaultStorage"
> 
> I think this should be either
> "org.freedesktop.Telepathy.MissionControl5.DefaultStorage" or the empty string.

An empty string, as I wrote in the spec :P
Removed all the new storage API from the default storage plugin, as I added fallbacks (see below).

> 
> @@ -101,6 +101,8 @@ struct _McpAccountStorageIface
> > +  const gchar *provider;
> > +  mcStorageRestrictionFlags restrictions;
> 
> Perhaps restrictions should be a get_restrictions() method, so you can have
> accounts of varying restrictedness within one plugin?
> 

3c71307

> > +GValue *
> > +mcp_account_storage_get_identifier (const McpAccountStorage *storage,
> > +    const gchar *account)
> 
> As implemented, all existing storage implementations will segfault (NULL
> pointer dereference) when this method is called. If iface->get_identifier is
> NULL, you need to do some default implementation; otherwise it's an ABI break.
> 
> I think the default implementation should be to return @account, and the
> default (keyfile + gnome-keyring) plugin shouldn't override that default
> implementation.
> 

1b989e7

> As implemented, this method needs to document how the returned GValue is
> allocated; saying "slice-allocated as if via tp_g_value_slice_new()" would be
> OK. However, that'll be problematic if/when we do language bindings for Mcp
> (the GType for GValue is g_malloc'd, not slice-allocated). You can sidestep the
> issue by changing the signature to be:
> 
> void mcp_account_storage_get_identifier (const McpAccountStorage *, const gchar
> *, GValue *)
> 
> and documenting that the GValue is initially zero-filled. This makes some code
> later on work better, too!

1b989e7

> 
> > +/**
> > + * mcp_account_storage_get_additional_info:
> 
> Again, if iface->get_additional_info is NULL, you need to do something
> sensible. Returning an empty hash table would make sense.
> 
> It would also be helpful to plugin authors if you catch
> iface->get_additional_info *returning* NULL, and turn *that* into an empty hash
> table as well.
> 
> Again, the default plugin should leave this unset, and get an empty hash table
> as a result.
> 
> The ownership of the returned hash table should be documented: I think it's
> intended to be "caller unrefs"?
> 

1b989e7

> > +const gchar *
> > +mcp_account_storage_provider (const McpAccountStorage *storage)
> 
> This needs to do something sane for NULL, too. I'd suggest returning either the
> empty string, or G_OBJECT_TYPE_NAME (storage).

1b989e7

> 
> > +/**
> > + * mcp_account_storage_restrictions:
> 
> Again, think about what happens if this is unimplemented; 0 is probably the
> right answer anyway, though.
> 

Made it a get_restrictions, with a 0 fallback (see above).

> > +void mcp_account_storage_iface_implement_get_identifier (
> > +void mcp_account_storage_iface_implement_get_additional_info (
> 
> void \n ... for definitions, please. Ideally, these should both be documented.
> 

Sry! Can't think straight now, hard to add docs, would be easier if the other *_implement_* ones had it.

40bdaf3

> In the SSO plugin:
> 
> > -    mission-control-plugins \
> >      libmcclient \
> > +    mission-control-plugins \
> 
> In principle this should have been part of the previous commit rather than the
> one it appears in, but in any case, don't do that. mcp appears before
> libmcclient partially so that people won't be tempted to make mcp depend on
> libmcclient :-P
> 

959fe5c

> > +static GHashTable *
> > +_get_additional_info (const McpAccountStorage *self,
> > +    const gchar *account)
> > +{
> > +  AgAccountId account_id = 0;
> 
> It's not clear to me that *all* the account info from libaccounts should be
> echoed onto D-Bus; it'd be better to have some actually useful subset. I'd be
> inclined to leave this method unimplemented for the moment, and implement one
> later if needed?
> 

Yes! We use it for getting the right SSO bits during a SASL sign on.
Made a hardcoded list for an interesting subset.

4944567

> In McdAccount:
> 
> > +static void
> > +get_storage_specific_info (TpSvcDBusProperties *self,
> > +    const gchar *name, GValue *value)
> 
> This leaks the GHashTable. Use g_value_take_boxed to transfer ownership.
> 

Oops
fab17a5

> > +static void
> > +get_storage_identifier (TpSvcDBusProperties *self,
> > +    const gchar *name, GValue *value)
> 
> This leaks @identifier. Also, you're doing the type wrong: a D-Bus property of
> type 'v' is a GValue containing a GValue. You should do something like this:
> 
>     GValue identifier = { 0 };
> 
>     mcp_account_storage_get_identifier (priv->storage_plugin,
> priv->unique_name, &identifier);
>     g_value_init (value, G_TYPE_VALUE);
>     g_value_set_boxed (value, &identifier);
>     g_value_unset (&identifier);
> 

1b989e7

> Miscellaneous documentation nits
> ================================
> 
> > + * Get the storage-specific identifier for this account. The type is variant,
> > + * hence the GValue.
> > + *
> > + * Returns: a #GValue that uniquely identifies the account.
> 
> This should say that the GValue can have any type accepted by dbus-glib (which
> is a subset of the type system). I'd have documented it as:
> 
> Return the storage-specific identifier for this account, typically a string or
> integer.
> 
> Returns: a #GValue whose type can be sent over D-Bus by dbus-glib
> 
> > + * Get additional fields and their values that are stored for this account
> > + * that are not Telepathy parameters.
> > + *
> > + * Returns: a #GHashtable mapping of #gchar keys and #GValue values.
> 
> Return additional storage-specific information about this account, which is
> made available on D-Bus but not otherwise interpreted by Mission Control.
> 
> tp_asv_new() can be used to construct a suitable mapping.
> 
> Returns: a #GHashTable with string keys and #GValue values
> 
> > + * Returns: a const #gchar* : the plugin's provider, a DBus namespaced name for
> > + * this plugin.
> 
> Saying "a const #gchar* :" here is not useful; the type already says that.
> Also, it's spelled D-Bus.

8175119

With a few more adjustments, this looks OK to merge when the spec lands in telepathy-glib.

> +    if (priv->storage_plugin)
> +      {
> +        g_object_unref (priv->storage_plugin);
> +        priv->storage_plugin = NULL;
> +      }

Non-merge-blocker: if you depend on telepathy-glib 0.11.7 (which I think current MC master already needs), you can replace this block with:

    tp_clear_object (&priv->storage_plugin);

> +    const gchar *account, GValue *identifier)

One arg per line in definitions, please.

>  mcp_account_storage_get_identifier (const McpAccountStorage *storage,
...
> +  g_return_if_fail (!G_IS_VALUE (identifier));

This will be happy with identifier == NULL, which is also an error. Add:

    g_return_if_fail (identifier != NULL);

> + * Returns: a caller owned #GHashtable mapping of #gchar keys and #GValue
> + * values.

"#GHashTable" (note capitalization)

> +  if (iface->get_additional_info != NULL)
> +    rv = iface->get_additional_info (storage, account);
> +
> +  if (iface->get_additional_info == NULL || rv == NULL)
> +    rv = g_hash_table_new (g_str_hash, g_str_equal);

You can simplify the second "if" to "if (rv == NULL)", since you initialize rv at the beginning. FYI we typically call the thing to be returned "ret", but "rv" is fine too.

> + *   mcp_account_storage_iface_implement_get_dentifier (iface,

"identifier"

The spec is a draft in master. Please canonize it!

(In reply to comment #7)
> With a few more adjustments, this looks OK to merge when the spec lands in
> telepathy-glib.
> 
> > +    if (priv->storage_plugin)
> > +      {
> > +        g_object_unref (priv->storage_plugin);
> > +        priv->storage_plugin = NULL;
> > +      }
> 
> Non-merge-blocker: if you depend on telepathy-glib 0.11.7 (which I think
> current MC master already needs), you can replace this block with:
> 
>     tp_clear_object (&priv->storage_plugin);
> 

a37accf

> > +    const gchar *account, GValue *identifier)
> 
> One arg per line in definitions, please.

8c45ea7

> 
> >  mcp_account_storage_get_identifier (const McpAccountStorage *storage,
> ...
> > +  g_return_if_fail (!G_IS_VALUE (identifier));
> 
> This will be happy with identifier == NULL, which is also an error. Add:
> 
>     g_return_if_fail (identifier != NULL);
> 

79702ef

> > + * Returns: a caller owned #GHashtable mapping of #gchar keys and #GValue
> > + * values.
> 
> "#GHashTable" (note capitalization)
> 

8c45ea7

> > +  if (iface->get_additional_info != NULL)
> > +    rv = iface->get_additional_info (storage, account);
> > +
> > +  if (iface->get_additional_info == NULL || rv == NULL)
> > +    rv = g_hash_table_new (g_str_hash, g_str_equal);
> 
> You can simplify the second "if" to "if (rv == NULL)", since you initialize rv
> at the beginning. FYI we typically call the thing to be returned "ret", but
> "rv" is fine too.
> 

0843f49

> > + *   mcp_account_storage_iface_implement_get_dentifier (iface,
> 
> "identifier"

8c45ea7

I think this looks good! Just taking a look at the spec draft, I agree that it's worth waiting on the spec being undrafted and in a released tp-glib before merging rather than needing to do that dance later.

I released the undrafted interface in spec 0.19.8; this is now blocking on tp-glib being updated.

(In reply to comment #10)
> I released the undrafted interface in spec 0.19.8; this is now blocking on
> tp-glib being updated.

tp-glib 0.11.9 has support for this interface.

I just push minus effed a heavily squashed branch that uses to tp-glib symbols. It's also rebased on a current master.

I'll try pinging on IRC tomorrow if it's not reviewed yet..

(In reply to comment #13)
> I'll try pinging on IRC tomorrow if it's not reviewed yet..

+#include "_gen/svc-Account_Interface_Storage.h"

This shouldn't be there any more.

-#include "mission-control-plugins/mission-control-plugins.h"

Intentional?


+guint
+mcp_account_storage_get_restrictions (const McpAccountStorage *storage,
+    const gchar *account)

Shouldn't this return TpStorageRestrictionFlags?


+static gboolean
+_find_account (McdAccountManagerSso *sso,
+    const gchar *account_name,
+    AgAccountId *account_id)

Wait what? Isn't there a better way to map backwards than iterating over every account? If not, :(

+#define PLUGIN_PROVIDER "org.maemo.Telepathy.Account.Storage.LibAccounts"

We'll want to define this somewhere that other applications can use, but I'm not sure where off-hand.

(In reply to comment #14)
> (In reply to comment #13)
> > I'll try pinging on IRC tomorrow if it's not reviewed yet..
> 
> +#include "_gen/svc-Account_Interface_Storage.h"
> 

e927755
> This shouldn't be there any more.
> 
> -#include "mission-control-plugins/mission-control-plugins.h"
> 
e927755

> Intentional?
> 
> 
> +guint
> +mcp_account_storage_get_restrictions (const McpAccountStorage *storage,
> +    const gchar *account)
> 
> Shouldn't this return TpStorageRestrictionFlags?
> 
e927755

> 
> +static gboolean
> +_find_account (McdAccountManagerSso *sso,
> +    const gchar *account_name,
> +    AgAccountId *account_id)
> 
> Wait what? Isn't there a better way to map backwards than iterating over every
> account? If not, :(
> 

Yeah, that is kind of silly. But bomb proof, wasn't familiar enough with the data model to have a hashtable mapping, afraid it would get stale. There is currently no private struct on that object either. Sorry! It is not a frequent function.

> +#define PLUGIN_PROVIDER "org.maemo.Telepathy.Account.Storage.LibAccounts"
> 
> We'll want to define this somewhere that other applications can use, but I'm
> not sure where off-hand.

Yeah, that might need to move soon.

ping!

ship it!

I had to make some changes because some tests were failing.
Apparently mcd_account_manager_get_storage_plugin can be called too early when the account is created, before it is written to the storage plugin.

This patch makes it reference the account storage plugin lazily when needed, and to gracefully fail when it can't retrieve it.

I tried to have it retrieve the storage plugin consistently at a later stage, but there is no way of knowing when the account is written to the storage plugin since there are multiple code paths. So this seemed like the safest choice.

431563f

This is merged and all done, righT?

(In reply to comment #19)
> This is merged and all done, righT?

(In reply to comment #18)
> I had to make some changes because some tests were failing.
[...]
> 431563f

This commit doesn't appear to exist. However, f9f788e31 looks like it might be a rebased equivalent of it, which was then cherry-picked as 8e70faef115. This exists in master, but not in any release yet.

Eitan, could you confirm that all changes you wanted are in fact present, then close this bug as fixed in git for 5.5.4? Thanks!

(-patch since there seems to be nothing to review right now)

This has all been merged (and redone since!).