Bug 30478

http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=shortlog;h=refs/heads/tp-account-storage

This currently implements access for the immutable properties.

There should also be a tp_account_get_storage_specific_information_async(), since the spec says that property should not be cached.

> There should also be a tp_account_get_storage_specific_information_async(),
> since the spec says that property should not be cached.

Also now done.

Generally looks good. Some minor things:

> +  if (self->priv->storage_provider != NULL &&
> +      strlen (self->priv->storage_provider) > 0)

That use of strlen is an inefficient way to spell (s->p->s[0] != '\0'). Even better, you could replace the entire condition with a call to the tp_str_empty() macro.

> +  self->priv->storage_provider = g_strdup (tp_asv_get_string (properties,
> +        "StorageProvider"));

I think I'd prefer it if you set s->p->s to "" if invalid/omitted/etc.

Is it useful to distinguish between "" and NULL at all? I can see two obvious alternatives:

* storage-provider is always a non-empty string or NULL
* storage-provider is always non-NULL but may be empty

This could either be enforced by the GetAll callback, or by the property getter and the "C binding".

> +   * TpAccount:storage-restrictions
> ... This value will be %NULL...

No, it won't. :-)

> + * tp_account_get_storage_specific_information_finish:

Depending how long this branch takes to merge, it might be worth going straight to a GVariant-based representation made with dbus_g_value_build_g_variant, rather than having to add a parallel API using GVariant as part of Bug #30422. If you do this, the GVariant-based representation should probably be a TpVariantMap from Bug #30423.

Perhaps the unique identifier could also usefully be represented as a GVariant?

(In reply to comment #3)
> Generally looks good. Some minor things:
> 
> > +  if (self->priv->storage_provider != NULL &&
> > +      strlen (self->priv->storage_provider) > 0)
> 
> That use of strlen is an inefficient way to spell (s->p->s[0] != '\0'). Even
> better, you could replace the entire condition with a call to the
> tp_str_empty() macro.

I was looking for that macro, but had believed it was called TP_STR_EMPTY. Fixed.

> > +  self->priv->storage_provider = g_strdup (tp_asv_get_string (properties,
> > +        "StorageProvider"));
> 
> I think I'd prefer it if you set s->p->s to "" if invalid/omitted/etc.

Fixed. I've also changed it to not be an error if the Storage interface can't be accessed, and instead act as if the account comes from MC.

> Is it useful to distinguish between "" and NULL at all? I can see two obvious
> alternatives:
> 
> * storage-provider is always a non-empty string or NULL
> * storage-provider is always non-NULL but may be empty

The behaviour now is NULL means unprepared; "" means prepared and no provider.

> > +   * TpAccount:storage-restrictions
> > ... This value will be %NULL...
> 
> No, it won't. :-)

Fixed.

(In reply to comment #4)

> Depending how long this branch takes to merge, it might be worth going 
> straight to a GVariant-based representation made with 
> dbus_g_value_build_g_variant, rather than having to add a parallel API using 
> GVariant as part of Bug #30422. If you do this, the GVariant-based 
> representation should probably be a TpVariantMap from Bug #30423.

> Perhaps the unique identifier could also usefully be represented as a 
> GVariant?

Both of these seem reasonable to me (I wondered about them at the time). When do you plan to merge that branch?

(In reply to comment #5)
> Both of these seem reasonable to me (I wondered about them at the time). When
> do you plan to merge that branch?

When someone reviews it, hint hint? :-)

(But seriously: I should probably also finish the inverse of dbus_g_value_build_g_variant before we start using TpVariantMap in APIs.)

>    if (error != NULL)
> -    {
> -      DEBUG ("Error getting Storage properties: %s", error->message);
> -      _tp_proxy_set_feature_prepared (proxy, TP_ACCOUNT_FEATURE_STORAGE,
> -          FALSE);
> -      return;
> -    }
> +    DEBUG ("Error getting Storage properties: %s", error->message);

Removing the early return looks wrong: properties will be NULL and all the accesses to it will (either g_critical() or) crash. Did you test this case?

> >    if (error != NULL)
> > -    {
> > -      DEBUG ("Error getting Storage properties: %s", error->message);
> > -      _tp_proxy_set_feature_prepared (proxy, TP_ACCOUNT_FEATURE_STORAGE,
> > -          FALSE);
> > -      return;
> > -    }
> > +    DEBUG ("Error getting Storage properties: %s", error->message);
> 
> Removing the early return looks wrong: properties will be NULL and all the
> accesses to it will (either g_critical() or) crash. Did you test this case?

Fixed.

review+ for the moment, and I'll add it to the list of APIs that need a GVarianty version.

Merged.