[Bug 31583] Expose TpProxyFeature in the API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #14 from Will Thompson <will.thompson at collabora.co.uk> 2011-01-25 09:09:45 PST ---
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?

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