GNOME Bugzilla – Bug 604348
/part not implemented
Last modified: 2011-03-07 07:50:58 UTC
From bug #573407: /part is not working
any progress on this?
When I closed an IRC window telepathy-gabble logs show that part command was sent so this just needs a patch for the task?
(In reply to comment #2) > When I closed an IRC window telepathy-gabble logs show that part command was > sent so this just needs a patch for the task? I doubt that. I checked the debug logs and found that PART command was sent for telepathy-idle but not in telepathy-gabble. The part command has the following usage: PART [<channel>] [<reason>] Idle definitely supports full command. If just /PART (without channel and reason options) was to be supported (with current channel as default <channel>), one workaround could be to simply close the chat tab & current channel for the time being, but that would be wrong as we ultimately need a way to close the chat-rooms window without actually leaving it (see https://bugzilla.gnome.org/show_bug.cgi?id=599184)
Created attachment 181549 [details] [review] Part command implementation and Join, Leave chat menuitems public branch: http://gitorious.org/glassrose-gnome/empathy/commits/muc-part-command-604348
Review of attachment 181549 [details] [review]: You should make more atomic commits: - One adding the tp-chat API - One implementing /part - One adding the menu entries We are in UI freeze now, so I think we should focus on /part for now. I tried the "leave chat" option in a XMPP muc and hit this error: empathy-CRITICAL **: empathy_chat_window_find_chat: assertion `!EMP_STR_EMPTY (id)' failed aborting... Program received signal SIGTRAP, Trace/breakpoint trap. 0x00007fffe835c688 in g_logv (log_domain=0x495918 "empathy", log_level=G_LOG_LEVEL_CRITICAL, format=0x7fffe83e4818 "%s: assertion `%s' failed", args1=0x7fffffffcc70) at gmessages.c:553 553 G_BREAKPOINT (); (gdb) (gdb) (gdb) (gdb) bt
+ Trace 226055
::: libempathy-gtk/empathy-chat.c @@ +766,3 @@ + if (chat_to_be_parted != NULL) { + empathy_tp_chat_leave (empathy_chat_get_tp_chat (chat_to_be_parted), strv[2]); + } else { If a chat has been passed and we can't find it then we should raise an error rather than leaving the current chat. ::: libempathy/empathy-tp-chat.c @@ +1186,3 @@ if (error) { DEBUG ("Error: %s", error->message); + empathy_tp_chat_leave (EMPATHY_TP_CHAT (chat), ""); Are these 2 cases really such a big deal that we should leave? @@ +1908,2 @@ tp_cli_channel_interface_group_call_remove_members (priv->channel, -1, array, + message, leave_remove_members_cb, self, NULL, G_OBJECT (self)); I think you can use tp_channel_leave_async() here. ::: src/empathy-chat-window.c @@ +844,3 @@ + + gtk_action_set_sensitive (priv->menu_conv_join_chat, disconnected); + gtk_action_set_sensitive (priv->menu_conv_leave_chat, !disconnected); You should group those; no need to make 2 calls. @@ +1007,3 @@ + strv = empathy_chat_command_parse ("part", 3); + + empathy_chat_command_part (priv->current_chat, strv); That's a bit hacky. We should call a proper part function rather than building a GStrv.
hey, the Leave Chat option froze? It's working fine for me. maybe I need to update master, rebase branch to the tip of master and retest. i'll split the commits.
(In reply to comment #6) > hey, the Leave Chat option froze? It's working fine for me. maybe I need to > update master, rebase branch to the tip of master and retest. i'll split the > commits. OK, this was an unnoticed error on my part. rectified. Commit split and branch updated with other changes except a few things- For this: >::: libempathy-gtk/empathy-chat.c >@@ +766,3 @@ >+ if (chat_to_be_parted != NULL) { >+ empathy_tp_chat_leave (empathy_chat_get_tp_chat (chat_to_be_parted), >strv[2]); >+ } else { > >If a chat has been passed and we can't find it then we should raise an error >rather than leaving the current chat. what if one wants to leave the current chat with some reason specified? I presumed that since current chat-room ID is default, then user should not explicitly have to type the current chat's name and be able to use command also as: /PART [<chatroom-ID>] [<reason>] /PART I am sleepy If "I" is not name of (an optional) MUC channel then shouldn't it be treated as part of the optional reason message? I find absence of this feature in xchat particularly annoying when I have to type even the current chat's name if I need to specify a reason especially when the syntax states that I can provide a reason without a chatroom ID. If this seems odd to you, i'll change it. I didn't put any new code in these lines: >::: libempathy/empathy-tp-chat.c >@@ +1186,3 @@ > if (error) { > DEBUG ("Error: %s", error->message); >+ empathy_tp_chat_leave (EMPATHY_TP_CHAT (chat), ""); > >Are these 2 cases really such a big deal that we should leave? I just adapted the code to invoke the changed method correctly. Also, if not leave then what else? After all, self handle and remote contact should be obtainable in constructor. and for this: >::: src/empathy-chat-window.c >@@ +844,3 @@ >+ >+ gtk_action_set_sensitive (priv->menu_conv_join_chat, disconnected); >+ gtk_action_set_sensitive (priv->menu_conv_leave_chat, !disconnected); > >You should group those; no need to make 2 calls. is there a new API for grouping these for which I can't find the relevant function?
(In reply to comment #7) > (In reply to comment #6) > >::: libempathy-gtk/empathy-chat.c > >@@ +766,3 @@ > >+ if (chat_to_be_parted != NULL) { > >+ empathy_tp_chat_leave (empathy_chat_get_tp_chat (chat_to_be_parted), > >strv[2]); > >+ } else { > > > >If a chat has been passed and we can't find it then we should raise an error > >rather than leaving the current chat. > > > what if one wants to leave the current chat with some reason specified? > I presumed that since current chat-room ID is default, then user should not > explicitly have to type the current chat's name and be able to use command also > as: > > /PART [<chatroom-ID>] [<reason>] > /PART I am sleepy > > If "I" is not name of (an optional) MUC channel then shouldn't it be treated as > part of the optional reason message? > I find absence of this feature in xchat particularly annoying when I have to > type even the current chat's name if I need to specify a reason especially when > the syntax states that I can provide a reason without a chatroom ID. If this > seems odd to you, i'll change it. Humm yeah I guess that makes sense. > I didn't put any new code in these lines: Oh you're right, sorry. > and for this: > > >::: src/empathy-chat-window.c > >@@ +844,3 @@ > >+ > >+ gtk_action_set_sensitive (priv->menu_conv_join_chat, disconnected); > >+ gtk_action_set_sensitive (priv->menu_conv_leave_chat, !disconnected); > > > >You should group those; no need to make 2 calls. > > is there a new API for grouping these for which I can't find the relevant > function? Rahh I missread that too (seems that yesterday was not my day...). tp_channel_leave_async (priv->channel,TP_CHANNEL_GROUP_CHANGE_REASON_NONE missing space after the ','
ok done :)
Rah sorry, I found some issues, I fail at reviewing :( if (tp_channel_leave_finish (TP_CHANNEL(source_object), res, &error)) { if (error != NULL) { That's the wrong pattern. You should do: if (!tp_channel_leave_finish (...)) { DEBUG () g_error_free () } If the _finish function fails, you can be sure that the GError has been set. No need to do all these check in empathy_tp_chat_leave, tp_channel_leave_async() will just do the right thing. empathy_tp_chat_leave just have to be a thin wrapper around tp_channel_leave_async(). Also, can you please rebase your branch on top of current master?
ok done, looks good now. I was wondering why this method tp_channel_leave_async() is not listed in my libtelepathy-glib-docs which is reported to be latest. Is it a new method not yet documented? I resolved a conflict too! :)
(In reply to comment #11) > ok done, looks good now. Great, thanks. I merged the 2 first patches to master but not the one adding the menu entry as we are now in UI freeze. Can you please open another bug about that? > I was wondering why this method tp_channel_leave_async() is not listed in my > libtelepathy-glib-docs which is reported to be latest. Is it a new method not > yet documented? Weird, it's documented, see http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-channel.html#tp-channel-leave-async This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
(In reply to comment #12) > (In reply to comment #11) > > ok done, looks good now. > > Great, thanks. I merged the 2 first patches to master but not the one adding > the menu entry as we are now in UI freeze. > Can you please open another bug about that? https://bugzilla.gnome.org/show_bug.cgi?id=643755 is opened for the menu items.