[Bug 39248] [API break] Use GQuark for TpContact features, or opaque pointers for all features

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-04-13 09:25:22 PDT ---
> 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?

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