Bug 32611

*** Bug 31470 has been marked as a duplicate of this bug. ***

This is a branch that implements the unmerged draft API I have proposed in #23151:

http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=shortlog;h=refs/heads/moar-room

These commits on top of "room" can obviously only be merged when said spec branch is merged.

> +  g_object_get (self, "target-id", &tmp, NULL);

Moar newlines to group parameter pairs (or pair in this case) please, as per /Style

> +  /* The target ID has already been checked so we don't care about the
> +   * return type of gabble_decode_jid, but it warns if it the result
> +   * is unused. */

g_assert (ok) then, so it's genuinely used and not just assigned to a useless variable? Or, explicitly cast the return to (void), which iirc silences that warning.

+    # Now a channel which already exists (any of the above) with
+    # RoomID set.
+    jid = 'booyakasha@conf.localhost'
+    call_async(q, conn.Requests, 'EnsureChannel', {
+            cs.CHANNEL_TYPE: cs.CHANNEL_TYPE_TEXT,
+            cs.TARGET_HANDLE_TYPE: cs.HT_ROOM,
+            cs.TARGET_ID: jid,
+            cs.ROOM_ROOM_ID: '',
+            })
+
+    e = q.expect('dbus-error', name=cs.INVALID_ARGUMENT, method='EnsureChannel')

I'm not convinced this should fail: the empty RoomID isn't really part of the identity of the channel, surely? We're requesting the same room as before, but this time providing the information that its name is meaningless, which we neglected to provide first time round.

That's all for /room.

/moar-room mostly looks good too:

+  gabble_svc_channel_interface_room_emit_subject_changed (chan,
+      g_value_get_string (g_value_array_get_nth (priv->subject, 0)),
+      g_value_get_string (g_value_array_get_nth (priv->subject, 1)),
+      g_value_get_int64 (g_value_array_get_nth (priv->subject, 2)),
+      room_subject_flags);

Perhaps have a helper function for "emit SubjectChanged, assuming the property has already been set to the new value"?

> +        check_room_props(chan, 'Testing', room + '/bob', 3, signal=s)

Please replace 3 with a constant (3 times)

> +    assertEquals(str(elem.children[0]), 'le lolz')

Please check that the first child is indeed <subject/>

(In reply to comment #3)
> Moar newlines to group parameter pairs (or pair in this case) please, as per
> /Style

Done.


> g_assert (ok) then, so it's genuinely used and not just assigned to a useless
> variable? Or, explicitly cast the return to (void), which iirc silences that
> warning.

Done.

> +    # Now a channel which already exists (any of the above) with
> +    # RoomID set.
> +    jid = 'booyakasha@conf.localhost'
> +    call_async(q, conn.Requests, 'EnsureChannel', {
> +            cs.CHANNEL_TYPE: cs.CHANNEL_TYPE_TEXT,
> +            cs.TARGET_HANDLE_TYPE: cs.HT_ROOM,
> +            cs.TARGET_ID: jid,
> +            cs.ROOM_ROOM_ID: '',
> +            })
> +
> +    e = q.expect('dbus-error', name=cs.INVALID_ARGUMENT,
> method='EnsureChannel')
> 
> I'm not convinced this should fail: the empty RoomID isn't really part of the
> identity of the channel, surely? We're requesting the same room as before, but
> this time providing the information that its name is meaningless, which we
> neglected to provide first time round.

Ah yes, I misread "Requests with conflicting TargetID and RoomID properties will fail with InvalidArgument."

I've fixed this in my room branch.

(In reply to comment #4)
> Perhaps have a helper function for "emit SubjectChanged, assuming the property
> has already been set to the new value"?

Done.

> Please replace 3 with a constant (3 times)

Done.

> > +    assertEquals(str(elem.children[0]), 'le lolz')

Done.

I rebased moar-room back on top of room.

> -  /* The target ID has already been checked

I'd prefer a similar comment reinstated above the g_assert, to explain why it's safe to do so.

> +      g_assert (gabble_decode_jid (target_id, &a, NULL, NULL));

Assertion with side effects detected! Deserves a similar comment, perhaps "/* JIDs that are handles must already be valid */".

Both of those also apply to the block below, for Server.

I'd prefer @a and @b to be called target_room and target_server, or something.

>      # Now a channel which already exists (any of the above) with
> -    # Server set.
> +    # a conflicting Server set.
...
> +            cs.ROOM_ROOM_ID: 'happynewyear',

Copy-paste error in the comment, but bonus penguin points for your choice of room ID (and happy new year to you too).

/moar-room looks good.

(In reply to comment #7)
> > -  /* The target ID has already been checked
> 
> I'd prefer a similar comment reinstated above the g_assert, to explain why it's
> safe to do so.

Done.

> > +      g_assert (gabble_decode_jid (target_id, &a, NULL, NULL));
> 
> Assertion with side effects detected! Deserves a similar comment, perhaps "/*
> JIDs that are handles must already be valid */".

Done.

> I'd prefer @a and @b to be called target_room and target_server, or something.

Done.

> Copy-paste error in the comment, but bonus penguin points for your choice of
> room ID (and happy new year to you too).

Fixed.

All pushed to room as usual.

I also just updated moar-room with the latest changes from bug #23151.

+          g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+              "TargetID and RoomID conflict.");

I think it'd be kinder to give more information. How about: "TargetID's node part (%s) doesn't match RoomID (%s)". Ditto the one below.

(In reply to comment #8)
> > > +      g_assert (gabble_decode_jid (target_id, &a, NULL, NULL));
> > 
> > Assertion with side effects detected! Deserves a similar comment, perhaps "/*
> > JIDs that are handles must already be valid */".
> 
> Done.

No it hasn't been done for the conflict-checking section. You added comments above the assertions, but didn't make them side-effect-free. Surely it must be possible to avoid repeatedly decoding the JID and asserting about it?

Do we really have to make handle_text_channel_request() over three hundred lines long? I appreciate that it was quite long before you started, so maybe this is a job for a cleanup branch later.

(In reply to comment #10)
> I think it'd be kinder to give more information. How about: "TargetID's node
> part (%s) doesn't match RoomID (%s)". Ditto the one below.

Done.

> (In reply to comment #8)
> > > > +      g_assert (gabble_decode_jid (target_id, &a, NULL, NULL));
> > > 
> > > Assertion with side effects detected! Deserves a similar comment, perhaps "/*
> > > JIDs that are handles must already be valid */".
> > 
> > Done.
> 
> No it hasn't been done for the conflict-checking section. You added comments
> above the assertions, but didn't make them side-effect-free.

Oh, right, I misunderstood what Simon said. Done.

> Surely it must be possible to avoid repeatedly decoding the JID and
> asserting about it?

Maybe. Suggestions?

(In reply to comment #11)
> (In reply to comment #10)
> > I think it'd be kinder to give more information. How about: "TargetID's node
> > part (%s) doesn't match RoomID (%s)". Ditto the one below.
> 
> Done.

Looks good.

> > (In reply to comment #8)
> > > > > +      g_assert (gabble_decode_jid (target_id, &a, NULL, NULL));
> > > > 
> > > > Assertion with side effects detected! Deserves a similar comment, perhaps "/*
> > > > JIDs that are handles must already be valid */".
> > > 
> > > Done.
> > 
> > No it hasn't been done for the conflict-checking section. You added comments
> > above the assertions, but didn't make them side-effect-free.
> 
> Oh, right, I misunderstood what Simon said. Done.

Cool.

> > Surely it must be possible to avoid repeatedly decoding the JID and
> > asserting about it?
> 
> Maybe. Suggestions?

Re-decode the jid once, just before the two blocks of checks about it? This isn't really a big deal, it just looks a bit messy.

Cheers, I've merged my room branch. I'll leave this bug open for reviewing the Room interface changes in my other branch.

I left my room branch on my personal repository to make reviewing moar-room eaiser. Go me.

(In reply to comment #13)
> Cheers, I've merged my room branch. I'll leave this bug open for reviewing the
> Room interface changes in my other branch.
> 
> I left my room branch on my personal repository to make reviewing moar-room
> eaiser. Go me.

Oh turns out smcv ++'d moar-room ages ago. So this really is just blocking on the spec being merged in bug #23151.

Okay, so…

I have a crapload of patches on top of Jonny's branch, at http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/log/?h=moar-room. It branches from Jonny's at the branch named 'hi-jonny': I've reviewed everything before that.

It depends on a branch of telepathy-glib, which I have split into pieces to make it easier to review.

First, http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=dbus-properties-mixin-stuff adds support for emitting PropertiesChanged to TpDBusPropertiesMixin.

Then the moar-room branch of Gabble depends on that (plus a snapshot of the spec from bug 23151). Along the way I write a prototype for TpBaseRoomConfig in Gabble. I then move that to tp-glib: http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=bye-bye-properties

I'm sorry the Gabble branch is so ridiculously big. Compared to Jonny's branch:

 19 files changed, 1344 insertions(+), 1741 deletions(-)

And compared to master:

 extensions/Channel_Interface_Room.xml    |  373 -------
 extensions/Makefile.am                   |    2 +-
 extensions/all.xml                       |    1 -
 src/Makefile.am                          |    3 +
 src/muc-channel.c                        | 1705 +++++++++++++-----------------
 src/muc-channel.h                        |   29 +-
 src/muc-factory.c                        |   38 +-
 src/room-config.c                        |  101 ++
 src/room-config.h                        |   61 ++
 src/server-tls-manager.c                 |    2 -
 tests/twisted/Makefile.am                |    2 +-
 tests/twisted/constants.py               |   12 +-
 tests/twisted/muc/room-config.py         |  388 +++++++
 tests/twisted/muc/room.py                |   40 +-
 tests/twisted/muc/subject.py             |  147 ++-
 tests/twisted/muc/test-muc-properties.py |  102 --
 tests/twisted/muc/test-muc.py            |   38 +-
 tests/twisted/mucutil.py                 |    6 +-
 tests/twisted/ns.py                      |    1 +
 19 files changed, 1453 insertions(+), 1598 deletions(-)

It removes all support for Telepathy.Properties from MucChannel. Pretty much the future!

I should mention how I've tested the Gabble branch. I haven't hooked Subject2 up in Empathy, let alone Room2 or RoomConfig1. But I tested that the lack of support for TpProperties doesn't crash Empathy (which it did, kinda, which I've fixed). And I've checked (by changing the subject in Pidgin) that the subject change notification works, and I've tested tweaking the room config using D-Feet.

dbus-properties-mixin-stuff --> In doc of tp_dbus_properties_mixin_set(): "If get would return a D-Bus error, sets @error and returns %FALSE" --> s/get/set/ AFAIK.

I've rebased that branch on master and what's left of it seems good.

Above comments are for http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=dbus-properties-mixin-stuff

Did not look at the 2 other branches yet.

+ if (priv->must_provide_password == !!must_provide_password)

lolz

I highly endorse commits like cdf60cb7bbec25d1.

Maybe you should return from SetSubject if the channel gets disposed? Does that DBusGMethodInvocation object get cleared up? Oh I now see you return in close_channel(). Does that always get called? I guess it does.

I just opened bug #40798.

Why are we using conn_util_send_iq_async which appears to only be a small wrapper around wocky_porter_send_iq_async? The error parsing seems pretty small. Maybe it's worth it. You clearly think so.

Nit-picking: why are the type macros underneath the method protoypes in room-config.h? I know it doesn't really matter but literally no other GObject library does it, no? It looks weird.

What's the point in copying the hash table in validate_properties? If it's valid then it returns a complete copy of the hash table given to UpdateConfiguration. If it's not valid, validate_properties returns NULL. Why don't you just make it return a boolean and then use the one given to the hash table? Ohh, is it so you can use the hash table after the function returns (but before the dbus method call returns) and you can't trust dbus-glib not to free the hash table even if you ref it? I guess that's fine then, but a comment explaining this would be nice.

I'm not sure this UpdateConfiguration API is great for protocols where setting property can succeed and another fail in the same operation. This seems to think all will succeed or all will fail. I don't have this protocol or even a solution in mind for this.

+ priv->properties_being_updated = validated_properties;

You're banking on the hash table which is owned by GabbleRoomConfig being around during the update_configuration call which might not hold if it's moved to tp-glib. I'd be happier if you reffed it tbh.

Question: why do you create a new GSimpleAsyncResult in gabble_room_config_update_configuration_async? Why don't you just pass everything through to gabble_muc_channel_update_configuration_async?

Well, I hope I've done a good enough job of reviewing this. Commits like 4308d550320 are pretty hard to review so that you are confident "it's fine".

Pushed a couple of patches to each branch fixing your comments. (I actually realised an untested aspect of the subject code too, so there's a patch fixing that as well.)

(In reply to comment #19)
> + if (priv->must_provide_password == !!must_provide_password)
> 
> lolz

I'm going to defend this on the grounds that it makes == on gbooleans safe.

> I highly endorse commits like cdf60cb7bbec25d1.

Thanks!

> Maybe you should return from SetSubject if the channel gets disposed? Does that
> DBusGMethodInvocation object get cleared up? Oh I now see you return in
> close_channel(). Does that always get called? I guess it does.

It does. I was a bit suspicious but it seems to be correct.

> I just opened bug #40798.

Yes.

> Why are we using conn_util_send_iq_async which appears to only be a small
> wrapper around wocky_porter_send_iq_async? The error parsing seems pretty
> small. Maybe it's worth it. You clearly think so.

It saves fishing the porter out of the Gabble connection, and doing the error parsing in the callback: porter_send_iq_finish() returns TRUE if we got a reply which happened to be an error, whereas conn_send_iq_finish() returns FALSE in that case.

I do think it's worth not typing that stuff out over and over. :)

> Nit-picking: why are the type macros underneath the method protoypes in
> room-config.h? I know it doesn't really matter but literally no other GObject
> library does it, no? It looks weird.

Straw poll of telepathy-glib:

• base-connection.h has them in the middle of the prototypes \o/;
• contact.h has them above the prototypes but below the typedefs;
• contact-search-result.h has them above the typedefs and prototypes;
• media-interfaces.h has them in between the typedefs and the prototypes.

So yeah, below typedefs and structs, but above prototypes, seems to be most common. I can change this if you like. I put them at the bottom because I think they're ugly and wanted them out of the way. Would you rather they were above or below the enum definition?

I picked above.

> What's the point in copying the hash table in validate_properties? If it's
> valid then it returns a complete copy of the hash table given to
> UpdateConfiguration. If it's not valid, validate_properties returns NULL. Why
> don't you just make it return a boolean and then use the one given to the hash
> table? Ohh, is it so you can use the hash table after the function returns (but
> before the dbus method call returns) and you can't trust dbus-glib not to free
> the hash table even if you ref it? I guess that's fine then, but a comment
> explaining this would be nice.

The two hash tables have different keys. The keys passed over D-Bus are strings; the keys on the table built by validate_properties are elements of the TpBaseRoomConfigProperty enum.

I'll document validate_properties to make the difference clear.

> I'm not sure this UpdateConfiguration API is great for protocols where setting
> property can succeed and another fail in the same operation. This seems to
> think all will succeed or all will fail. I don't have this protocol or even a
> solution in mind for this.

That's true. IRC has this issue, for instance. FWIW the old API had the same issue. :) What else should we do? Only allow changing one field at a time? Represent partial success with a successful return with a dictionary of what failed? (I really hate APIs like the latter.)

With the status quo, if some, but not all, changes were applied, the properties which did get changed should have their values updated in any case. Well-behaved CMs could prioritise more crucial settings over less crucial ones: so like, update the password before disabling invite-only, or whatever.

> + priv->properties_being_updated = validated_properties;
> 
> You're banking on the hash table which is owned by GabbleRoomConfig being
> around during the update_configuration call which might not hold if it's moved
> to tp-glib. I'd be happier if you reffed it tbh.

Good point.

> Question: why do you create a new GSimpleAsyncResult in
> gabble_room_config_update_configuration_async? Why don't you just pass
> everything through to gabble_muc_channel_update_configuration_async?

Because TpBaseRoomConfig expects the source object in the callback to be a TpBaseRoomConfig subclass.

The longer answer is that I was planning to move all the room config code into room-config.c in Gabble, but didn't bother.

> Well, I hope I've done a good enough job of reviewing this. Commits like
> 4308d550320 are pretty hard to review so that you are confident "it's fine".

I appreciate that. I'm pretty confident they're fine.

Your extra patches look fine.

(In reply to comment #20)
> (In reply to comment #19)
> > I'm not sure this UpdateConfiguration API is great for protocols where setting
> > property can succeed and another fail in the same operation. This seems to
> > think all will succeed or all will fail. I don't have this protocol or even a
> > solution in mind for this.
> 
> That's true. IRC has this issue, for instance. FWIW the old API had the same
> issue. :) What else should we do? Only allow changing one field at a time?
> Represent partial success with a successful return with a dictionary of what
> failed? (I really hate APIs like the latter.)
> 
> With the status quo, if some, but not all, changes were applied, the properties
> which did get changed should have their values updated in any case.
> Well-behaved CMs could prioritise more crucial settings over less crucial ones:
> so like, update the password before disabling invite-only, or whatever.

I'm not sure what the solution should be. Perhaps we should talk about this at some point.

Merged dbus-properties-mixin-stuff with the get vs. set typo fixed; bringing the remaining branches up to scratch …

(In reply to comment #22)
> Merged dbus-properties-mixin-stuff with the get vs. set typo fixed; bringing
> the remaining branches up to scratch …

tp-glib bits released in 0.15.8.

Merged the Gabble bit, having updated it to depend on the brand spanking new tp-glib. It'll be in 0.13.7.