After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 604702 - support creation of PMUCs using the Conference interface
support creation of PMUCs using the Conference interface
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Chat
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
: 603608 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-12-16 10:49 UTC by Danielle Madeley
Modified: 2011-08-29 10:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch against master (for ease of review) (31.30 KB, patch)
2009-12-16 10:49 UTC, Danielle Madeley
reviewed Details | Review
updated patch (46.26 KB, patch)
2009-12-21 02:18 UTC, Danielle Madeley
reviewed Details | Review
final patch (43.71 KB, patch)
2009-12-22 03:19 UTC, Danielle Madeley
none Details | Review

Description Danielle Madeley 2009-12-16 10:49:54 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
Comment 1 Guillaume Desmottes 2009-12-16 12:47:34 UTC
*** Bug 603608 has been marked as a duplicate of this bug. ***
Comment 2 Guillaume Desmottes 2009-12-16 16:05:13 UTC
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...

  • #0 *__GI_raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 64
  • #1 *__GI_abort
    at abort.c line 92
  • #2 IA__g_logv
    at /build/buildd/glib2.0-2.22.2/glib/gmessages.c line 549
  • #3 IA__g_log
    at /build/buildd/glib2.0-2.22.2/glib/gmessages.c line 569
  • #4 free_dispatcher_request_data
    at empathy-dispatcher.c line 255
  • #5 dispatcher_request_failed
    at empathy-dispatcher.c line 1241
  • #6 dispatcher_connection_new_requested_channel
    at empathy-dispatcher.c line 1262
  • #7 _tp_cli_connection_interface_requests_invoke_callback_create_channel
    at _gen/tp-cli-connection-body.h line 12218
  • #8 tp_proxy_pending_call_idle_invoke
    at proxy-methods.c line 153
  • #9 g_main_dispatch
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c line 1960
  • #10 IA__g_main_context_dispatch
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c line 2513
  • #11 g_main_context_iterate
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c line 2591
  • #12 IA__g_main_loop_run
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c line 2799
  • #13 IA__gtk_main
    at /build/buildd/gtk+2.0-2.18.3/gtk/gtkmain.c line 1218
  • #14 main
    at empathy.c line 1051


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.
Comment 3 Guillaume Desmottes 2009-12-16 17:48:01 UTC
We should also unsensitive the menu entry when we are disconnected.
Comment 4 Danielle Madeley 2009-12-16 21:06:21 UTC
(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.
Comment 5 Danielle Madeley 2009-12-16 21:18:43 UTC
> ::: 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.
Comment 6 Danielle Madeley 2009-12-16 21:53:14 UTC
> > 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.
Comment 7 Danielle Madeley 2009-12-21 02:18:51 UTC
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
Comment 8 Guillaume Desmottes 2009-12-21 12:34:23 UTC
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 :)
Comment 9 Danielle Madeley 2009-12-21 22:01:10 UTC
(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.)
Comment 10 Danielle Madeley 2009-12-22 03:19:53 UTC
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.
Comment 11 Guillaume Desmottes 2009-12-22 10:25:03 UTC
(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.
Comment 12 Danielle Madeley 2009-12-22 11:16:17 UTC
Merged.

In case conference isn't undrafted before the 2.30 release: https://bugzilla.gnome.org/show_bug.cgi?id=605214
Comment 13 Leandro Martínez 2010-01-29 10:32:10 UTC
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?
Comment 14 Guillaume Desmottes 2010-02-08 15:44:19 UTC
You need a recent Empathy and telepathy-gabble 0.9.4 which is not in the PPA.