Bug 22768

Review of Jonny's branch:

util.c has code that could do the namespace lookup semi-properly. That
is to say: lm_message_node_has_namespace() does look like it would work,
except that lm_message_node_get_child_with_namespace() strips off the
prefix if there is one. Soo.... I'd vaguely prefer that we fix that and
then just use it in muc-factory.

You need to find out what Google Talk returns if you disco #pmuc-v1, and
hook that up in connection_iq_disco_cb(). Currently, your branch
advertises the pmuc-v1 bundle, but if another client discos that bundle
it'll return an error.

You should remove the explicit implementation of TpSvcConnection from
connection.c: it's already implemented by TpBaseConnection, and now
we're not overriding any of the methods there's no need to re-implement
it at all.

Using conn as the context for TpDynamicHandleRepo feels a bit like a
hack. Maybe file a bug on tp-glib wondering about a better way?

Can't most of gabble_normalize_room be replaced by gabble_decode_jid() ?
Though it seems a bit weird to call gabble_encode_jid() — in
_gabble_connection_get_canonical_room_name() — and then immediately call
gabble_decode_jid().  :)  I suppose that using gabble_decode_jid() would
make the error messages slightly less useful, since it doesn't say
what's wrong when decoding fails. But they're just D-Bus error messages,
so not really that useful.

_gabble_connection_get_canonical_room_name() should have its leading
underscore removed.

Finally, I don't think you can really say this fixes the pmuc bug: we
don't really support creating them. :P

Since fixing Bug #21152 is a prerequisite for this one, and has implications for regression tests, perhaps Jonny (or someone) could construct a branch that does that but doesn't do the bonus pmuc stuff, and we could deal with that first?

(Possibly cherry-picking 8b657e3e3a8 would *be* that branch - I haven't looked at it in detail.)

(In reply to comment #2)
> Since fixing Bug #21152 is a prerequisite for this one, and has implications
> for regression tests, perhaps Jonny (or someone) could construct a branch that
> does that but doesn't do the bonus pmuc stuff, and we could deal with that
> first?

It'd be better to fix the review comments and merge the branch. The only "bonus pmuc stuff" in the branch is adding a single cap bundle. It wouldn't fix this bug, but that's blocked by figuring out how the Telepathy API looks.

FYI, I've fixed/looked at the following comments in my branch:

(In reply to comment #1)
> You should remove the explicit implementation of TpSvcConnection from
> connection.c: it's already implemented by TpBaseConnection, and now
> we're not overriding any of the methods there's no need to re-implement
> it at all.

Done.

> Using conn as the context for TpDynamicHandleRepo feels a bit like a
> hack. Maybe file a bug on tp-glib wondering about a better way?

I'm unconvinced. You're probably right, but surely the GabbleConnection *is* the "context" of the jid normalization. Looking at the docs for TpDynamicHandleRepoNormalizeFunc, this seems to support my claim?

> Can't most of gabble_normalize_room be replaced by gabble_decode_jid() ?
> Though it seems a bit weird to call gabble_encode_jid() — in
> _gabble_connection_get_canonical_room_name() — and then immediately call
> gabble_decode_jid().  :)  I suppose that using gabble_decode_jid() would
> make the error messages slightly less useful, since it doesn't say
> what's wrong when decoding fails. But they're just D-Bus error messages,
> so not really that useful.

Done.

> _gabble_connection_get_canonical_room_name() should have its leading
> underscore removed.

Done.

> Finally, I don't think you can really say this fixes the pmuc bug: we
> don't really support creating them. :P

Aww, okay.

The remaining issues which I haven't gotten around to yet:

> util.c has code that could do the namespace lookup semi-properly. That
> is to say: lm_message_node_has_namespace() does look like it would work,
> except that lm_message_node_get_child_with_namespace() strips off the
> prefix if there is one. Soo.... I'd vaguely prefer that we fix that and
> then just use it in muc-factory.

I should write a test for this as I'm getting a little confused reading that code.

> You need to find out what Google Talk returns if you disco #pmuc-v1, and
> hook that up in connection_iq_disco_cb(). Currently, your branch
> advertises the pmuc-v1 bundle, but if another client discos that bundle
> it'll return an error.

Apparently, gajim is the way forward here. I should get on this.

We can, and should, merge the parts Jonny has implemented so far (once they pass review) without having to finish implementing or even designing PMUC-joining.

My current proposal for an API to open new PMUCs, or upgrade a 1-1 conversation into a PMUC, is part of Bug #24906 (conference calls / grand unified conferring interface), with some more specific use-cases for XMPP (and MSN) explained in Bug #24939 (upgrading 1-1 to multi-user).

(In reply to comment #5)
> We can, and should, merge the parts Jonny has implemented so far (once they
> pass review) without having to finish implementing or even designing
> PMUC-joining.

That's exactly what we had all already decided. I just haven't completely fixed the branch since Will's comments.

Okay, I finally updated my branch.

Since the last update, I fixed the problem with LM not chaining up its parent's namespace, another test and just acked the disco for the pmuc-v1 cap, because that's what gmail appear to do.

Reviews welcome!

http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=9ee14ac89a
> /* LM doesn't chain up to its parent's namespaces. boo hoo */

I initially wrote: "The first hunk of this commit seems to be a behaviour change: we'll now consider <message xmlns="jabber:client"><x><invite/></x></message> to be a MUC node, which isn't right. Please revert it or explain why." ... but it's reverted later in the branch, so never mind.

I'd prefer it if Bug #22456 was mentioned in the "subject" of the commit that fixed it; our usual style is "fd.o#xyz: detect badgers even if a snake is present".

http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=d5ce260581f23

"one()" and "two()" are poor test names! test_inherited_ns() and test_uninherited_prefix_ns() perhaps?

http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=9ef46bd71a26

I'd prefer a comment /* send an empty reply, matching GMail's behaviour */ just before calling _gabble_connection_send_or_complain().

(In reply to comment #8)
> I'd prefer it if Bug #22456 was mentioned in the "subject" of the commit that
> fixed it; our usual style is "fd.o#xyz: detect badgers even if a snake is
> present".

Amended those commit messages.

> http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=d5ce260581f23
> 
> "one()" and "two()" are poor test names! test_inherited_ns() and
> test_uninherited_prefix_ns() perhaps?

Appended a new commit fixing this.

> http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=9ef46bd71a26
> 
> I'd prefer a comment /* send an empty reply, matching GMail's behaviour */ just
> before calling _gabble_connection_send_or_complain().

Appended a new commit fixing this.

Looks good. Please ship it, but leave this bug open til we have the ability to join those rooms in a non-crap way, and say "fd.o#22768 (partial): ..." in NEWS.

Er, actually, leaving this for wjt to decide since it's against 0.8; Jonny's going to forward-port the applicable bits to 0.9.

(In reply to comment #11)
> Jonny's going to forward-port the applicable bits to 0.9.

Done and merged.

Merged Jonny's branch to 0.8: http://git.collabora.co.uk/?p=telepathy-gabble.git;a=commit;h=9454be81a8e2395c8b6d808af9701280b6da12a7

Clearing patch keyword etc. accordingly, and reassigning to Danni who's working on supporting these more fully with the Conference interface.

The branch
http://git.collabora.co.uk/?p=user/danni/telepathy-gabble.git;a=shortlog;h=refs/heads/pmuc-conf
is a WIP implementation of using the Conference interface to request a PMUC
channel.

As it stands, you can request a PMUC with InitialChannels and
InitialInvitee{Handles,IDs} (here is a WIP Empathy branch that implements this,
but it requires major UI work still:
http://git.collabora.co.uk/?p=user/danni/empathy.git;a=shortlog;h=refs/heads/pmuc)

At the moment, all PMUCs are created on the groupchat.google.com server, this
is non-ideal. The GMail client can't accept invites for non Google-PMUC MUCs
(it just doesn't work), we need a metric for deciding when to create a PMUC on
the Google server. Also, without libuuid we can't generate a valid JID.

MUC channels in Gabble do not yet implement the Conference interface
themselves. This will be easy enough for MUCs that were created via a
Conference request, but for incoming channels, it will not be easy to determine
what incoming channels are upgrades of what conversations.

You request { TargetHandleType: None }, but get back { TargetHandleType: Room,
TargetID: meaningless-identifier }, is this an issue? Should we consider
further spec work for anonymous rooms? Something like: { TargetHandleType:
None, TargetID: unset, RealTargetHandleType: Room, RealTargetID:
meaningless-identifer } ?

Created attachment 32051 [details] [review]
collated WIP patch for requesting PMUCs

(In reply to comment #14)
> Also, without libuuid we can't generate a valid JID.

I wouldn't object to upgrading libuuid to a mandatory dependency, tbh.

> MUC channels in Gabble do not yet implement the Conference interface
> themselves. This will be easy enough for MUCs that were created via a
> Conference request, but for incoming channels, it will not be easy to determine
> what incoming channels are upgrades of what conversations.

If there's a <continue/> in the invitation, it's a continuation of the 1-1 conversation (if any) with the inviter.

> You request { TargetHandleType: None }, but get back { TargetHandleType: Room,
> TargetID: meaningless-identifier }, is this an issue?

I don't think the request for upgrade should carry a target handle type at all; it should just be as described in the spec <http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Channel.Interface.Conference.DRAFT.html>. Specifically, CreateChannel({...ChannelType: ...Text, ...InitialChannels: [C1]}) should be enough parameters.

If you're naming a specific chatroom, *then* you need to specify THT=Room along with one of TargetHandle or TargetID.

In practice this probably means the MUC manager should have these requestable channel classes:

* fixed={ChannelType: Text}, allowed=[InitialChannels]
* fixed={ChannelType: Text, TargetHandleType: Room}, allowed=[InitialChannels, TargetHandle, TargetID]

> > MUC channels in Gabble do not yet implement the Conference interface
> > themselves. This will be easy enough for MUCs that were created via a
> > Conference request, but for incoming channels, it will not be easy to determine
> > what incoming channels are upgrades of what conversations.
> 
> If there's a <continue/> in the invitation, it's a continuation of the 1-1
> conversation (if any) with the inviter.

It doesn't seem there is, contrary to what their own spec document implies.

> In practice this probably means the MUC manager should have these requestable
> channel classes:
> 
> * fixed={ChannelType: Text}, allowed=[InitialChannels]
> * fixed={ChannelType: Text, TargetHandleType: Room}, allowed=[InitialChannels,
> TargetHandle, TargetID]

Ok. This seems workable, because then it doesn't matter what TargetHandleType is returned. How do you propose we deal with the case of anonymous MUCs where the TargetID isn't really useful to display as the room name?

(In reply to comment #17)
> > > MUC channels in Gabble do not yet implement the Conference interface
> > > themselves. This will be easy enough for MUCs that were created via a
> > > Conference request, but for incoming channels, it will not be easy to determine
> > > what incoming channels are upgrades of what conversations.
> > 
> > If there's a <continue/> in the invitation, it's a continuation of the 1-1
> > conversation (if any) with the inviter.
> 
> It doesn't seem there is, contrary to what their own spec document implies.

Obviously for clients that _do_ supply <continue/> in their invite (as Gabble should be made to do), we should support that, but my question was how to handle clients that do not?

(In reply to comment #14)

> MUC channels in Gabble do not yet implement the Conference interface
> themselves. This will be easy enough for MUCs that were created via a
> Conference request, but for incoming channels, it will not be easy to determine
> what incoming channels are upgrades of what conversations.

MUCs requested with Conference will now show the Conference interface.

Not sure if it's related to your change, but while testing your Empathy branch implementing this feature I discovered this crash 100%reproductible. Basically each time Gabble receives the invitation to a (classic) muc, it crashes!

(telepathy-gabble:26571): wocky-DEBUG: Parsing chunk: <message from='tp@conference.jabber.belnet.be' to='cassidy-test2@jabber.belnet.be' type='normal'><x xmlns='http://jabber.org/protocol/muc#user'><invite from='cassidy-test1@jabber.belnet.be/Telepathy'><reason>Inviter ? cette salle</reason></invite></x><x xmlns='jabber:x:conference' jid='tp@conference.jabber.belnet.be'>Inviter ? cette salle</x></message>
(telepathy-gabble:26571): wocky-DEBUG: _end_element_ns: Received stanza
* message xmlns='jabber:client' from='tp@conference.jabber.belnet.be' to='cassidy-test2@jabber.belnet.be' type='normal'
    * x xmlns='http://jabber.org/protocol/muc#user'
        * invite from='cassidy-test1@jabber.belnet.be/Telepathy'
            * reason
                "Inviter ? cette salle"
    * x xmlns='jabber:x:conference' jid='tp@conference.jabber.belnet.be'
        "Inviter ? cette salle"
(telepathy-gabble:26571): gabble-DEBUG: new_muc_channel: creating new chan, object path /org/freedesktop/Telepathy/Connection/gabble/jabber/cassidy_2dtest2_40jabber_2ebelnet_2ebe_2fceb47283/MucChannel1
tp_group_mixin_change_flags: emitting group flags changed
  added    : [CAN_ADD|CHANNEL_SPECIFIC_HANDLES|HANDLE_OWNERS_NOT_AVAILABLE|PROPERTIES]
  removed  : []
  flags now: [CAN_ADD|CHANNEL_SPECIFIC_HANDLES|HANDLE_OWNERS_NOT_AVAILABLE|PROPERTIES]
emit_members_changed_signals: emitting members changed
  message       : "Inviter à cette salle"
  added         : [3 (cassidy-test1@jabber.belnet.be)]
  removed       : []
  local_pending : [6 (tp@conference.jabber.belnet.be/cassidy-test2@jabber.belnet.be)]
  remote_pending: []
  actor         : 3
  reason        : 4: invited
(telepathy-gabble:26571): gabble-DEBUG: muc_ready_cb: text chan=0xa45030

** ERROR **: out of memory
aborting...

Program received signal SIGABRT, Aborted.
0x00007ffff547c4b5 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64	../nptl/sysdeps/unix/sysv/linux/raise.c: Aucun fichier ou dossier de ce type.
	in ../nptl/sysdeps/unix/sysv/linux/raise.c


(gdb) bt
#0  0x00007ffff547c4b5 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ffff547ff50 in *__GI_abort () at abort.c:92
#2  0x00007ffff57fddfa in IA__g_logv (log_domain=<value optimized out>, log_level=<value optimized out>, format=<value optimized out>, 
    args1=0x7fffffffcfa0) at /build/buildd/glib2.0-2.22.2/glib/gmessages.c:549
#3  0x00007ffff57fde93 in IA__g_log (log_domain=0x67cb <Address 0x67cb out of bounds>, log_level=26571, format=0x6 <Address 0x6 out of bounds>)
    at /build/buildd/glib2.0-2.22.2/glib/gmessages.c:569
#4  0x00007ffff7bd1c0d in ?? () from /usr/lib/libdbus-glib-1.so.2
#5  0x00007ffff7bd1db3 in ?? () from /usr/lib/libdbus-glib-1.so.2
#6  0x00007ffff7bd0b2a in ?? () from /usr/lib/libdbus-glib-1.so.2
#7  0x00007ffff7bd576a in ?? () from /usr/lib/libdbus-glib-1.so.2
#8  0x00007ffff57e4d03 in IA__g_hash_table_foreach (hash_table=0xa466d0, func=0x7ffff7bd56d0, user_data=0x7fffffffd2f0)
    at /build/buildd/glib2.0-2.22.2/glib/ghash.c:1211
#9  0x00007ffff7bd559e in ?? () from /usr/lib/libdbus-glib-1.so.2
#10 0x00007ffff7bd1a5e in ?? () from /usr/lib/libdbus-glib-1.so.2
#11 0x00007ffff7bd0a23 in ?? () from /usr/lib/libdbus-glib-1.so.2
#12 0x00007ffff7bd07c9 in ?? () from /usr/lib/libdbus-glib-1.so.2
#13 0x00007ffff7bd5183 in ?? () from /usr/lib/libdbus-glib-1.so.2
#14 0x00007ffff7bd1b60 in ?? () from /usr/lib/libdbus-glib-1.so.2
#15 0x00007ffff7bc847c in ?? () from /usr/lib/libdbus-glib-1.so.2
#16 0x00007ffff5c905ae in IA__g_closure_invoke (closure=0x7359d0, return_value=0x0, n_param_values=2, param_values=0xa25730, 
    invocation_hint=0x7fffffffd7a0) at /build/buildd/glib2.0-2.22.2/gobject/gclosure.c:767
#17 0x00007ffff5ca5983 in signal_emit_unlocked_R (node=0x70ca40, detail=<value optimized out>, instance=<value optimized out>, 
    emission_return=<value optimized out>, instance_and_params=<value optimized out>) at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3247
#18 0x00007ffff5ca6d39 in IA__g_signal_emit_valist (instance=0x720150, signal_id=<value optimized out>, detail=0, var_args=0x7fffffffd990)
    at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:2980
#19 0x00007ffff5ca7283 in IA__g_signal_emit (instance=0x67cb, signal_id=26571, detail=6) at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3037
#20 0x00007ffff63f788a in manager_new_channels_cb (manager=<value optimized out>, channels=0xa46540, self=0x720150) at base-connection.c:1085
#21 0x00007ffff5c905ae in IA__g_closure_invoke (closure=0x72c8f0, return_value=0x0, n_param_values=2, param_values=0xa27960, 
    invocation_hint=0x7fffffffdc70) at /build/buildd/glib2.0-2.22.2/gobject/gclosure.c:767
#22 0x00007ffff5ca5983 in signal_emit_unlocked_R (node=0x7243c0, detail=<value optimized out>, instance=<value optimized out>, 
    emission_return=<value optimized out>, instance_and_params=<value optimized out>) at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3247
#23 0x00007ffff5ca6d39 in IA__g_signal_emit_valist (instance=0x728830, signal_id=<value optimized out>, detail=0, var_args=0x7fffffffde60)
    at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:2980
#24 0x00007ffff5ca7283 in IA__g_signal_emit (instance=0x67cb, signal_id=26571, detail=6) at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3037
#25 0x00007ffff641fd3d in tp_channel_manager_emit_new_channel (instance=0x728830, channel=0xa45030, request_tokens=0x0) at channel-manager.c:368
#26 0x000000000043dbd9 in muc_ready_cb (text_chan=0xa45030, data=<value optimized out>) at muc-factory.c:335
#27 0x000000000043deeb in new_muc_channel (fac=0x728830, handle=1, invited=<value optimized out>, inviter=<value optimized out>, 
    message=<value optimized out>, requested=<value optimized out>, initial_channels=0x0, initial_handles=0x0, initial_ids=0x0) at muc-factory.c:493
#28 0x000000000043e0f3 in do_invite (fac=0x728830, room=0xa24650 "tp@conference.jabber.belnet.be", inviter_handle=<value optimized out>, 
    reason=0xa34c00 "Inviter à cette salle") at muc-factory.c:570
#29 0x00000000004406e2 in process_muc_invite (handler=<value optimized out>, connection=<value optimized out>, message=<value optimized out>, 
    user_data=<value optimized out>) at muc-factory.c:708
#30 muc_factory_message_cb (handler=<value optimized out>, connection=<value optimized out>, message=<value optimized out>, 
    user_data=<value optimized out>) at muc-factory.c:842
#31 0x000000000047af3b in stanza_cb (self=<value optimized out>, stanza=<value optimized out>, user_data=0x0) at lm-connection.c:32
---Type <return> to continue, or q <return> to quit---
#32 0x00000000004731e5 in handle_stanza (source=<value optimized out>, res=<value optimized out>, user_data=<value optimized out>) at wocky-porter.c:792
#33 stanza_received_cb (source=<value optimized out>, res=<value optimized out>, user_data=<value optimized out>) at wocky-porter.c:979
#34 0x0000000000469bc3 in _xmpp_connection_received_data (source=0x72ba40, result=0x7fffec0022a0, user_data=<value optimized out>)
    at wocky-xmpp-connection.c:524
#35 0x00007ffff5f0bc09 in async_ready_callback_wrapper (source_object=0x72ba40, res=0x7fffec0022a0, user_data=0x9e88b0)
    at /build/buildd/glib2.0-2.22.2/gio/ginputstream.c:471
#36 0x000000000046cf36 in wocky_tls_job_result_gssize (job=<value optimized out>, result=356) at wocky-tls.c:338
#37 0x00007ffff5f0bc09 in async_ready_callback_wrapper (source_object=0x718eb0, res=0x7fffec002640, user_data=0x745090)
    at /build/buildd/glib2.0-2.22.2/gio/ginputstream.c:471
#38 0x00007ffff5f19789 in complete_in_idle_cb (data=0x67cb) at /build/buildd/glib2.0-2.22.2/gio/gsimpleasyncresult.c:598
#39 0x00007ffff57f3bbe in g_main_dispatch (context=0x70fc60) at /build/buildd/glib2.0-2.22.2/glib/gmain.c:1960
#40 IA__g_main_context_dispatch (context=0x70fc60) at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2513
#41 0x00007ffff57f7588 in g_main_context_iterate (context=0x70fc60, block=<value optimized out>, dispatch=<value optimized out>, 
    self=<value optimized out>) at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2591
#42 0x00007ffff57f79e5 in IA__g_main_loop_run (loop=0x7129e0) at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2799
#43 0x00007ffff646fac6 in tp_run_connection_manager (prog_name=<value optimized out>, version=<value optimized out>, 
    construct_cm=0x424020 <construct_cm>, argc=<value optimized out>, argv=<value optimized out>) at run.c:281
#44 0x0000000000423fce in gabble_main (argc=1, argv=0x7fffffffe5c8) at gabble.c:148
#45 0x00007ffff5467abd in __libc_start_main (main=<value optimized out>, argc=<value optimized out>, ubp_av=<value optimized out>, 
    init=<value optimized out>, fini=<value optimized out>, rtld_fini=<value optimized out>, stack_end=0x7fffffffe5b8) at libc-start.c:220
#46 0x0000000000423e29 in _start () at ../sysdeps/x86_64/elf/start.S:113

I've rebased the pmuc branch against current master, the conflicts were basically trivial. I've also fixed some bugs (which the tests and some smoke-testing revealed) in that branch. Now reviewing your code :)

(In reply to comment #21)
> I've rebased the pmuc branch against current master, the conflicts were
> basically trivial. I've also fixed some bugs (which the tests and some
> smoke-testing revealed) in that branch. Now reviewing your code :)
> 

http://git.collabora.co.uk/?p=user/sjoerd/telepathy-gabble.git;a=shortlog;h=refs/heads/pmuc btw

@@ -1254,6 +1342,24 @@ gabble_muc_channel_finalize (GObject *object)

If we check pointers for non-NULL it's prefered to use != NULL instead of assuming that non-NULL is true (for reading clarity not correctness)

@@ -1131,30 +1170,208 @@ handle_text_channel_request (GabbleMucFactory *self,

* Use DEBUG instead of g_warning, especially for stuff that can be caused by clients
* Mapping channel object paths back to ImChannel objects via dbus-glib feels a bit scary, wouldn't it  better to give the IMManager a function to lookup a channel by a path, then you wouldn't have to check if the return value is actually something you want
* Is it really wrong to mix both initial ids and handles ?
* I don't like the hardcoding of the google groupchat server. For example if i want to chat with two others on the same server, i'd like to use the conference server of that server. Especially for privacy reasons (e.g. i want the collabora ccu server, not googles groupchat)... I noticed that you can invite the google talk desktop client to remote mucs, but not their webclient. So if we want to be really nice we could check if any of your Initial people are using the webclient (by checking for their caps node for example) and only then fallback to the google groupchat server. Otherwise you'd use your own default conference server..
* Using v as the return value is not entirely obvious or common, most code uses ret for that, which makes it easier to understand :)
* +  if (room != 0) tp_handle_unref (room_handles, room);, missing newline


@@ -1385,7 +1650,7 @@ gabble_muc_factory_request (GabbleMucFactory *self,

+  g_assert (conference || handle != 0); => conference != NULL...

(In reply to comment #23)
> @@ -1254,6 +1342,24 @@ gabble_muc_channel_finalize (GObject *object)
> 
> If we check pointers for non-NULL it's prefered to use != NULL instead of
> assuming that non-NULL is true (for reading clarity not correctness)

Yep, laziness. It's worth noting that nothing else in this function is written like that.

> @@ -1131,30 +1170,208 @@ handle_text_channel_request (GabbleMucFactory *self,
> 
> * Use DEBUG instead of g_warning, especially for stuff that can be caused by
> clients

Yep.

> * Mapping channel object paths back to ImChannel objects via dbus-glib feels a
> bit scary, wouldn't it  better to give the IMManager a function to lookup a
> channel by a path, then you wouldn't have to check if the return value is
> actually something you want

It seems very convenient to do. Willing to change if someone else agrees with you.

> * Is it really wrong to mix both initial ids and handles ?

Spec says so.

> * I don't like the hardcoding of the google groupchat server. For example if i
> want to chat with two others on the same server, i'd like to use the conference
> server of that server. Especially for privacy reasons (e.g. i want the
> collabora ccu server, not googles groupchat)... I noticed that you can invite
> the google talk desktop client to remote mucs, but not their webclient. So if
> we want to be really nice we could check if any of your Initial people are
> using the webclient (by checking for their caps node for example) and only then
> fallback to the google groupchat server. Otherwise you'd use your own default
> conference server..

I agree. I want to do that, but I couldn't see how to request the caps (help appreciated). I wonder if there should be some kind of property that allows you to force the server you create on.

> * Using v as the return value is not entirely obvious or common, most code uses
> ret for that, which makes it easier to understand :)

Ok.

> * +  if (room != 0) tp_handle_unref (room_handles, room);, missing newline

Ok.

> 
> @@ -1385,7 +1650,7 @@ gabble_muc_factory_request (GabbleMucFactory *self,
> 
> +  g_assert (conference || handle != 0); => conference != NULL...

conference is a boolean.

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

(In reply to comment #24)
> (In reply to comment #23)
> > @@ -1254,6 +1342,24 @@ gabble_muc_channel_finalize (GObject *object)
> > 
> > If we check pointers for non-NULL it's prefered to use != NULL instead of
> > assuming that non-NULL is true (for reading clarity not correctness)
> 
> Yep, laziness. It's worth noting that nothing else in this function is written
> like that.

Sure, but new code should follow our style, so things slowly improve :) (Not worth fixing old code for that as it just pollutes git blame)
 
> > * Mapping channel object paths back to ImChannel objects via dbus-glib feels a
> > bit scary, wouldn't it  better to give the IMManager a function to lookup a
> > channel by a path, then you wouldn't have to check if the return value is
> > actually something you want
> 
> It seems very convenient to do. Willing to change if someone else agrees with
> you.

Fair enough, not a merge blocker from my perspective

 
> > * Is it really wrong to mix both initial ids and handles ?
> 
> Spec says so.

Hrm, just discussed it with smcv, He agrees that mixing them isn't actually an issue. Filed a bug against the spec for that: https://bugs.freedesktop.org/show_bug.cgi?id=25991

> > * I don't like the hardcoding of the google groupchat server. For example if i
> > want to chat with two others on the same server, i'd like to use the conference
> > server of that server. Especially for privacy reasons (e.g. i want the
> > collabora ccu server, not googles groupchat)... I noticed that you can invite
> > the google talk desktop client to remote mucs, but not their webclient. So if
> > we want to be really nice we could check if any of your Initial people are
> > using the webclient (by checking for their caps node for example) and only then
> > fallback to the google groupchat server. Otherwise you'd use your own default
> > conference server..
> 
> I agree. I want to do that, but I couldn't see how to request the caps (help
> appreciated).

I'm not entirely sure either, poke daf or someone else more familair with the caps code. For me this is the only real blocker for merging this branch. (In some ways i'd prefer it if we change the code to always use the users default conference server and add a branch to be nicer to google people later)

> I wonder if there should be some kind of property that allows you
> to force the server you create on.

Yes there should be, but that's somewhat orthogonal to this branch. Also the default heuristic should be what people want in 99.9% of the time ;)

> > @@ -1385,7 +1650,7 @@ gabble_muc_factory_request (GabbleMucFactory *self,
> > 
> > +  g_assert (conference || handle != 0); => conference != NULL...
> 
> conference is a boolean.
Woops, sorry :) 

Ok. All of the issues should be resolved.

Review plz!

looks good, merging