Bug 31583

(In reply to comment #0)
> Any reason to not move TpProxyFeature as public API ?

The reason was: not knowing whether the current API was something we wanted to commit to longer-term.

We can revisit this if needed, by re-reviewing the current API.

typedef void (*TpProxyProc) (TpProxy *);

struct _TpProxyFeature {
    /*<public>*/
    GQuark name;
    gboolean core;
    TpProxyProc start_preparing;
    /*<private>*/
    GCallback _reserved[4];
    gpointer priv;
};

gboolean _tp_proxy_is_preparing (gpointer self,
    GQuark feature);
void _tp_proxy_set_feature_prepared (TpProxy *self,
    GQuark feature,
    gboolean succeeded);
void _tp_proxy_set_features_failed (TpProxy *self,
    const GError *error);


This API served us will and we still have room for improvements. Maybe have a proper TpProxyPrivate struct?

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/proxy-feature-31583

Concerns about having this as our public API:

Feature introspection functions (start_preparing()) are quite boilerplate'y: the ones on Connection all start with

* a check for whether they've already completed (feature-specific: the feature might be able to grow extra functionality after CONNECTED)

* a check for whether we want them (not needed when called as start_preparing(), but needed when CORE introspection has progressed)

* a check for whether the object's CORE/CONNECTED functionality is ready (feature-specific: they might be able to achieve something without CONNECTED)

* a check for whether the same preparation is already in-flight

* a check for whether we have the desired interface

Also, the functions are generally called from two places: once from the introspection pipeline, and once from the feature pseudo-vtable by TpProxy.

Features that don't need CONNECTED, but grow more functionality after CONNECTED (of which there aren't any yet, but SimplePresence would be one such) need to be able to delay CONNECTED state, which currently means they need to be able to call into the object-specific introspection pipeline.

telepathy-qt4 abstracts some of this, but the cost is that it needs more elaborate data structures, and its generic introspection pipeline is rather complicated (it also has a special case for Connection's three statuses which doesn't make sense on any other object!)

What feature in Empathy needs this? Can we put that feature in telepathy-glib instead?

(In reply to comment #4)
> * a check for whether they've already completed (feature-specific: the feature
> might be able to grow extra functionality after CONNECTED)
> 
> * a check for whether we want them (not needed when called as
> start_preparing(), but needed when CORE introspection has progressed)
> 
> * a check for whether the object's CORE/CONNECTED functionality is ready
> (feature-specific: they might be able to achieve something without CONNECTED)
> 
> * a check for whether the same preparation is already in-flight
> 
> * a check for whether we have the desired interface
> 
> Also, the functions are generally called from two places: once from the
> introspection pipeline, and once from the feature pseudo-vtable by TpProxy.

Can't we solve most of this by adding some guarantee about
when start_preparing() is called: "start_preparing() is called only once when we actually want to
prepare the feature and we are not already preparing it".

We could add API saying if we need CONNECTED or CORE prepared or not.

> Features that don't need CONNECTED, but grow more functionality after CONNECTED
> (of which there aren't any yet, but SimplePresence would be one such) need to
> be able to delay CONNECTED state, which currently means they need to be able to
> call into the object-specific introspection pipeline.

I'm not sure to understand this. Why is SimplePresence special?

> What feature in Empathy needs this? Can we put that feature in telepathy-glib
> instead?

I'm trying to get ride of EmpathyTpChat by using TpTextChannel. For now my
TpTextChannel implementation doesn't offer all the features provided by
TpTextChannel, so as a transitionnal step I'm rebasing EmpathyTpChat on top of
TpTextChannel (relying as much on TpTextChannel as possible).

One of this feature is about channel "upgrades" (using Conference), and it
needs to check if the Connection supports Conference. To do so, the
TpConnection of the channel needs to have TP_CONNECTION_FEATURE_CAPABILITIES
prepared which we can't guarantee. So I need to have a CORE feature on
EmpathyTpChat that will do this preparation and init the needed bits in
EmpathyTpChat.

Anyway, not being able to define our own features kinda misses most of the
point of having user-defined TpChannel subclasses (which was one of the main
goal when we designed channel factories).

Another obvious use case for this is to experiment/test a TpChannel subclass for a new channel type in, say, Empathy and then move it to tp-glib once the spec is stable and we are happy with its API.

(In reply to comment #5)
> > Features that don't need CONNECTED, but grow more functionality after CONNECTED
> > (of which there aren't any yet, but SimplePresence would be one such) need to
> > be able to delay CONNECTED state, which currently means they need to be able to
> > call into the object-specific introspection pipeline.
> 
> I'm not sure to understand this. Why is SimplePresence special?

Before the D-Bus status becomes CONNECTED, enabling SimplePresence should call GetAll and look at the properties.

When the D-Bus status becomes CONNECTED, the SimplePresence properties may have changed. telepathy-qt4 has the useful guarantee that the change to CONNECTED is not signalled up to API users until each feature has had a chance to sort itself out: in this case we'd call GetAll again, and not allow the CONNECTED feature to become ready until that feature had finished.

I think we could probably do this with a structure more like this (pseudocode):

ProxyFeature
{
  Quark name;

  /* If TRUE, every non-core feature in this class depends on this one,
   * and every feature (core or not) in subclasses depends on this one. */
  boolean core;

  /* If TRUE, allow retrying prep of this feature even if it failed once
   * already. ConnectionManager.CORE needs this. */
  boolean can_retry;

  /* Other dependencies */
  Quark[] depends_on;

  /* D-Bus interfaces without which preparation just fails */
  Quark[] interfaces_needed;

  /* Guaranteed not to be called until every feature this depends on was
   * prepared successfully. If any of them failed, this fails too.
   *
   * If this is the core feature, the error is equivalent to
   * tp_proxy_invalidate, unless can_retry is true, in which case
   * it's equivalent to _tp_proxy_set_features_failed.
   *
   * If !can_retry, this will never be called again.
   */
  async void prepare() throws GError;

  /* As above but for before CONNECTED; only relevant to Connection. */
  async void prepare_after_connected() throws GError;
}

Removing 'patch' keyword as this branch is not ready yet. Are you planning to implement your proposed design?

(In reply to comment #7)
> (In reply to comment #5)
> > > Features that don't need CONNECTED, but grow more functionality after CONNECTED
> > > (of which there aren't any yet, but SimplePresence would be one such) need to
> > > be able to delay CONNECTED state, which currently means they need to be able to
> > > call into the object-specific introspection pipeline.
> > 
> > I'm not sure to understand this. Why is SimplePresence special?
> 
> Before the D-Bus status becomes CONNECTED, enabling SimplePresence should call
> GetAll and look at the properties.
> 
> When the D-Bus status becomes CONNECTED, the SimplePresence properties may have
> changed. telepathy-qt4 has the useful guarantee that the change to CONNECTED is
> not signalled up to API users until each feature has had a chance to sort
> itself out: in this case we'd call GetAll again, and not allow the CONNECTED
> feature to become ready until that feature had finished.

I see.

> I think we could probably do this with a structure more like this (pseudocode):
> 
> ProxyFeature
> {
>   Quark name;
> 
>   /* If TRUE, every non-core feature in this class depends on this one,
>    * and every feature (core or not) in subclasses depends on this one. */
>   boolean core;

IIRC, core features are also automatically prepared when creating the proxy.

> 
>   /* If TRUE, allow retrying prep of this feature even if it failed once
>    * already. ConnectionManager.CORE needs this. */
>   boolean can_retry;
> 
>   /* Other dependencies */
>   Quark[] depends_on;
> 
>   /* D-Bus interfaces without which preparation just fails */
>   Quark[] interfaces_needed;
> 
>   /* Guaranteed not to be called until every feature this depends on was
>    * prepared successfully. If any of them failed, this fails too.
>    *
>    * If this is the core feature, the error is equivalent to
>    * tp_proxy_invalidate, unless can_retry is true, in which case
>    * it's equivalent to _tp_proxy_set_features_failed.
>    *
>    * If !can_retry, this will never be called again.
>    */
>   async void prepare() throws GError;
> 
>   /* As above but for before CONNECTED; only relevant to Connection. */
>   async void prepare_after_connected() throws GError;
> }

This design sounds good to me.

(In reply to comment #8)
> Removing 'patch' keyword as this branch is not ready yet. Are you planning to
> implement your proposed design?

I was going to, but got pre-empted; someone else will have to pick this up I'm afraid.

> IIRC, core features are also automatically prepared when creating the proxy.

Not necessarily; it used to be desirable for ChannelRequest to not prepare anything automatically, because channel requests are nearly always used as an opaque token whose properties aren't needed. (Having said that, we now provide immutable properties everywhere a ChannelRequest is used, I think...)

(In reply to comment #9)
> (In reply to comment #8)
> > Removing 'patch' keyword as this branch is not ready yet. Are you planning to
> > implement your proposed design?
> 
> I was going to, but got pre-empted; someone else will have to pick this up I'm
> afraid.

Ok, I'll do it then.

(In reply to comment #7)
> ProxyFeature
> {
>   Quark name;
> 
>   /* If TRUE, every non-core feature in this class depends on this one,
>    * and every feature (core or not) in subclasses depends on this one. */
>   boolean core;

Does that mean that if we have MY_CHANNEL_CORE_FEATURE, we shouldn't call its
prepare function while TP_CHANNEL_CORE hasn't be prepared? We don't do that
atm.

>   /* If TRUE, allow retrying prep of this feature even if it failed once
>    * already. ConnectionManager.CORE needs this. */
>   boolean can_retry;

How so? TP_CONNECTION_MANAGER_FEATURE_CORE is never explicitely prepared by
the user (it doesn't have a prepare function).

>   /* Other dependencies */
>   Quark[] depends_on;
> 
>   /* D-Bus interfaces without which preparation just fails */
>   Quark[] interfaces_needed;
> 
>   /* Guaranteed not to be called until every feature this depends on was
>    * prepared successfully. If any of them failed, this fails too.
>    *
>    * If this is the core feature, the error is equivalent to
>    * tp_proxy_invalidate, unless can_retry is true, in which case
>    * it's equivalent to _tp_proxy_set_features_failed.
>    *
>    * If !can_retry, this will never be called again.
>    */
>   async void prepare() throws GError;
> 
>   /* As above but for before CONNECTED; only relevant to Connection. */
>   async void prepare_after_connected() throws GError;
> }

I made good progess in this branch (based on smcv's work):
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/proxy-feature-31583

So far I've done the following changes:
- Make feature preparation async
- Add depends_on
- Add interfaces_needed
+ tests

Still missing:
- Improve CORE dependencies (see above) ?
- Add can_retry
- add prepare_after_connected()
- Make the API public

This branch is already pretty big, so do you prefer reviewing/merging this
first before continuing the refactoring?
It shouldn't introduce any regression comparing to the existing code.

(In reply to comment #11)
> Does that mean that if we have MY_CHANNEL_CORE_FEATURE, we shouldn't call its
> prepare function while TP_CHANNEL_CORE hasn't be prepared? We don't do that
> atm.

Yes, I think we should avoid preparing subclasses' core features until the superclass's core feature is ready. I'm aware that this is a new guarantee that we didn't previously have, but I think the semantics I proposed here are probably what we want.

> >   /* If TRUE, allow retrying prep of this feature even if it failed once
> >    * already. ConnectionManager.CORE needs this. */
> >   boolean can_retry;
> 
> How so? TP_CONNECTION_MANAGER_FEATURE_CORE is never explicitely prepared by
> the user (it doesn't have a prepare function).

Preparation is implicitly started by creating the object, but if the API user calls tp_proxy_prepare_async twice at different times, it can return FALSE then TRUE (if a buggy CM-under-test crashes the first time, then works correctly the second time, for instance).

(In reply to comment #12)
> (In reply to comment #11)
> > Does that mean that if we have MY_CHANNEL_CORE_FEATURE, we shouldn't call its
> > prepare function while TP_CHANNEL_CORE hasn't be prepared? We don't do that
> > atm.
> 
> Yes, I think we should avoid preparing subclasses' core features until the
> superclass's core feature is ready. I'm aware that this is a new guarantee that
> we didn't previously have, but I think the semantics I proposed here are
> probably what we want.

I agree, that's better. I've done that.

> Still missing:
> - Improve CORE dependencies (see above) ?

done.

> - Add can_retry

done.

> - add prepare_after_connected()

Done. I finally went for prepare_before_signalling_connected_async() as it's
clearer ({before,after}_connected can both make sense and so are pretty
confusing imho) but I'm open to a better suggestion.

Any chance you could take a quick look on my branch and see if it fits what
you had in mind ?

> - Make the API public

That's still TODO.

225dbfc:

+              case FEATURE_STATE_UNWANTED:
+                /* this can only happen in the special pseudo-request for the
+                 * core features, which blocks everything */
+                g_assert (req == self->priv->prepare_core);
+                wait = TRUE;
+                break;
+
+                /* fall through to treat it as WANTED */

That does not look like a fall through to me. This later moved into request_is_complete(). And then was fixed! I bet you left this in to test me. ;-)


a5adfbb:

@@ -649,7 +676,8 @@ get_pending_messages_cb (TpProxy *proxy,
     gpointer user_data,
     GObject *weak_object)
 {
-  TpTextChannel *self = user_data;
+  TpTextChannel *self = (TpTextChannel *) weak_object;

We may as well use a checked cast if we're casting at all. But...

+typedef struct
+{
+  GList *parts_list;
+  GSimpleAsyncResult *result;
+} IdentifyMessagesCtx;
+

If you use the GSimpleAsyncResult object as the weak_object, you don't need to faff with context structs, since you can fish the TpTextChannel out of it using g_simple_async_result_get_source_object.

(Since the GSimpleAsyncResult refs the channel anyway, this doesn't change the weak_object-y behaviour at all.)

-              0, NULL, got_pending_senders_contact_by_id_cb, parts_list,
-              free_parts_list, G_OBJECT (self));
+              0, NULL, got_pending_senders_contact_by_id_cb, ctx,
+              (GDestroyNotify) identify_messages_free, G_OBJECT (self));

You seem to have changed the calls, but not updated the callbacks for the changed type of the user_data. So I'm not sure how this can possibly work.


c9daa96:

+      if (state == FEATURE_STATE_INVALID)
+        continue;
+      else if (state == FEATURE_STATE_UNWANTED)
+        {
+          if (check_feature_interfaces (self, features[i]))
+            {
+              tp_proxy_set_feature_state (self, features[i],
+                  FEATURE_STATE_WANTED);
+            }
+          else
+            {
+              tp_proxy_set_feature_state (self, features[i],
+                  FEATURE_STATE_FAILED);
+            }

Coding style: if one block uses {}s, so should they all.

I wonder if this loop's body would be clearer as a switch.

Also, wait, what? You're upgrading unwanted features to wanted features if the object has the necessary interfaces? Am I missing something here? Are the enum members mis-documented?

Commit ca12418 rearranges this code a bit, but still has the suspicious Unwanted → Wanted transition.


ca12418:

+                    /* We have to wait than deps finished their
+                     * preparation */

"We have to wait until the deps finish their preparation."

+            DEBUG ("Can't prepare %s, one is dep %s: %s",
+                g_quark_to_string (name),
+                dep_state == FEATURE_STATE_INVALID ? "is invalid": "failed",
+                g_quark_to_string (dep));

"Can't prepare foo, one is dep is invalid: bar" isn't English.

"Can't prepare %(name)s, because %(dep)s (a dependency) is invalid/failed to prepare" would be better.

+  if (G_UNLIKELY (need_avatars[0] == 0))
+    need_avatars[0] = TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS;
+  features[FEAT_AVATAR_REQUIREMENTS].interfaces_needed = need_avatars;

is duplicated constantly. The G_UNLIKELY() is not just unlikely, it's impossible, because the whole function is guarded by

   if (G_LIKELY (features[0].name != 0))
     return features;

so what's wrong with a function that allocates and returns a zero-terminated quark array? It could even be varargs for people who want to depend on more than one feature.


53d656:

+  g_type_init ();
+  tp_tests_abort_after (10);
+  tp_debug_set_flags ("all");
+
+  g_test_init (&argc, &argv, NULL);
+  g_test_bug_base ("http://bugs.freedesktop.org/show_bug.cgi?id=");

Every single test starts with this. Can we have tp_tests_init (&argc, &argv); ?

+  /* Prepare capabilities on a new proxy. Core should be prepared *before*
+   * checking if Requests is implemented */

Does this test test that? Why? I assume this test caught the bug the previous commit fixes. :)

+ * Copyright © 2010 Collabora Ltd. <http://www.collabora.co.uk/>

I don't think we're in 2010 any more, Toto. (Maybe you wrote this in 2010 and rebased?


ea3d6466:

+static const TpProxyFeature *
+list_features (TpProxyClass *cls G_GNUC_UNUSED)
+{
+  static TpProxyFeature features[N_FEAT + 1] = { { 0 } };
+
+    if (G_LIKELY (features[0].name != 0))
+    return features;


indentaaation.


Your tests' prepare_foo_async functions should assert that the dependent features have been prepared before they're called. I see that you do check that the interfaces are checked, and that children of unpreparable features aren't told to prepare; adding an assertion to feature B's preparation function wouldn't hurt.


+    /* List of TpProxyPrepareRequest. The first requests are the core one,
+     * sorted from the most upper super class to the subclass core features.
+     * This is needed to guarantee than subclass features are not prepared
+     * until the super class features have been prepared. */
     GList *prepare_requests;

This is a hobbyhorse of mine, but: this should be a GQueue (nb. you can use GQueue as opposed to GQueue * if you're into that sort of thing). We prepend in one place, and append in another place, so GQueue is genuinely a more appropriate data structure.



+static gboolean
+core_prepared (TpProxy *self)
+{
+  /* All the core features have been prepared if the head of the
+   * prepare_requests list is NOT a core feature */
+  TpProxyPrepareRequest *req = self->priv->prepare_requests->data;
+
+  if (req == NULL)
+    return TRUE;
+
+  return !req->core;
+}

Why would the data in the head of the list be NULL? Did you mean to check whether the list is empty?

       /* Core features have to be prepared first */
       if (!core_prepared (self) &&
-          !req->core)
+          req != self->priv->prepare_requests->data)

I think this deserves more of a comment. “Core features have to be prepared first, in superclass-to-subclass order. The next core feature to be prepared, if any, is always at the head of prepare_requests.”


+static void
+check_feature_validity (TpProxy *self,
+    const TpProxyFeature *feature)

I'd include "assert" in the function name.

I think it's really sketchy that proxy.c has connection-specific stuff in it. I don't think any of this stuff should be in there. Can't the connection have the special knowledge about which features should be prepared just before announcing Connected, and call tp_proxy_prepare_async() for them all?

(In reply to comment #14)
> 225dbfc:
> 
> +              case FEATURE_STATE_UNWANTED:
> +                /* this can only happen in the special pseudo-request for the
> +                 * core features, which blocks everything */
> +                g_assert (req == self->priv->prepare_core);
> +                wait = TRUE;
> +                break;
> +
> +                /* fall through to treat it as WANTED */
> 
> That does not look like a fall through to me. This later moved into
> request_is_complete(). And then was fixed! I bet you left this in to test me.
> ;-)

Oops, I guess I forgot to squash the fix, nice catch! :)

> a5adfbb:
> 
> @@ -649,7 +676,8 @@ get_pending_messages_cb (TpProxy *proxy,
>      gpointer user_data,
>      GObject *weak_object)
>  {
> -  TpTextChannel *self = user_data;
> +  TpTextChannel *self = (TpTextChannel *) weak_object;
> 
> We may as well use a checked cast if we're casting at all. But...

Those are more time consuming, so I try to limit them.

> +typedef struct
> +{
> +  GList *parts_list;
> +  GSimpleAsyncResult *result;
> +} IdentifyMessagesCtx;
> +
> 
> If you use the GSimpleAsyncResult object as the weak_object, you don't need to
> faff with context structs, since you can fish the TpTextChannel out of it using
> g_simple_async_result_get_source_object.
> 
> (Since the GSimpleAsyncResult refs the channel anyway, this doesn't change the
> weak_object-y behaviour at all.)

Oh, good idea. done.

> -              0, NULL, got_pending_senders_contact_by_id_cb, parts_list,
> -              free_parts_list, G_OBJECT (self));
> +              0, NULL, got_pending_senders_contact_by_id_cb, ctx,
> +              (GDestroyNotify) identify_messages_free, G_OBJECT (self));
> 
> You seem to have changed the calls, but not updated the callbacks for the
> changed type of the user_data. So I'm not sure how this can possibly work.

Yeah you're right, it's fixed now. Unfortunatelly this code was untested :(
(testing isn't trivial as that's the fallback path used with CMs not
 implementing newer spec (ImmportaHandles) which is automatically implemented
by TpBaseConnection so with all tp-glib CMs...)

> c9daa96:
> 
> +      if (state == FEATURE_STATE_INVALID)
> +        continue;
> +      else if (state == FEATURE_STATE_UNWANTED)
> +        {
> +          if (check_feature_interfaces (self, features[i]))
> +            {
> +              tp_proxy_set_feature_state (self, features[i],
> +                  FEATURE_STATE_WANTED);
> +            }
> +          else
> +            {
> +              tp_proxy_set_feature_state (self, features[i],
> +                  FEATURE_STATE_FAILED);
> +            }
> 
> Coding style: if one block uses {}s, so should they all.

fixed.

> I wonder if this loop's body would be clearer as a switch.

Don't think so as we have to check feature->can_retry as well.

> Also, wait, what? You're upgrading unwanted features to wanted features if the
> object has the necessary interfaces? Am I missing something here? Are the enum
> members mis-documented?

Yeah, as the feature as been requested (using tp_proxy_prepare_async()), if
the interfaces/deps are good, the property is now WANTED .

> ca12418:
> 
> +                    /* We have to wait than deps finished their
> +                     * preparation */
> 
> "We have to wait until the deps finish their preparation."

fixed.

> +            DEBUG ("Can't prepare %s, one is dep %s: %s",
> +                g_quark_to_string (name),
> +                dep_state == FEATURE_STATE_INVALID ? "is invalid": "failed",
> +                g_quark_to_string (dep));
> 
> "Can't prepare foo, one is dep is invalid: bar" isn't English.
> 
> "Can't prepare %(name)s, because %(dep)s (a dependency) is invalid/failed to
> prepare" would be better.

changed.

> +  if (G_UNLIKELY (need_avatars[0] == 0))
> +    need_avatars[0] = TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS;
> +  features[FEAT_AVATAR_REQUIREMENTS].interfaces_needed = need_avatars;
> 
> is duplicated constantly. The G_UNLIKELY() is not just unlikely, it's
> impossible, because the whole function is guarded by
> 
>    if (G_LIKELY (features[0].name != 0))
>      return features;

Oh you're right. I removed those.

> so what's wrong with a function that allocates and returns a zero-terminated
> quark array? It could even be varargs for people who want to depend on more
> than one feature.

Humm, then we'd have to allocate the arrays on the heap rather than on the
stack, and they will be leaked which won't make valgrind happy. :\

> 53d656:
> 
> +  g_type_init ();
> +  tp_tests_abort_after (10);
> +  tp_debug_set_flags ("all");
> +
> +  g_test_init (&argc, &argv, NULL);
> +  g_test_bug_base ("http://bugs.freedesktop.org/show_bug.cgi?id=");
> 
> Every single test starts with this. Can we have tp_tests_init (&argc, &argv); ?

Done. I recorded in such way we can easily cherry pick the commits to master
if needed.

> +  /* Prepare capabilities on a new proxy. Core should be prepared *before*
> +   * checking if Requests is implemented */
> 
> Does this test test that? Why? I assume this test caught the bug the previous
> commit fixes. :)

I improved this comment to make it clearer.

> + * Copyright © 2010 Collabora Ltd. <http://www.collabora.co.uk/>
> 
> I don't think we're in 2010 any more, Toto. (Maybe you wrote this in 2010 and
> rebased?

I updated to 2010-2011.

> ea3d6466:
> 
> +static const TpProxyFeature *
> +list_features (TpProxyClass *cls G_GNUC_UNUSED)
> +{
> +  static TpProxyFeature features[N_FEAT + 1] = { { 0 } };
> +
> +    if (G_LIKELY (features[0].name != 0))
> +    return features;
> 
> 
> indentaaation.

fixed.

> Your tests' prepare_foo_async functions should assert that the dependent
> features have been prepared before they're called. I see that you do check that
> the interfaces are checked, and that children of unpreparable features aren't
> told to prepare; adding an assertion to feature B's preparation function
> wouldn't hurt.

Humm I'm not sure to understand. prepare_b_async() does check if A is
prepared.

> +    /* List of TpProxyPrepareRequest. The first requests are the core one,
> +     * sorted from the most upper super class to the subclass core features.
> +     * This is needed to guarantee than subclass features are not prepared
> +     * until the super class features have been prepared. */
>      GList *prepare_requests;
> 
> This is a hobbyhorse of mine, but: this should be a GQueue (nb. you can use
> GQueue as opposed to GQueue * if you're into that sort of thing). We prepend in
> one place, and append in another place, so GQueue is genuinely a more
> appropriate data structure.

done.

> +static gboolean
> +core_prepared (TpProxy *self)
> +{
> +  /* All the core features have been prepared if the head of the
> +   * prepare_requests list is NOT a core feature */
> +  TpProxyPrepareRequest *req = self->priv->prepare_requests->data;
> +
> +  if (req == NULL)
> +    return TRUE;
> +
> +  return !req->core;
> +}
> 
> Why would the data in the head of the list be NULL? Did you mean to check
> whether the list is empty?

Indeed, but that's now fixed with the GQueue-ification.

>        /* Core features have to be prepared first */
>        if (!core_prepared (self) &&
> -          !req->core)
> +          req != self->priv->prepare_requests->data)
> 
> I think this deserves more of a comment. “Core features have to be prepared
> first, in superclass-to-subclass order. The next core feature to be prepared,
> if any, is always at the head of prepare_requests.”

I used your comment, thanks.

> +static void
> +check_feature_validity (TpProxy *self,
> +    const TpProxyFeature *feature)
> 
> I'd include "assert" in the function name.

good idea; done.

> I think it's really sketchy that proxy.c has connection-specific stuff in it. I
> don't think any of this stuff should be in there. Can't the connection have the
> special knowledge about which features should be prepared just before
> announcing Connected, and call tp_proxy_prepare_async() for them all?

It's more subtile than that. prepare_before_signalling_connected_async()
doesn't mean we have to prepare the feature before announcing CONNECTED but
that IF the feature is already prepared, then we have to give it a chance to
update itself before annoucning CONNECTED.

I agree that the connection special case in proxy.c isn't great, but tbh
prepare_before_signalling_connected_async is already connection specific any
way. I discussed this with smcv when I started working on this and he agreed
that modifying proxy.c was probably the way to go.

I guess the alternative would be to expose details about interfaces to
connection.c and move mosf of the code there?

I had to rebase the branch on top of master in order to start using in Empathy. The latest commit you reviewed was:


commit b55b2d2f236dc2f09220f93bf964454ad685c62b
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Tue Jan 18 15:12:04 2011 +0100

    test prepare_before_signalling_connected_async

(In reply to comment #15)
> (In reply to comment #14)
> > a5adfbb:
> > 
> > @@ -649,7 +676,8 @@ get_pending_messages_cb (TpProxy *proxy,
> >      gpointer user_data,
> >      GObject *weak_object)
> >  {
> > -  TpTextChannel *self = user_data;
> > +  TpTextChannel *self = (TpTextChannel *) weak_object;
> > 
> > We may as well use a checked cast if we're casting at all. But...
> 
> Those are more time consuming, so I try to limit them.

Avoiding repeated checked casts in the same function is fine: if you've already checked the cast to TpTextChannel, then casting to GObject is fine. But a callback's user_data seems like a perfect place to use a checked cast.

If you insist on not using TP_TEXT_CHANNEL(), then don't use any explicit cast at all, since user_data is a gpointer. (Is 'self' the same pointer as 'proxy' here?)

> > +typedef struct
> > +{
> > +  GList *parts_list;
> > +  GSimpleAsyncResult *result;
> > +} IdentifyMessagesCtx;
> > +
> > 
> > If you use the GSimpleAsyncResult object as the weak_object, you don't need to
> > faff with context structs, since you can fish the TpTextChannel out of it using
> > g_simple_async_result_get_source_object.
> > 
> > (Since the GSimpleAsyncResult refs the channel anyway, this doesn't change the
> > weak_object-y behaviour at all.)
> 
> Oh, good idea. done.

You could use free_parts_list as the destroy callback for tp_connection_get_contacts_by_id() and tp_connection_get_contacts_by_handle().

> > -              0, NULL, got_pending_senders_contact_by_id_cb, parts_list,
> > -              free_parts_list, G_OBJECT (self));
> > +              0, NULL, got_pending_senders_contact_by_id_cb, ctx,
> > +              (GDestroyNotify) identify_messages_free, G_OBJECT (self));
> > 
> > You seem to have changed the calls, but not updated the callbacks for the
> > changed type of the user_data. So I'm not sure how this can possibly work.
> 
> Yeah you're right, it's fixed now. Unfortunatelly this code was untested :(
> (testing isn't trivial as that's the fallback path used with CMs not
>  implementing newer spec (ImmportaHandles) which is automatically implemented
> by TpBaseConnection so with all tp-glib CMs...)

Fun.

Olli dealt with this for some other stuff by deliberately breaking the DBus.Properties implementation for an interface; if you were keen, you could do the same to force the fallback?

> > I wonder if this loop's body would be clearer as a switch.
> 
> Don't think so as we have to check feature->can_retry as well.

Okay.

> > Also, wait, what? You're upgrading unwanted features to wanted features if the
> > object has the necessary interfaces? Am I missing something here? Are the enum
> > members mis-documented?
> 
> Yeah, as the feature as been requested (using tp_proxy_prepare_async()), if
> the interfaces/deps are good, the property is now WANTED .

I still don't really understand. I'll take another look on Monday.

> > so what's wrong with a function that allocates and returns a zero-terminated
> > quark array? It could even be varargs for people who want to depend on more
> > than one feature.
> 
> Humm, then we'd have to allocate the arrays on the heap rather than on the
> stack, and they will be leaked which won't make valgrind happy. :\

Fair enough.

> > 53d656:
> > 
> > +  g_type_init ();
> > +  tp_tests_abort_after (10);
> > +  tp_debug_set_flags ("all");
> > +
> > +  g_test_init (&argc, &argv, NULL);
> > +  g_test_bug_base ("http://bugs.freedesktop.org/show_bug.cgi?id=");
> > 
> > Every single test starts with this. Can we have tp_tests_init (&argc, &argv); ?
> 
> Done. I recorded in such way we can easily cherry pick the commits to master
> if needed.

Great.

> > Your tests' prepare_foo_async functions should assert that the dependent
> > features have been prepared before they're called. I see that you do check that
> > the interfaces are checked, and that children of unpreparable features aren't
> > told to prepare; adding an assertion to feature B's preparation function
> > wouldn't hurt.
> 
> Humm I'm not sure to understand. prepare_b_async() does check if A is
> prepared.

I'll double-check this on Monday too.

> > +    /* List of TpProxyPrepareRequest. The first requests are the core one,
> > +     * sorted from the most upper super class to the subclass core features.
> > +     * This is needed to guarantee than subclass features are not prepared
> > +     * until the super class features have been prepared. */
> >      GList *prepare_requests;
> > 
> > This is a hobbyhorse of mine, but: this should be a GQueue (nb. you can use
> > GQueue as opposed to GQueue * if you're into that sort of thing). We prepend in
> > one place, and append in another place, so GQueue is genuinely a more
> > appropriate data structure.
> 
> done.

In http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=855f03acf403ad9359d199f22294498169e4710e you used a GQueue * in the struct, not a GQueue...

I mean, it's fine, it works, but…

Also:

-  for ( ; iter != NULL; iter = g_list_delete_link (iter, iter))
+  for (iter = tmp->head; iter != NULL; iter = g_list_next (iter))
     {
       tp_proxy_prepare_request_finish (iter->data, error);
     }
+

This could become while ((proxy = g_queue_pop_head (iter)) != NULL) { tp_proxy_prepare_request_finish (proxy, error); }. Not a big deal though.

> > I think it's really sketchy that proxy.c has connection-specific stuff in it. I
> > don't think any of this stuff should be in there. Can't the connection have the
> > special knowledge about which features should be prepared just before
> > announcing Connected, and call tp_proxy_prepare_async() for them all?
> 
> It's more subtile than that. prepare_before_signalling_connected_async()
> doesn't mean we have to prepare the feature before announcing CONNECTED but
> that IF the feature is already prepared, then we have to give it a chance to
> update itself before annoucning CONNECTED.

Ah, I see.

> I agree that the connection special case in proxy.c isn't great, but tbh
> prepare_before_signalling_connected_async is already connection specific any
> way. I discussed this with smcv when I started working on this and he agreed
> that modifying proxy.c was probably the way to go.
> 
> I guess the alternative would be to expose details about interfaces to
> connection.c and move mosf of the code there?

Pragmatically, let's stick with the current solution for now.

(In reply to comment #17)
> I still don't really understand. I'll take another look on Monday.

I took another look — on *a* Monday, just not the one I originally said ;-) — and this looks fine when looking at the actual code and not just at a diff!

> I'll double-check this on Monday too.

Yep, it looks fine, I just missed it apparently.

The only outstanding issues as such are the cast and GDestroyNotify nitpicks. I leave the choice of whether or not to change them before merging this branch to master up to your impeccable judgement!

I rebased the branch on top of master to make sure it still applies nicely (it
does) and didn't break tests (it doesn't!).
The last commit your viewed was:

commit b55ba11e86d458df41fcf56f214b9675dbe37416
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Jan 27 14:49:03 2011 +0100

    use tp_tests_init() in most tests

(In reply to comment #17)
> > > a5adfbb:
> > > 
> > > @@ -649,7 +676,8 @@ get_pending_messages_cb (TpProxy *proxy,
> > >      gpointer user_data,
> > >      GObject *weak_object)
> > >  {
> > > -  TpTextChannel *self = user_data;
> > > +  TpTextChannel *self = (TpTextChannel *) weak_object;
> > > 
> > > We may as well use a checked cast if we're casting at all. But...
> > 
> > Those are more time consuming, so I try to limit them.
> 
> Avoiding repeated checked casts in the same function is fine: if you've already
> checked the cast to TpTextChannel, then casting to GObject is fine. But a
> callback's user_data seems like a perfect place to use a checked cast.
> 
> If you insist on not using TP_TEXT_CHANNEL(), then don't use any explicit cast
> at all, since user_data is a gpointer. (Is 'self' the same pointer as 'proxy'
> here?)

Yeah it is. I changed to now cast from proxy instead of the weak_object as
that's a probably bit clearer.

> > > +typedef struct
> > > +{
> > > +  GList *parts_list;
> > > +  GSimpleAsyncResult *result;
> > > +} IdentifyMessagesCtx;
> > > +
> > > 
> > > If you use the GSimpleAsyncResult object as the weak_object, you don't need to
> > > faff with context structs, since you can fish the TpTextChannel out of it using
> > > g_simple_async_result_get_source_object.
> > > 
> > > (Since the GSimpleAsyncResult refs the channel anyway, this doesn't change the
> > > weak_object-y behaviour at all.)
> > 
> > Oh, good idea. done.
> 
> You could use free_parts_list as the destroy callback for
> tp_connection_get_contacts_by_id() and tp_connection_get_contacts_by_handle().

Good idea; done.

If you're happy with the refactoring, this branch move feature as public API (which was the main goal of all of this) : http://cgit.collabora.co.uk/git/user/cassidy/telepathy-glib/log/?h=proxy-feature-31583%2bpublic

ship it!

Wooo I merged the refactoring branch \o/

Are you ok with http://cgit.collabora.co.uk/git/user/cassidy/telepathy-glib/log/?h=proxy-feature-31583%2bpublic as well ?

Merged; will be in 0.15.1 !

Created attachment 60652 [details] [review]
Retry to prepare features which failed because of a missing conn iface

If we try to prepare a connection feature before the connection is connected,
the preparation may fall if a required interface hasn't been announced yet by
the CM. So, we give those features a second chance to be prepared when the
connection has been connected.

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

Comment on attachment 60652 [details] [review]
Retry to prepare features which failed because of a missing conn iface

Sorry wrong bug.