GNOME Bugzilla – Bug 643755
Create Join Chat and Leave chat menu items for the conversation menu in MUCs
Last modified: 2013-07-18 14:38:32 UTC
This bug branches from https://bugzilla.gnome.org/show_bug.cgi?id=599184#c6 and https://bugzilla.gnome.org/show_bug.cgi?id=599184#c9 These menu-items are needed to make MUCs independent of the open/closed status of the chatroom window. Join option can come handy to a kicked user to reconnect and Leave Chat will give the user ability to leave chat while having the window still open in front of him if he wants to (for some reference maybe).
My branch http://gitorious.org/glassrose-gnome/empathy/commits/join-leave-chat-menuitems-643755 deals with this bug but it depends on branch: http://gitorious.org/glassrose-gnome/empathy/commits/moving-part-functionality-to-empathy-chat-window-643295 which solves bug: https://bugzilla.gnome.org/show_bug.cgi?id=643295
I have pushed a patch to my branch which destroys password info-bar on leaving a password protected chatroom to prevent multiple instances from getting displayed on re-joining without closing the chatroom window.
Since the master is no more frozen, can we have this and the /part message implementation merged now?
Branch updated to meet the changes in the underlying bug
Rebased on top of merged moving-part-functionality-to-empathy-chat-window-643295 branch
+ if ((tp_chat = empathy_chat_get_tp_chat (priv->current_chat))) once again, please split this.
ah! done. sorry for missing that out.
I tried your branch, joined a XMPP muc, tried to leave it and it crashed: (empathy-chat:5655): empathy-DEBUG: empathy_tp_chat_leave: Leaving channel test@conference.jabber.belnet.be with message "" (empathy-chat:5655): tp-glib/proxy-DEBUG: tp_proxy_poll_features: 0x7e3150: request 0x13a9520 prepared (empathy-chat:5655): tp-glib/proxy-DEBUG: tp_proxy_prepare_request_finish: 0x13a9520 (empathy-chat:5655): empathy-DEBUG: chat_window_update_chat_tab_full: Updating chat tab, name=test, account=/org/freedesktop/Telepathy/Account/gabble/jabber/cassidy_2dtest1_40jabber_2ebelnet_2ebe0, subject=(null), remote_contact=(nil) (empathy-chat:5655): tp-glib/groups-DEBUG: tp_channel_group_members_changed_cb: 0x7e3150 MembersChanged: added 0, removed 1, moved 0 to LP and 0 to RP, actor 9, reason 0, message (empathy-chat:5655): tp-glib/groups-DEBUG: handle_members_changed: --- contact#9 Program received signal SIGSEGV, Segmentation fault. 0x000000000045db2c in escape_and_append_len (string=0x13aa360, str=0x0, len=-1) at empathy-theme-adium.c:286 286 while (*str != '\0' && len != 0) { (gdb) (gdb) (gdb) (gdb) (gdb) (gdb) (gdb) bt
+ Trace 227177
ok, I didn't expect this. I'll need WebKit to check this.
Its weird but I tried to produce this crash even with WebKit support but couldn't. Can't see this with branch rebased on master too. I tried it with conference.jabber.org and conference.collabora.co.uk (can't connect to conference.jabber.belnet.be with my gmail account which might be another bug?) Can you please re-check this. Though I noticed another thing happening once in some ten attempts i.e. channel sometimes gets disconnected long after the user leaves the chatroom, it being an asynchronous call, to solve which I can either 1) Disable both Join and Leave chat menu options when chatroom is already left but not disconnected or 2) Completely disconnect the channel on clicking leave chat by calling tp_channel_close_async instead of tp_channel_leave_async. whatever you say.
(In reply to comment #10) > Its weird but I tried to produce this crash even with WebKit support but > couldn't. Can't see this with branch rebased on master too. I tried it with > conference.jabber.org and conference.collabora.co.uk (can't connect to > conference.jabber.belnet.be with my gmail account which might be another bug?) > Can you please re-check this. Humm indeed it seems to work fine now. I guess your rebased fixed it. > Though I noticed another thing happening once in some ten attempts i.e. channel > sometimes gets disconnected long after the user leaves the chatroom, it being > an asynchronous call, to solve which I can either > 1) Disable both Join and Leave chat menu options when chatroom is already left > but not disconnected or > 2) Completely disconnect the channel on clicking leave chat by calling > tp_channel_close_async instead of tp_channel_leave_async. What do mean by "channel gets disconnected after user left"? What happen exactly from a D-Bus pov?
I meant to say that there is sometimes a lag between the members-changed signal emitted on tp-chat and invalidated signal being emitted on the channel and presently the "Leave Chat" menu option persists until tp-chat is actually destroyed(channel invalidated). What should be the best way to address this in your opinion?
Humm I think the "Leave Chat" menu should be insensitive as soon as we (our self handle) is removed from the channel members.
So for that duration (until channel is invalidated and chat is destroyed) both Join chat and Leave chat will be insensitive as one should not even attempt to re-join the chat when a disconnection is on the way..
I wouldn't worry too much about that tbh. :)
done! https://gitorious.org/glassrose-gnome/empathy/commits/join-leave-chat-menuitems-643755
Your patch doesn't build: empathy-chat-window.c: In function ‘chat_window_conv_activate_cb’: empathy-chat-window.c:912:4: error: implicit declaration of function ‘empathy_tp_chat_get_channel’ empathy-chat-window.c:913:5: error: passing argument 1 of ‘tp_channel_group_get_self_handle’ makes pointer from integer without a cast /home/cassidy/usr/include/telepathy-1.0/telepathy-glib/channel.h:106:10: note: expected ‘struct TpChannel *’ but argument is of type ‘int’ + gboolean member = (tp_channel_group_get_self_handle ( + empathy_tp_chat_get_channel (empathy_chat_get_tp_chat ( + priv->current_chat))) != 0); is a bit scary tbh, best to split it a bit.
We are past UI freeze so that probably won't make it for 3.2.
Was this actually needed anymore? I thought it would be useless with the new chat UI coming up for bug#599184 from which this bug was branched. Well I have completed it now if you are still interested in it. https://gitorious.org/glassrose-gnome/empathy/commits/join-leave-chat-menuitems-643755
Yeah I'm not convinced either that's a so useful thing to add tbh.
bug#599184 seems to be stuck for lifelong. I didn't foresee that. We should give this another go. Rebasing on gnome-3.8 branch.
Updated branch. I am unable to get the menu-items for testing even after setting empathy srcdir using the command EMPATHY_SRCDIR=./ ./src/empathy-chat Also they aren't showing up in the installed empathy-chat.
Created attachment 248104 [details] [review] "Join Chat" and "Leave Chat" menu items for Conversation menu in MUCs
Review of attachment 248104 [details] [review]: ::: src/empathy-chat-window.c @@ +1141,3 @@ + remote_contact == NULL); + gtk_action_set_visible (self->priv->menu_conv_leave_chat, + remote_contact == NULL); these two set_visible calls are useless as we re-call it in the two branches of the if/else. @@ +1142,3 @@ + gtk_action_set_visible (self->priv->menu_conv_leave_chat, + remote_contact == NULL); + I'd add a comment explaining the logic when we hide/display these 2 menu items as it's not straightforward. ::: src/empathy-chat-window.ui @@ +190,3 @@ <menuitem action="menu_conv_invite_participant"/> <separator/> + <menuitem action="menu_conv_join_chat"/> I'd move these to the top of the menu.
Are you able to produce the menu-items when tested? I am not! Maybe because tp_channel_group_get_self_contact is returning me a null value for a non-null value of channel which is weird. I am looking into it.
(In reply to comment #25) > Are you able to produce the menu-items when tested? I am not! Uh, that was when I was trying with one-to-one chats as expected so ignore this. (In reply to comment #24) > Review of attachment 248104 [details] [review]: > > ::: src/empathy-chat-window.c > @@ +1141,3 @@ > + remote_contact == NULL); > + gtk_action_set_visible (self->priv->menu_conv_leave_chat, > + remote_contact == NULL); > > these two set_visible calls are useless as we re-call it in the two branches of > the if/else. No, these deal with visibility of the menus while the ones in the conditional blocks deal with the sensitivity of the menus. > > @@ +1142,3 @@ > + gtk_action_set_visible (self->priv->menu_conv_leave_chat, > + remote_contact == NULL); > + > > I'd add a comment explaining the logic when we hide/display these 2 menu items > as it's not straightforward. Done > > ::: src/empathy-chat-window.ui > @@ +190,3 @@ > <menuitem action="menu_conv_invite_participant"/> > <separator/> > + <menuitem action="menu_conv_join_chat"/> > > I'd move these to the top of the menu. Done.
Created attachment 248774 [details] [review] "Join Chat" and "Leave Chat" menu items for Conversation menu in MUCs
Is this patch good to go?
Review of attachment 248774 [details] [review]: ::: src/empathy-chat-window.c @@ +1142,3 @@ + remote_contact == NULL); + gtk_action_set_visible (self->priv->menu_conv_leave_chat, + remote_contact == NULL); I'd just display the sensitive one actually are they are mutually exclusive. There is no much point displaying the insensitive menu item in this context. @@ +1297,3 @@ + + tp_chat = empathy_chat_get_tp_chat (self->priv->current_chat); + if (tp_chat != NULL)
Created attachment 249506 [details] [review] "Join Chat" and "Leave Chat" menu items for Conversation menu in MUCs
Review of attachment 249506 [details] [review]: ++ Thanks for the patch!