GNOME Bugzilla – Bug 643295
EmpathyChat shouldn't call empathy_chat_window_find_chat
Last modified: 2011-05-13 14:46:28 UTC
EmpathyChat lives in libempathy-gtk and so shouldn't use empathy_chat_window_find_chat() which is defined in src/. A way to solve this would be to fire a signal in chat_command_part and let chat-window handle the command.
1) Part command dealing function shifted to src/empathy-chat-window 2) Corrected the reason message being send for default chatroom and added a DEBUG message. branch: http://gitorious.org/glassrose-gnome/empathy/commits/moving-part-functionality-to-empathy-chat-window-643295
I think you should use G_TYPE_STRV instead of G_TYPE_POINTER in the signal definition. Please remove the #include "src/..." in EmpathyChat. message = g_strconcat(strv[1], " ", strv[2], NULL); a comment explaining the logic here would be good.
Branch updated
EmpathyChatManager calls empathy_chat_window_find_chat() also.
Which is in src... ignore me.
Master doesn't compile for me but I have re-based my branch for this bug ( http://gitorious.org/glassrose-gnome/empathy/commits/moving-part-functionality-to-empathy-chat-window-643295 ) to a branch-point close to the tip of master from where I guess it will be easier to merge this. I have also added a check to prevent multiple instances of password info-bars which were getting popped up on re-joining protected rooms after parting. Also, if no exception is needed then nothing should be stopping this from getting merged to master now.
message = g_strconcat(strv[1], " ", strv[2], NULL); missing space before '(' "make check" should catch this kind of coding style issue. I think the infobar should be destroyed in EmpathyChat when the channel is invalidated. We shouldn't expose such details outside of EmpathyChat.
(In reply to) > I think the infobar should be destroyed in EmpathyChat when the channel is > invalidated. We shouldn't expose such details outside of EmpathyChat. but the parting code calling empathy_tp_chat_leave() which in turn starts an asynchronous call to tp_channel_leave_async() is placed in the chat window itself. I didn't see any channel invalidating code in EmpathyChat.
The updated branch http://gitorious.org/glassrose-gnome/empathy/commits/moving-part-functionality-to-empathy-chat-window-643295 handles infobar destruction details in EmpathyChat method: empathy_chat_leave_chat(). This is the same method that will leave the tp_chat on Leave Chat menuitem selection in branch http://gitorious.org/glassrose-gnome/empathy/commits/join-leave-chat-menuitems-643755 which also has been updated. I hope this looks good.
Created attachment 187343 [details] [review] Destroy password infobar on parting password protected chatrooms
Review of attachment 187343 [details] [review]: The first 2 commits look good so I'm just commenting on this one. Would be cool if you could rebase the branch and test it with master though. ::: libempathy-gtk/empathy-chat.c @@ +761,3 @@ +void +empathy_chat_leave_chat (EmpathyChat *chat, I think that's the wrong approach. We should'n introduce this function doing all these scary things. the chat window should continue using tp_chat_leave() and display_password_info_bar() should just be modified to connect on the invalidated signal on the channel and destroy the infobar in the cb. ::: libempathy-gtk/empathy-chat.h @@ +74,3 @@ EmpathyContact * empathy_chat_get_remote_contact (EmpathyChat *chat); GtkWidget * empathy_chat_get_contact_menu (EmpathyChat *chat); +GList * empathy_chat_get_password_infobar_vbox_children (EmpathyChat *chat); why adding this as API? It's not used outside of empathy-chat ::: src/empathy-chat-window.c @@ +1487,3 @@ GStrv strv) { + EmpathyChat *chat_to_be_parted; you added an extra space here.
Branch updated with reviews checked. I re-framed the comments in the last commit too so that they are more clear now.
Cool, looking much better ! Can you please add a comment in chat_invalidated_cb() explaining what you're doing, it's not that clear looking at the code. I'd add a \n after tp_g_signal_connect_object(). gtk_widget_set_sensitive (self->input_text_view, FALSE); Didn't we need this before? When is it set back to TRUE?
Branch updated. (In reply to comment #13) > Cool, looking much better ! > > Can you please add a comment in chat_invalidated_cb() explaining what you're > doing, it's not that clear looking at the code. > > I'd add a \n after tp_g_signal_connect_object(). Done. > > gtk_widget_set_sensitive (self->input_text_view, FALSE); > Didn't we need this before? When is it set back to TRUE? I don't remember whether I noticed this earlier or not, but the input_text_view is getting set to sensitive whenever the chat is connected: http://git.gnome.org/browse/empathy/tree/libempathy-gtk/empathy-chat.c#n3600 so I made a check here. I added another patch to make /part attempt to leave_tp_chat only if tp_chat is not null.
> I added another patch to make /part attempt to leave_tp_chat only if tp_chat is > not null. Why is this needed?
If not, it will crash if I use /part on a chat which is already disconnected. I had lost this check with the commit that was removed so re-introducing it here.
Yeah but if the chat has been disconnected, the entry should be insensitive and si we can't type /part.
We can by entering "/part <chatroom1-id>" in any chatroom2 connected from the same account.
Oh, very good point! I'm not fan of this syntax: if ((tp_chat = empathy_chat_get_tp_chat (chat))) assignation in test are pretty confusing (most people will read '==' instead of '='); I'd prefer if you split it. After that I think we're good. :)
Done! :)
Merged; thanks a lot ! 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.
yay! np :)