[Bug 30478] Add TP_ACCOUNT_FEATURE_STORAGE to TpAccount

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Danielle Madeley <danielle.madeley at collabora.co.uk> 2010-10-04 17:11:13 PDT ---
(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?

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