[Bug 28192] support for immutable accounts in Mission Control

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-06-17 16:41:48 PDT ---
(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

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