GNOME Bugzilla – Bug 604702
support creation of PMUCs using the Conference interface
Last modified: 2011-08-29 10:12:28 UTC
Created attachment 149823 [details] [review] patch against master (for ease of review) http://git.collabora.co.uk/?p=user/danni/empathy.git;a=shortlog;h=refs/heads/pmuc Allows the upgrade of 1-to-1 chats to PMUCs and to invite further users into a MUC. FIXME: EmpathyInviteParticipantsDialog should be replaced by EmpathyContactSelectorDialog from cassidy's new-call branch, which does much the same thing. Requires Conference enabled Gabble: http://bugs.freedesktop.org/show_bug.cgi?id=22768 See also: http://bugs.freedesktop.org/show_bug.cgi?id=24939
*** Bug 603608 has been marked as a duplicate of this bug. ***
Review of attachment 149823 [details] [review]: What's the ETA of the Conference spec (as stable)? I'm fine with merging the implementation as a DRAFT but would like to be sure it will be undrafted for 2.30 (29th March). I didn't review the dialog code. I got this crash: - Open a chat with a non gmail contact running a not patched Gabble (just open the window, didn't say anything) - Invite another contact running a patched gabble (/home/cassidy/gnome/empathy/src/.libs/empathy:26809): empathy-DEBUG: dispatcher_connection_new_requested_channel: Channel request failed: invalid room JID private-chat-4b2903b0.489e6/0/b2fe389f@groupchat.google.com: contains nickname part after '/' too GLib-CRITICAL **: g_hash_table_unref: assertion `hash_table->ref_count > 0' failed aborting...
+ Trace 219640
I also got this crash while using a non patched Gabble; I guess that's the same issue. (/home/cassidy/gnome/empathy/src/.libs/empathy:26710): empathy-DEBUG: dispatcher_connection_new_requested_channel: Channel request failed: Not implemented GLib-CRITICAL **: g_hash_table_unref: assertion `hash_table->ref_count > 0' failed aborting... ::: src/empathy-chat-window.c @@ +36,3 @@ #include <libnotify/notification.h> +#include <telepathy-glib/telepathy-glib.h> This requiers telepathy-glib 0.9.2. You should bump the dep and announce it on ddl. @@ +858,3 @@ + /* Although this is a MUC, it's anonymous, so CreateChannel is valid */ + empathy_dispatcher_create_channel (dispatcher, connection, + props, NULL, NULL); What happen with the existing 1-1 channel? Did you consider to do that in EmpathyChat instead? @@ +912,3 @@ + channel, -1, &handles, + _("Inviting to this room"), + NULL, NULL, NULL, NULL); Would be good to factor out this code in EmpathyChat and share it with the existing invite implementation. ::: src/empathy-chat-window.ui @@ +152,3 @@ <menuitem action="menu_conv_favorite"/> <menuitem action="menu_conv_toggle_contacts"/> + <menuitem action="menu_conv_invite_participant"/> This menu item should be disabled in 1-1 chat if the CM doesn't support upgrade.
We should also unsensitive the menu entry when we are disconnected.
(In reply to comment #2) > What's the ETA of the Conference spec (as stable)? I'm fine with merging the > implementation as a DRAFT but would like to be sure it will be undrafted for > 2.30 (29th March). Unknown. We need a couple of implementations of it though, of course. > I didn't review the dialog code. Fair enough. > I got this crash: > - Open a chat with a non gmail contact running a not patched Gabble (just open > the window, didn't say anything) > - Invite another contact running a patched gabble > > (/home/cassidy/gnome/empathy/src/.libs/empathy:26809): empathy-DEBUG: > dispatcher_connection_new_requested_channel: Channel request failed: invalid > room JID private-chat-4b2903b0.489e6/0/b2fe389f@groupchat.google.com: contains > nickname part after '/' too Your gabble isn't built with libuuid, so your uuid is nonsense, this is a known issue. > GLib-CRITICAL **: g_hash_table_unref: assertion `hash_table->ref_count > 0' > failed > aborting... This one is weird though. I've seen this a few times. I don't _think_ it's related to my work, I think it's a bug in EmpathyDispatcher, but it could be triggered by the way I'm calling it.
> ::: src/empathy-chat-window.c > @@ +36,3 @@ > #include <libnotify/notification.h> > > +#include <telepathy-glib/telepathy-glib.h> > > This requiers telepathy-glib 0.9.2. You should bump the dep and announce it on > ddl. Yep. Ok. > @@ +858,3 @@ > + /* Although this is a MUC, it's anonymous, so CreateChannel is valid */ > + empathy_dispatcher_create_channel (dispatcher, connection, > + props, NULL, NULL); > > What happen with the existing 1-1 channel? > Did you consider to do that in EmpathyChat instead? At the moment the existing 1-to-1 channel remains. Eventually Empathy should be taught about the Conference interface for new channels, and to upgrade the interface. We can't do this for incoming channels yet, so I've left it alone for the moment. It probably should belong in EmpathyChat, yes. > @@ +912,3 @@ > + channel, -1, &handles, > + _("Inviting to this room"), > + NULL, NULL, NULL, NULL); > > Would be good to factor out this code in EmpathyChat and share it with the > existing invite implementation. Agree. > ::: src/empathy-chat-window.ui > @@ +152,3 @@ > <menuitem action="menu_conv_favorite"/> > <menuitem action="menu_conv_toggle_contacts"/> > + <menuitem action="menu_conv_invite_participant"/> > > This menu item should be disabled in 1-1 chat if the CM doesn't support > upgrade. Hmm. I suppose that information is in RequestableChannelClasses. > We should also unsensitive the menu entry when we are disconnected. Yes.
> > GLib-CRITICAL **: g_hash_table_unref: assertion `hash_table->ref_count > 0' > > failed > > aborting... > > This one is weird though. I've seen this a few times. I don't _think_ it's > related to my work, I think it's a bug in EmpathyDispatcher, but it could be > triggered by the way I'm calling it. Ok, this is my fault. EmpathyDispatcher steals your ref to the hash table. Added some docs to empathy_dispatcher_create_channel to explain it's not like tp-glib.
Created attachment 150145 [details] [review] updated patch Ok, these issues are now cleared up: http://git.collabora.co.uk/?p=user/danni/empathy.git;a=shortlog;h=refs/heads/pmuc configure.ac | 2 +- extensions/Channel_Interface_Conference.xml | 400 ++++++++++++++++++++++ extensions/misc.xml | 1 + libempathy-gtk/empathy-contact-menu.c | 9 +- libempathy-gtk/empathy-contact-selector-dialog.c | 57 ++-- libempathy-gtk/empathy-contact-selector-dialog.h | 7 +- libempathy-gtk/empathy-new-call-dialog.c | 24 +- libempathy-gtk/empathy-new-message-dialog.c | 25 +- libempathy/empathy-dispatcher.c | 13 + libempathy/empathy-tp-chat.c | 109 ++++++- libempathy/empathy-tp-chat.h | 1 + src/Makefile.am | 1 + src/empathy-chat-window.c | 101 ++++++- src/empathy-chat-window.ui | 7 + src/empathy-invite-participant-dialog.c | 77 +++++ src/empathy-invite-participant-dialog.h | 46 +++ 16 files changed, 809 insertions(+), 71 deletions(-) This branch also contains two more patches to EmpathyContactSelectorDialog: http://git.collabora.co.uk/?p=user/danni/empathy.git;a=commitdiff;h=b9d6d1df4ef6fd3fd3b7ceeb3e61b873e7f02b01 http://git.collabora.co.uk/?p=user/danni/empathy.git;a=commitdiff;h=e393222985ef9b76be04d6825141e53660d29751
Review of attachment 150145 [details] [review]: I tried your branches and got this error: dispatcher_connection_new_requested_channel: Channel request failed: invalid room JID private-chat-4b2f68fc.663da/1/37913f49@groupchat.google.com: contains nickname part after '/' too ::: libempathy/empathy-dispatcher.c @@ +1622,3 @@ + * reference to @request. DO NOT unref or destroy @request. When the request is + * done, @request will be unreferenced. Take another reference if you want to + * keep it around. Shouldn't we call g_hash_table_ref instead of stealing the caller one? ::: src/empathy-chat-window.c @@ +884,3 @@ + + /* FIXME: should filter out the existing participants from the + * list */ Would be good to fix this before merging if it's not too much work. ::: src/empathy-invite-participant-dialog.c @@ +22,3 @@ +struct _EmpathyInviteParticipantDialogPrivate +{ + int dum; What's this? @@ +37,3 @@ +{ + EmpathyContactSelectorDialog *parent = EMPATHY_CONTACT_SELECTOR_DIALOG (self); + // EmpathyInviteParticipantDialogPrivate *priv = GET_PRIVATE (self); Please remove if unused (booh C++ style comments :)
(In reply to comment #8) > Review of attachment 150145 [details] [review]: > > I tried your branches and got this error: > dispatcher_connection_new_requested_channel: Channel request failed: invalid > room JID private-chat-4b2f68fc.663da/1/37913f49@groupchat.google.com: contains > nickname part after '/' too I already answered that, you built your gabble without libuuid, and so it can't generate a valid UUID. > ::: libempathy/empathy-dispatcher.c > @@ +1622,3 @@ > + * reference to @request. DO NOT unref or destroy @request. When the request > is > + * done, @request will be unreferenced. Take another reference if you want to > + * keep it around. > > Shouldn't we call g_hash_table_ref instead of stealing the caller one? I wondered about that, but I didn't want to change (and possibly break) existing code inside Empathy, so I just documented how the call works instead. > ::: src/empathy-chat-window.c > @@ +884,3 @@ > + > + /* FIXME: should filter out the existing participants from the > + * list */ > > Would be good to fix this before merging if it's not too much work. Yeah. I had forgotten about this. > ::: src/empathy-invite-participant-dialog.c > @@ +22,3 @@ > +struct _EmpathyInviteParticipantDialogPrivate > +{ > + int dum; > > What's this? Oh, that was to shut up the warning about zero sized structs, it actually means I don't need the private struct. > @@ +37,3 @@ > +{ > + EmpathyContactSelectorDialog *parent = EMPATHY_CONTACT_SELECTOR_DIALOG > (self); > + // EmpathyInviteParticipantDialogPrivate *priv = GET_PRIVATE (self); > > Please remove if unused (booh C++ style comments :) Yeah. (I use C++-style comments to comment out blocks of code, because that way they don't interfere with my explanation comments.)
Created attachment 150215 [details] [review] final patch Ok, this resolves the remaining cleanups. > ::: src/empathy-chat-window.c > @@ +884,3 @@ > + > + /* FIXME: should filter out the existing participants from the > + * list */ > > Would be good to fix this before merging if it's not too much work. I've just worked out why this can't be fixed. In general a MUC doesn't have the real JID for a participant, so there's no point filtering those JIDs out from the contact list [for XMPP at least]. I've decided to forget it, it's not a major issue.
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 150145 [details] [review] [details]: > > > > I tried your branches and got this error: > > dispatcher_connection_new_requested_channel: Channel request failed: invalid > > room JID private-chat-4b2f68fc.663da/1/37913f49@groupchat.google.com: contains > > nickname part after '/' too > > I already answered that, you built your gabble without libuuid, and so it can't > generate a valid UUID. Oh you're right. I read some many bug reports each day that sometime I loose context when switching. Sorry. If you're confident that the Gabble implementation will be undrafted before the 2.30 release feel free to merge and open a blocker bug saying we should undraft our implementation as well.
Merged. In case conference isn't undrafted before the 2.30 release: https://bugzilla.gnome.org/show_bug.cgi?id=605214
I'm using Empathy 2.29.5.1 from PPA and I still have the same problem as I reported here: https://bugs.launchpad.net/ubuntu/+source/empathy/+bug/491374 Also, I still cannot find anyway in which I could add someone to a group chat in a chat room already open. Should it be fixed in this version or not?
You need a recent Empathy and telepathy-gabble 0.9.4 which is not in the PPA.