+      for (i = 0; iface->presence_statuses[i].name; i++)
+        {
+          ret = g_list_prepend (ret, &(iface->presence_statuses[i]));
+        }
+      ret = g_list_reverse (ret);

I'm allergic to this prepend-and-reverse pattern. I think this function would be clearer written as follows (with coding style fixes like adding a blank line before the 'for', adding the explicit != NULL):

  GQueue statuses = G_QUEUE_INIT;

  if (iface->presence_statuses != NULL)
    {
      gint i;

      for (i = 0; iface->presence_statuses[i].name != NULL; i++)
        g_queue_push_tail (&statuses, &(iface->presence_statuses[i]));
    }

  return statuses.head;

But the point is moot because if this function just returned the array of statuses from the interface, gabble_plugin_loader_append_statuses() would be way shorter and easier to follow (untested):

  GArray *result = g_array_new (TRUE, TRUE, sizeof (TpPresenceStatusSpec));
  guint i, j;

  for (i = 0; base_statuses[i].name != NULL; i++)
    g_array_append_val (result, base_statuses[i]);

  for (i = 0; i < priv->plugins->len; i++)
    {
      GabblePlugin *p = g_ptr_array_index (priv->plugins, i);
      TpPresenceStatusSpec *statuses = gabble_plugin_get_custom_presence_statuses (p));

      for (j = 0; statuses[j].name != NULL; j++)
        g_array_append_val (result, statuses[j]);
    }

  return g_array_free (result, FALSE);

+      if (status)
+          return status;

Coding style dictates:

+      if (status != NULL)
+        return status;

Ditto indentation of:

+  if (iface->privacy_list_map == NULL)
+      return NULL;

I think indenting the second argument of tp_strdiff() in

+      if (!tp_strdiff (list_name,
+          iface->privacy_list_map[i].privacy_list_name))

by four spaces would be more readable.

+/* TODO: update this when telepathy-glib supports setting
+ * statuses at constructor time; until then, gabble_statuses
+ * leaks.
+ */

It's a once-per-process leak; is that so bad that it warrants an entry in FIXME.out? :)

+GabblePresenceId _conn_presence_get_type (GabblePresence *presence);

This is the only function in that header with a _ prefix.

+        GabblePresenceId presence_type = _conn_presence_get_type (presence);
+
+        switch (presence_type)
+          {
+          case TP_CONNECTION_PRESENCE_TYPE_AVAILABLE:

The TpConnectionPresenceType enum doesn't match the GabblePresenceId enum. And I don't understand this function body at all now. Do plugins have to assign IDs to presences that fall off the end of the GabblePresenceId enum or something?

Hit commit too early, more review to follow...

It would have been easier to review http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=commitdiff;h=0794ab632674651887d6eb200965aed5caf6cebe if it had been in smaller chunks. :(

+  if (list_name)
+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active",
+            '@', "name", list_name,
+          ')',
+        ')',
+        NULL);
+  else
+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active", ')',
+        ')',
+        NULL);

I think the following is probably clearer:

+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active",
+            '*', &active_node,
+          ')',
+        ')',
+        NULL);
+
+    if (list_name != NULL)
+      wocky_node_set_attribute (active_node, name, list_name);

I'm not overly bothered though. Maybe we should make wocky_stanza_build() accept NULL attribute values and just skip that attribute.

Also, since list_name can be NULL, the following will crash on Windows and other platforms where printf ("%s", NULL); crashes:

+  DEBUG ("Privacy status %s, backed by %s",
+    gabble_statuses[presence->status].name, list_name);

+  else if (!list_name && base->status != TP_CONNECTION_STATUS_CONNECTED)

list_name == NULL plz?


       conn_presence_signal_own_presence (self, NULL, &error);
       g_simple_async_result_complete_in_idle (result);
       g_object_unref (result);
+
+      goto OUT;

This leaks 'error'.

This patch seems to swap set_xep0126_invisible_cb and set_xep0186_invisible_cb — which is fine, though it would have been nicer as a separate patch. That said, the swap does show that the former needlessly casts user_data, while the latter just assigns it to a GSimpleAsyncResult *. I think they should be consistent, and maybe even use G_SIMPLE_ASYNC_RESULT().

+  g_assert (priv->privacy_statuses);

Why is this an assertion whereas earlier we had a g_return_if_fail() for it? Both should really use != NULL anyway.

I'm having a hard time convincing myself that this is guaranteed to be NULL or not NULL at the various points we assert about it. Maybe it could have a comment explaining its lifecycle, at least?


+      for (i = node_iter (iq); i; i = node_iter_next (i))

There's no reason to use the node_iter() macros in new code. In fact, this code should use Wocky's node iterators to only iterate across "list" children.

-          gchar *new_name = g_strdup_printf ("%s-gabble",
-              priv->invisible_list_name);
           g_free (priv->invisible_list_name);
-          priv->invisible_list_name = new_name;
+          priv->invisible_list_name = g_strdup ("invisible-gabble");

I think this code deliberately extended the previous name over and over, because it can be hit more than once. If there's a test case for both "invisible" and "invisible-gabble" existing but not satisfying is_valid_invisible_list(), this change is okay with me, though.


+  /* This relies on the fact the first entries in the statuses table
+   * are from base_statuses. If index to the statuses table is outside
+   * the base_statuses table, the status is provided by a plugin. */
+  if (status >= (sizeof (base_statuses) / sizeof (TpPresenceStatusSpec)))

Yeah, this is pretty scary. Also, G_N_ELEMENTS.


+static TpPresenceStatusSpec test_presences[] = {
+  { "testbusy", TP_CONNECTION_PRESENCE_TYPE_BUSY, TRUE, NULL, NULL, NULL },
+  { "testaway", TP_CONNECTION_PRESENCE_TYPE_AWAY, FALSE, NULL, NULL, NULL },
+  { NULL, 0, FALSE, NULL, NULL, NULL }
+};
+
+static GabblePluginPrivacyListMap privacy_list_map[] = {
+  { "testbusy", "test-busy-list" },
+  { NULL, NULL },
+};
+

What's the point of testaway here? Why is it legal for plugins to provide statuses that aren't linked to privacy lists?

+    elem_list=[]
+    for li in lists:
+        elem_list.append(elem('list', name=li))

elem_list = [elem('list', name=l) for l in lists]. Or even inline it into building the iq.

Thanks for the review.

(In reply to comment #1)
> But the point is moot because if this function just returned the array of
> statuses from the interface, gabble_plugin_loader_append_statuses() would be
> way shorter and easier to follow (untested).

Indeed, rewrote it based on your suggestion.

> Coding style dictates:
> 
> +      if (status != NULL)
> +        return status;
> 
> Ditto indentation of:
> 
> +  if (iface->privacy_list_map == NULL)
> +      return NULL;

Thanks, fixed.

> I think indenting the second argument of tp_strdiff() in
> 
> +      if (!tp_strdiff (list_name,
> +          iface->privacy_list_map[i].privacy_list_name))
> 
> by four spaces would be more readable.

It is indented by four spaces. Did you mean additional four (ie. 8) spaces?

> +/* TODO: update this when telepathy-glib supports setting
> + * statuses at constructor time; until then, gabble_statuses
> + * leaks.
> + */
> 
> It's a once-per-process leak; is that so bad that it warrants an entry in
> FIXME.out? :)

Hm, I hadn't known TODOs go into FIXME.out; It's a reminder what to do
once Tp-Glib supports it, and is actually more about the fact that
it'd be nicer to do it at constructor time, than the fact that it
leaks, but I wanted to document it. Removed the TODO keywword.

> +GabblePresenceId _conn_presence_get_type (GabblePresence *presence);
> 
> This is the only function in that header with a _ prefix.

Changed so it's not-prefixed. Originally I wanted to denote that it's "internal", but otoh, gabble is an application not a library (leaving aside the convenience library and the plugins), so everything's internal anyways.

> +        GabblePresenceId presence_type = _conn_presence_get_type (presence);
> +
> +        switch (presence_type)
> +          {
> +          case TP_CONNECTION_PRESENCE_TYPE_AVAILABLE:
> 
> The TpConnectionPresenceType enum doesn't match the GabblePresenceId enum. And

The enum type is wrong, I can't believe the compiler didn't catch it. Originally i tried to use GabblePresenceId directly, but then found out it's not enough and i have to fall back, so  I've copy/pasted the code, but didn't change the type appropriately. The type should be TpConnectionPresenceType.

> I don't understand this function body at all now. Do plugins have to assign IDs
> to presences that fall off the end of the GabblePresenceId enum or something?

There's never been any explicit ID assignment - it was always implicit, by making sure gabble_presences table (now base_presences) is in sync with the PresenceId enum. Which means, that all statuses added by plugins will have id after the GabblePresenceId (because they'll be after the base_statuses in the table generated by plugin loader).

But this function is not at all about that. This one is about knowing how to "render" the status. Since we don't have support for plugins doing it manually (maybe we'll have to, at some time, but we don't now, and I don't want to guess the future), this function has a fallback using the presence type id to guess.

So, e.g. if you have custom "at-work" status which is using "at-work-list" privacy list, but is actually a specialised kind of "busy", when you select it, gabble would do:
  1. set the privacy list to "at-work-list"
  2. signal own presence

For (2), it needs to know how to "render" at-work; since it's a specialised "busy" (the specialisation provided by "at-work-list"), we can put anything (except "unavailable") there, but to be semantically nice, we put "busy" there (based on its presence type).

Hope it's clearer now.

(In reply to comment #3)
> It would have been easier to review
> http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=commitdiff;h=0794ab632674651887d6eb200965aed5caf6cebe
> if it had been in smaller chunks. :(

Yes, definitely. I'm sorry for dumping this big chunk of diff on you. But since logic is intertwoven, I couldn't break it into smaller self-sufficient pieces, unless I rewrote it again, piece by piece... Since the code went through many iterations of code before being final, I couldn't just commit after each logical change. 

> +  if (list_name)
> +    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> +        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> +        '(', "query", ':', NS_PRIVACY,
> +          '(', "active",
> +            '@', "name", list_name,
> +          ')',
> +        ')',
> +        NULL);
> +  else
> +    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> +        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> +        '(', "query", ':', NS_PRIVACY,
> +          '(', "active", ')',
> +        ')',
> +        NULL);

That's old code, the only thing I touched here is changed invisible
list name / invisibility boolean flag with the list name.

> I think the following is probably clearer:
> 
> +    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> +        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
> +        '(', "query", ':', NS_PRIVACY,
> +          '(', "active",
> +            '*', &active_node,
> +          ')',
> +        ')',
> +        NULL);
> +
> +    if (list_name != NULL)
> +      wocky_node_set_attribute (active_node, name, list_name);

Wow, I didn't know wocky could do that. It's almost as if it were a whole new language :) Yes, that does look clearer to me too, changed the code accordingly.

> Also, since list_name can be NULL, the following will crash on Windows and
> other platforms where printf ("%s", NULL); crashes:
> 
> +  DEBUG ("Privacy status %s, backed by %s",
> +    gabble_statuses[presence->status].name, list_name);

Good catch, fixed.

> +  else if (!list_name && base->status != TP_CONNECTION_STATUS_CONNECTED)
> 
> list_name == NULL plz?

Fixed styling.

> +  g_assert (priv->privacy_statuses);
> 
> Why is this an assertion whereas earlier we had a g_return_if_fail() for it?
> Both should really use != NULL anyway.

Actually both should be asserts, since both of them are used only internally.

> I'm having a hard time convincing myself that this is guaranteed to be NULL or
> not NULL at the various points we assert about it. Maybe it could have a
> comment explaining its lifecycle, at least?

The idea is that privacy_statuses being != NULL signals we know the server has privacy list support. So, it's NULL up until we get response to our "get all privacy list" request (and the response is not error); then, it's not null until the object finalization.

I've added comments explaining this hopefully a bit better.


> +      for (i = node_iter (iq); i; i = node_iter_next (i))
> 
> There's no reason to use the node_iter() macros in new code. In fact, this code
> should use Wocky's node iterators to only iterate across "list" children.

I cargo-culted that from another part of conn-presence. Fixed.

> 
> -          gchar *new_name = g_strdup_printf ("%s-gabble",
> -              priv->invisible_list_name);
>            g_free (priv->invisible_list_name);
> -          priv->invisible_list_name = new_name;
> +          priv->invisible_list_name = g_strdup ("invisible-gabble");
> 
> I think this code deliberately extended the previous name over and over,
> because it can be hit more than once. If there's a test case for both
> "invisible" and "invisible-gabble" existing but not satisfying
> is_valid_invisible_list(), this change is okay with me, though.

Well, AFAICS, the code never had a feedback loop for verifying the list after it's beeen created. There's a possibility of having the code being executed more than once because of list_push_cb, but in that case the whole sequence starting with setup_* is ran, not just the verification.

(didn't change anything now; I can revert it, but I'd like to know whether my
assumption of how the old code works are correct, or whether I've missed something there)

> +  /* This relies on the fact the first entries in the statuses table
> +   * are from base_statuses. If index to the statuses table is outside
> +   * the base_statuses table, the status is provided by a plugin. */
> +  if (status >= (sizeof (base_statuses) / sizeof (TpPresenceStatusSpec)))
> 
> Yeah, this is pretty scary.

Not really, the index is never outside gabble_statuses (which is used elsewhere in the code). This is just the easiest way to determine whether the status is one of the base statuses, or one of the statuses provided by plugins.

> Also, G_N_ELEMENTS.

Changed.

> What's the point of testaway here? Why is it legal for plugins to provide
> statuses that aren't linked to privacy lists?

The point of testaway here is to check that plugins can add any status that could be added statically, too; including the one that is not backed by privacy lists, and in fact, is not settable by us. I think it's unneccessary to artificially restrict type of statuses that could be added, since:
  a) it's more work to restrict it than to support adding any status
  b) maybe in the future we'll want a second way for plugins to implement statuses, in which case it wouldn't be nice presence addition was coupled with privacy list linking

I think the two things are orthogonal and should be decoupled, so I did it that way.

> 
> +    elem_list=[]
> +    for li in lists:
> +        elem_list.append(elem('list', name=li))
> 
> elem_list = [elem('list', name=l) for l in lists]. Or even inline it into
> building the iq.

Fixed, thanks.

I've updated my branch with fixes based on your review.

(In reply to comment #4)
> The enum type is wrong, I can't believe the compiler didn't catch it.

For what it's worth, gcc 4.5 issues more warnings about suspicious uses of enums.

> This patch seems to swap set_xep0126_invisible_cb and set_xep0186_invisible_cb
> — which is fine, though it would have been nicer as a separate patch. That

I've swapped them because otherwise I'd have more invasive changes to set up privacy lists even if they're not used for invisibility, so this looked a bit cleaner to me. Unfortunately, I did that in the middle of reorganising the privacy list code, and couldn't decouple it into self-sufficient standalone patches.

> said, the swap does show that the former needlessly casts user_data, while the
> latter just assigns it to a GSimpleAsyncResult *. I think they should be
> consistent, and maybe even use G_SIMPLE_ASYNC_RESULT().

For some reason I skipped this during fixing up things. Patch to consistently use G_SIMPLE_ASYNC_RESULT pushed.

(In reply to comment #4)
> (In reply to comment #1)
> > I think indenting the second argument of tp_strdiff() in
> > 
> > +      if (!tp_strdiff (list_name,
> > +          iface->privacy_list_map[i].privacy_list_name))
> > 
> > by four spaces would be more readable.
> 
> It is indented by four spaces. Did you mean additional four (ie. 8) spaces?

Yes, sorry. I'd make it more obvious it's an argument to tp_strdiff(), I think. ☃

> > +/* TODO: update this when telepathy-glib supports setting
> > + * statuses at constructor time; until then, gabble_statuses
> > + * leaks.
> > + */
> > 
> > It's a once-per-process leak; is that so bad that it warrants an entry in
> > FIXME.out? :)
> 
> Hm, I hadn't known TODOs go into FIXME.out; It's a reminder what to do
> once Tp-Glib supports it, and is actually more about the fact that
> it'd be nicer to do it at constructor time, than the fact that it
> leaks, but I wanted to document it. Removed the TODO keywword.

Hmm, yeah, I guess I misunderstood the point of the TODO. Add a link to <https://bugs.freedesktop.org/show_bug.cgi?id=12896> and mention this on that bug? ☃

> The enum type is wrong, I can't believe the compiler didn't catch it.

Yeah, I'm pretty surprised too. 

> > I don't understand this function body at all now. Do plugins have to assign IDs
> > to presences that fall off the end of the GabblePresenceId enum or something?
> 
> There's never been any explicit ID assignment - it was always implicit, by
> making sure gabble_presences table (now base_presences) is in sync with the
> PresenceId enum. Which means, that all statuses added by plugins will have id
> after the GabblePresenceId (because they'll be after the base_statuses in the
> table generated by plugin loader).

Ah yes. I'd forgotten how exactly the presence mixin works.

> But this function is not at all about that. This one is about knowing how to
> "render" the status. Since we don't have support for plugins doing it manually
> (maybe we'll have to, at some time, but we don't now, and I don't want to guess
> the future), this function has a fallback using the presence type id to guess.

Yep, okay, this makes sense now. The duplication is a shame, but it seems unavoidable because the Telepathy enum doesn't have an equivalent of "chat". I guess we can leave it for now.

> (In reply to comment #3)
> > It would have been easier to review
> > http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=commitdiff;h=0794ab632674651887d6eb200965aed5caf6cebe
> > if it had been in smaller chunks. :(
> 
> Yes, definitely. I'm sorry for dumping this big chunk of diff on you. But since
> logic is intertwoven, I couldn't break it into smaller self-sufficient pieces,
> unless I rewrote it again, piece by piece... Since the code went through many
> iterations of code before being final, I couldn't just commit after each
> logical change.

Yep, no worries.

> That's old code, the only thing I touched here is changed invisible
> list name / invisibility boolean flag with the list name.

Ah yeah. Thanks for making it clearer anyway. :)

> > I'm having a hard time convincing myself that this is guaranteed to be NULL or
> > not NULL at the various points we assert about it. Maybe it could have a
> > comment explaining its lifecycle, at least?
> 
> The idea is that privacy_statuses being != NULL signals we know the server has
> privacy list support. So, it's NULL up until we get response to our "get all
> privacy list" request (and the response is not error); then, it's not null
> until the object finalization.
> 
> I've added comments explaining this hopefully a bit better.

Yep, much clearer. Thanks.

> > 
> > -          gchar *new_name = g_strdup_printf ("%s-gabble",
> > -              priv->invisible_list_name);
> >            g_free (priv->invisible_list_name);
> > -          priv->invisible_list_name = new_name;
> > +          priv->invisible_list_name = g_strdup ("invisible-gabble");
> > 
> > I think this code deliberately extended the previous name over and over,
> > because it can be hit more than once. If there's a test case for both
> > "invisible" and "invisible-gabble" existing but not satisfying
> > is_valid_invisible_list(), this change is okay with me, though.
> 
> Well, AFAICS, the code never had a feedback loop for verifying the list after
> it's beeen created. There's a possibility of having the code being executed
> more than once because of list_push_cb, but in that case the whole sequence
> starting with setup_* is ran, not just the verification.
> 
> (didn't change anything now; I can revert it, but I'd like to know whether my
> assumption of how the old code works are correct, or whether I've missed
> something there)

Yeah, you're right. This does work correctly; I was wrong about how the code worked. It's pretty confusing, but the diagram I rediscovered helped. :)

> > What's the point of testaway here? Why is it legal for plugins to provide
> > statuses that aren't linked to privacy lists?
> 
> The point of testaway here is to check that plugins can add any status that
> could be added statically, too; including the one that is not backed by privacy
> lists, and in fact, is not settable by us. I think it's unneccessary to
> artificially restrict type of statuses that could be added, since:
>   a) it's more work to restrict it than to support adding any status
>   b) maybe in the future we'll want a second way for plugins to implement
> statuses, in which case it wouldn't be nice presence addition was coupled with
> privacy list linking
> 
> I think the two things are orthogonal and should be decoupled, so I did it that
> way.

Okay, fair enough.

Having fixed the things labelled ☃, ship it!

(In reply to comment #7)
> Having fixed the things labelled ☃, ship it!

Thanks, fixed & merged!

(In reply to comment #8)
> (In reply to comment #7)
> > Having fixed the things labelled ☃, ship it!
> 
> Thanks, fixed & merged!

This branch introduced a regression, at least in my humble opinion.
When going invisible, I think XEP-0186 should have precedence over XEP-0126. Although this might not work too well with what you tried to achieve here.

An another note, ☃ with small enough typeface looks like a middle finger.