[Bug 28192] support for immutable accounts in Mission Control

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-06-17 11:00:13 PDT ---
> 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.

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