Bug 39248

On Bug #24661 I wrote:
> When we break telepathy-glib ABI for spec 1.0 and/or GDBus, we should either
> change TpContactFeature into a quark, or change both TpProxy and TpContact
> features into an opaque pointer of some sort.

While fixing this, please also change the function signature of tp_simple_client_factory_add_contact_features() to match tp_simple_client_factory_add_account_features(), tp_simple_client_factory_add_channel_features(), and tp_simple_client_factory_add_connection_features(). The latter all take a 0-terminated array of GQuarks; whereas tp_simple_client_factory_add_contact_features() takes two parameters: guint n_features and the array of features.

(Or, for that matter, change those three to use the (n, array) pattern. Your call!)

I'm pushing the proxy feature stuff into a mixin so hopefully TpContact can use it after some modifications.

Here's my initial branch just doing that. What do you think?

> I'm pushing the proxy feature stuff into a mixin so hopefully
> TpContact can use it after some modifications.

I'm unsure about this. Proxy features are very much "you prepare the object", whereas contact features are "you prepare a batch" (and have pessimal performance if you don't). I'm not sure that they're really enough of "the same shape" to merge the implementations.

(There's a little bit of crossover, in that the account manager prepares multiple accounts - but you have nowhere near as many accounts as contacts, and our D-Bus API can't batch account preparation anyway.)

My inclination would be to make the APIs consistent, and worry about the implementation later, if at all?

I'm not sure whether we should use quarks or opaque pointers for the features; opaque pointers are more type-safe. Either way, to be introspectable, I think we have to have the underlying function be considered to be public API, because g-i code is going to have to call it rather than using a macro.

+ * tp_feature_mixin_class_get_offset_quark: (skip)

Can we please have this not be API in new mixins? I'm reasonably sure there's at least one mixin (D-Bus properties, maybe?) that has the quark be non-API.

In fact, it might be worth supporting (or requiring) a usage mode where nothing is added to the instance struct and access is always via qdata, like the D-Bus properties mixin. You're going to have to access qdata anyway, to get the offset, so the qdata might as well be per-instance "pointer to the struct" rather than per-type "offset of the struct within the instance".

(You could even do the same for classes.)

As it stands at the moment, I think adding the feature mixin to an object that doesn't have it will normally be an ABI break, unless you're very careful about removing an amount of padding that matches the size of the mixed-in part?

+#include <gio/gio.h>
+
+#include "telepathy-glib/feature-mixin.h"

In each .c file, its own header (and internal header, if applicable) should always come straight after config.h, to verify that it's self-contained. In this case, the internal header isn't self-contained, because you forgot to include gio/gio.h in it.

+ /* invalidation ensures that these have gone away */
+ g_assert_cmpuint (g_queue_get_length (mixin->priv->prepare_requests), ==, 0);
+ tp_clear_pointer (&mixin->priv->prepare_requests, g_queue_free);

Is this going to become a problem when we make dispose not signal invalidation? (Probably not, because each prepare request hopefully refs the object.)

+ /* TODO: this is kind of ugly, but is harmless */
+ if (TP_IS_PROXY (self))
+ {
+ const GError *error = tp_proxy_get_invalidated ((TpProxy *) self);

... yeah. Maybe things with features need to be of some "invalidatable" subclass, or GInterface, or something?

I wonder whether doing mixins as a magic GInterface with side-effects would make them more introspectable?

(In reply to comment #5)
> > I'm pushing the proxy feature stuff into a mixin so hopefully
> > TpContact can use it after some modifications.
> 
> I'm unsure about this. Proxy features are very much "you prepare the object",
> whereas contact features are "you prepare a batch" (and have pessimal
> performance if you don't). I'm not sure that they're really enough of "the same
> shape" to merge the implementations.
> 
> (There's a little bit of crossover, in that the account manager prepares
> multiple accounts - but you have nowhere near as many accounts as contacts, and
> our D-Bus API can't batch account preparation anyway.)
> 
> My inclination would be to make the APIs consistent, and worry about the
> implementation later, if at all?

Hmm, yeah I guess you're right here, unfortunately.

Shall we still include this TpFeatureMixin like I've done even if we don't use it for TpContact? It might be useful if for nothing else but splitting up code to make it more legible and more split-up?

Created attachment 60241 [details] [review]
wip with some rubbish in it

For the record, here is what I had in mind about bunching together features so you'd only make one, say, GetContactAttributes call. I wasn't too happy with the 'multi' API but this was a first start.

(Obviously this is a total WIP patch which asserts somewhere else for some reason I never bothered to look into.)

But yes, this wouldn't allow calling GetContactAttributes on multiple contacts which is pretty loss. Oh well. :-(

Okay here's the branch you always wanted to see. Given TpContact translates features into flags soon anyway this was fairly trivial to switch to quarks.

Yeaaaaaaaaaaaaaaaaaaaaaaaaaaaah.

(In reply to comment #8)
> Okay here's the branch you always wanted to see.

Looks good, except that if you give random undefined quarks to a function expecting contact features, I'd like to see that be a critical (like it is for TpProxy) rather than silently accepted.

Rationale: it's far too easy to miss off the 0-termination...

(In reply to comment #3)
> (Or, for that matter, change those three to use the (n, array) pattern. Your
> call!)

... which is also an argument for changing everything to do this instead of the zero-termination pattern; but let's merge your branch first so it's at least consistent, then decide whether to be consistently the other thing afterwards.

(In reply to comment #9)
> Looks good, except that if you give random undefined quarks to a function
> expecting contact features, I'd like to see that be a critical (like it is for
> TpProxy) rather than silently accepted.
> 
> Rationale: it's far too easy to miss off the 0-termination...

Good point! I did this and merged the branch.