Bug 40734

I've updated the branch to depend on a version of telepathy-glib which actually includes the necessary generated code.

> static void _network_member_left(IdleMUCChannel *chan, TpHandle leaver, TpHandle actor, const gchar *message, TpChannelGroupChangeReason reason) {
> -        IdleMUCChannelPrivate *priv = IDLE_MUC_CHANNEL_GET_PRIVATE(chan);
> +        TpBaseChannel *base = TP_BASE_CHANNEL (chan);
> +TpBaseConnection *base_conn = tp_base_channel_get_connection (base);

Indentation needed.

Otherwise looks good.

+ send_command (self, cmd);
+ /* FIXME: don't return till we get a reply */
+ tp_svc_channel_interface_subject_return_from_set_subject (context);

Do you want me to fix this? I can, if you are terribly busy or something. :-)

(In reply to comment #3)
> + send_command (self, cmd);
> + /* FIXME: don't return till we get a reply */
> + tp_svc_channel_interface_subject_return_from_set_subject (context);
> 
> Do you want me to fix this? I can, if you are terribly busy or something. :-)

I had some free time today, and so I made the SetSubject method not return till we get a reply from the server and implemented a queueing system similar to the RequestContactInfo method so that successive SetSubject calls get queued.

I tried to fix the test case but the following changes don't work:

diff --git a/tests/twisted/channels/muc-channel-topic.py b/tests/twisted/channels/muc-channel-topic.py
index 0b7ccb3..4552895 100644
--- a/tests/twisted/channels/muc-channel-topic.py
+++ b/tests/twisted/channels/muc-channel-topic.py
@@ -37,10 +37,9 @@ def expect_and_check_can_set(q, channel, can_set):
         channel.Properties.Get(CHANNEL_IFACE_SUBJECT, 'CanSet'))
 
     if can_set:
-        # FIXME: this shouldn't return until the server gets back to us with
-        # RPL_TOPIC
-        channel.Subject2.SetSubject('what up')
-        e = q.expect('stream-TOPIC', data=[room, 'what up'])
+        call_async(q, channel.Subject2, 'SetSubject', 'what up')
+        q.expect_many(EventPattern('stream-TOPIC', data=[room, 'what up']),
+                      EventPattern('dbus-return', method='SetSubject'))
     else:
         call_async(q, channel.Subject2, 'SetSubject', 'boo hoo')
         q.expect('dbus-error', method='SetSubject',
@@ -90,8 +89,6 @@ def test(q, bus, conn, stream):
     assertEquals('', subject_props['Actor'])
 
     # Before the topic arrives from the server, check that our API works okay.
-    # FIXME: when we make SetSubject return asynchronously, this will need
-    # revising.
     test_can_set(q, stream, channel)
 
     # We're told the channel's topic, and (in a separte message) who set it and

Wondering what can be wrong.

Created attachment 52835 [details] [review]
MUCChannel: Queue SetSubject calls and return asynchronously

Should apply on top of Will's loyal-subjects branch.

Comment on attachment 52835 [details] [review]
MUCChannel: Queue SetSubject calls and return asynchronously

Review of attachment 52835 [details] [review]:
-----------------------------------------------------------------

AFAICT: the server returning an error isn't handled, and nor is the case where the server has truncated the topic. So in either of these cases, a SetSubject() call will never return, and the queued follow-up commands will never return either.

In Gabble, I made SetSubject() return:

• successfully when *any* subject update is received (the protocol doesn't really allow the client to match up the request and the response);
• an error when the server responds with an error (which is a little fiddly to match up, but possible).

Queueing up successive requests is nice to have, but if the channel closes without getting a reply from the server, those method calls will never return either.

(In reply to comment #6)
> Comment on attachment 52835 [details] [review] [review]
> MUCChannel: Queue SetSubject calls and return asynchronously
> 
> Review of attachment 52835 [details] [review] [review]:

Thanks for the review.

> -----------------------------------------------------------------
> 
> AFAICT: the server returning an error isn't handled, and nor is the case where
> the server has truncated the topic. So in either of these cases, a SetSubject()
> call will never return, and the queued follow-up commands will never return
> either.
> 
> In Gabble, I made SetSubject() return:
> 
> • successfully when *any* subject update is received (the protocol doesn't
> really allow the client to match up the request and the response);
> • an error when the server responds with an error (which is a little fiddly to
> match up, but possible).

Right. It was a quick hack. I will address these issues.
 
> Queueing up successive requests is nice to have, but if the channel closes
> without getting a reply from the server, those method calls will never return
> either.

Right. I am wondering if the queueing implementation in idle-contact-info.c will have similar problems or not. We free up the queue when the IdleConnection is finalized.

(In reply to comment #2)
> > static void _network_member_left(IdleMUCChannel *chan, TpHandle leaver, TpHandle actor, const gchar *message, TpChannelGroupChangeReason reason) {
> > -        IdleMUCChannelPrivate *priv = IDLE_MUC_CHANNEL_GET_PRIVATE(chan);
> > +        TpBaseChannel *base = TP_BASE_CHANNEL (chan);
> > +TpBaseConnection *base_conn = tp_base_channel_get_connection (base);
> 
> Indentation needed.

Thanks, fixed.

> Otherwise looks good.

I've merged my branch to master, but I'll retitle the bug for the returning-later issue.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-idle/issues/31.