[Bug 31583] Expose TpProxyFeature in the API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #15 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2011-01-27 05:59:44 PST ---
(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?

-- 
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.