Bug 50828

Summary: Remove Tubes code
Product: Telepathy Reporter: Jonny Lamb <jonny.lamb>
Component: salutAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~jonny/telepathy-salut/log/?h=cheerio-tubes
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on: 32612, 48210    
Bug Blocks: 47760    

Description Jonny Lamb 2012-06-07 06:09:41 UTC
We really need to do this.

Here's a patch. I threw the 'only expose the text muc channel on the bus when necessary' stuff on top.
Comment 1 Will Thompson 2012-07-21 10:44:27 UTC
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?
Comment 2 Simon McVittie 2012-07-23 10:43:19 UTC
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.
Comment 3 Jonny Lamb 2012-08-03 09:42:27 UTC
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.
Comment 4 Simon McVittie 2012-08-14 15:20:37 UTC
Yeah, looks good, when you've added a dependency on the telepathy-glib
that closes Bug #48210.
Comment 5 Jonny Lamb 2012-08-28 13:32:48 UTC
┏━┓┏┳┓┏━╸   ╺┳┓┏━┓┏┓╻┏━╸
┃ ┃┃┃┃┃╺┓    ┃┃┃ ┃┃┗┫┣╸ 
┗━┛╹ ╹┗━┛   ╺┻┛┗━┛╹ ╹┗━╸

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.