[Bug 50828] Remove Tubes code

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2012-07-21 10:44:27 PDT ---
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?

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