I am reviewing this because I would like to debug bug 49491 but I can't build Salut due to

tubes-manager.c:388:5: error: 'tp_channel_manager_emit_new_channels' is deprecated (declared at /usr/include/telepathy-1.0/telepathy-glib/channel-manager.h:139) [-Werror=deprecated-declarations]

and of course this code actually does want to emit two channels at once. So…

Checking cls->target_handle_type in one class to see whether this is actually an instance of the MUC subclass and changing behaviour accordingly smells weird to me, but I'm sure it smelled weird to you too.


 struct _SalutTubeDBusClass {
-  GObjectClass parent_class;
+  TpBaseChannelClass parent_class;
 
   TpDBusPropertiesMixinClass dbus_props_class;

You can drop the TpDBusPropertiesMixinClass if you want (but you don't have to, obviously).

<http://cgit.freedesktop.org/~jonny/telepathy-salut/commit/?h=cheerio-tubes&id=0b62569>:

@@ -692,6 +693,12 @@ salut_tubes_manager_foreach_channel (TpChannelManager *manager,
     salut_tubes_channel_foreach (SALUT_TUBES_CHANNEL (chan), foreach,
         user_data);
   }
+
+  g_hash_table_iter_init (&iter, priv->tubes);
+  while (g_hash_table_iter_next (&iter, NULL, &value))
+  {
+    foreach (TP_EXPORTABLE_CHANNEL (value), user_data);
+  }
 }

Le coding style.

+  g_hash_table_iter_init (&iter, priv->tubes);
+  while (g_hash_table_iter_next (&iter, NULL, &value))
+    {
+      SalutTubeIface *tube = value;
+      gboolean match = FALSE;
+
+      gchar *channel_type, *channel_service;
+      TpHandle channel_handle;
+
+      g_object_get (tube,
+          "channel-type", &channel_type,
+          "handle", &channel_handle,
+          "service", &channel_service,
+          NULL);
+
+      if (!tp_strdiff (type, channel_type)
+          && handle == channel_handle
+          && !tp_strdiff (service, channel_service))
+        match = FALSE;
+
+      g_free (channel_type);
+      g_free (channel_service);
+
+      if (match)
+        return tube;
+    }

match is never TRUE here. I think this means there is no test for ensuring a tube.

+  guint out;
+
+  /* probably totally overkill */
+  do
+    {
+      out = g_random_int_range (0, G_MAXINT);
+    }
+  while (g_hash_table_lookup (priv->tubes,
+          GUINT_TO_POINTER (out)) != NULL);

I had a moment of panic about G_MAXINT vs. G_MAXUINT. I will just note that g_random_int_range takes a gint32 so technically you want G_MAXINT32 (to protect against the unlikely event that sizeof(int) > 32).

   /* Temporarily disabled since the implementation is incomplete. */
lol   ^^^^^^^^^^^

http://cgit.freedesktop.org/~jonny/telepathy-salut/commit/?h=cheerio-tubes&id=a54df63

I don't understand this patch … It adds a hash table field which is always NULL? I guess you ended up using it later. NBD.

-    # TODO: get rid of Tubes and this'll start making more sense
-    return
+#    yours, ensured_path, ensured_props = conn.Requests.EnsureChannel(
+#            { CHANNEL_TYPE: CHANNEL_TYPE_STREAM_TUBE,
+#              TARGET_HANDLE_TYPE: HT_ROOM,
+#              TARGET_HANDLE: handle,
+#              STREAM_TUBE_SERVICE: 'loldongs',
+#              })
 
-    assert not yours
-    assert ensured_path == path2, (ensured_path, path2)
+#    assert not yours
+#    assert ensured_path == tube_path, (ensured_path, tube_path2)

Does this end up getting removed?

As a general remark which is easy to say in hindsight: it might have been nice to keep the code that moved from tubes-channel.c into muc-channel.c in a separate file, muc-channel-tubes.c or something.

http://cgit.freedesktop.org/~jonny/telepathy-salut/commit/?h=cheerio-tubes&id=5e54b74

+    # request tube, incoming message, assert text channel appears
+#    exec_test(test_message)
+
+    # request text & tube, close text, incoming message, assert text
+    # reappears with correct props
+#    exec_test(test_requested_message)

Probably you intended to uncomment these?

I reviewed similar code in Gabble (Bug #32612) and it's reassuring to see how similar your review is :-)

The same comments as on Bug #32612 probably apply. I didn't review this one yet on the basis that it's presumably remarkably similar code.

Will, if you want Salut prioritized over Gabble in this department, that order would be fine too.

> I would like to debug bug 49491 but I can't build
> Salut due to
>
> tubes-manager.c:388:5: error: 'tp_channel_manager_emit_new_channels' is
> deprecated (declared at
> /usr/include/telepathy-1.0/telepathy-glib/channel-manager.h:139)
> [-Werror=deprecated-declarations]

I think this means we want

AC_DEFINE(TP_VERSION_MIN_REQUIRED, TP_VERSION_0_18,
    [Ignore post 0.18 deprecations])

in Salut's configure.ac until this has been fixed.

Okay I think I fixed everything in the gabble review. Also:

(In reply to comment #1)
> You can drop the TpDBusPropertiesMixinClass if you want (but you don't have to,
> obviously).

I didn't... sorry...

> -    # TODO: get rid of Tubes and this'll start making more sense
> -    return
> +#    yours, ensured_path, ensured_props = conn.Requests.EnsureChannel(
> +#            { CHANNEL_TYPE: CHANNEL_TYPE_STREAM_TUBE,
> +#              TARGET_HANDLE_TYPE: HT_ROOM,
> +#              TARGET_HANDLE: handle,
> +#              STREAM_TUBE_SERVICE: 'loldongs',
> +#              })
> 
> -    assert not yours
> -    assert ensured_path == path2, (ensured_path, path2)
> +#    assert not yours
> +#    assert ensured_path == tube_path, (ensured_path, tube_path2)
> 
> Does this end up getting removed?

No. I forgot to add a TODO before which I've now done. Perhaps I should fix the TODO, or something; there's lots of other stuff to do though.

> Probably you intended to uncomment these?

I removed those tests as they're not really testing much, and are much more annoying to implement in salut. Oh well..

Branch updated.

Yeah, looks good, when you've added a dependency on the telepathy-glib
that closes Bug #48210.

┏━┓┏┳┓┏━╸   ╺┳┓┏━┓┏┓╻┏━╸
┃ ┃┃┃┃┃╺┓    ┃┃┃ ┃┃┗┫┣╸ 
┗━┛╹ ╹┗━┛   ╺┻┛┗━┛╹ ╹┗━╸