GNOME Bugzilla – Bug 571642
when an irc user changes nick, empathy doesn't indicate that the name changed
Last modified: 2009-11-18 19:10:16 UTC
Instead, it says that the old nick quit and the new nick entered the chat room. my muc-rename branch attempts to fix this.
Created attachment 128648 [details] [review] Proposed fix in branch muc-rename http://git.collabora.co.uk/?p=user/jonathon/empathy;a=summary libempathy-gtk/empathy-chat.c | 33 ++++++++++++++++ libempathy-gtk/empathy-contact-list-store.c | 56 +++++++++++++++++++++++++++ libempathy/empathy-contact-list.c | 9 ++++ libempathy/empathy-tp-chat.c | 15 +++++++ libempathy/empathy-tp-group.c | 49 ++++++++++++++++++++++- 5 files changed, 160 insertions(+), 2 deletions(-)
I'm not familiar enough with these part of Empathy, so I'll let Xavier do the final review. Few comments though: - chat_member_renamed_cb: it would be good to avoid to use empathy_contact_run_until_ready. we should probably have a _call_when_ready () - We are trying to get rid of EmpathyTpGroup. Is it possible to do the same using tp-glib API directly? - tp_group_update_members: add "old" and "new" in comments so we known which contact we are talking about. - Always use TpHandle instead of guint when storing handles in variables. - contact_list_store_member_renamed_cb: would be good to share the common code with contact_list_store_members_changed_cb in a common function - Please fill a bug explaining the problem when a contact is renamed in a private chat.
Created attachment 128657 [details] [review] patch libempathy-gtk/empathy-chat.c | 33 +++++++++ libempathy-gtk/empathy-contact-list-store.c | 94 ++++++++++++++++++++------ libempathy/empathy-contact-list.c | 9 +++ libempathy/empathy-tp-chat.c | 15 ++++ libempathy/empathy-tp-group.c | 49 +++++++++++++- 5 files changed, 176 insertions(+), 24 deletions(-)
I should also note that in order to be able to see the effect of this patch, you'll need to be running telepathy-idle from git since it didn't use the RENAMED reason before.
The patch does not apply anymore at all. Lots of things changed since... Could you please rebase/rewrite the patch? Sorry to be so slow to review :(
Created attachment 138204 [details] [review] Handle the case where a user's id changes in a chatroom Telepathy-glib has a enum value for the MembersChanged signal to signify that a user's ID has changed. Previously, empathy was simply interpreting this as if a user with the old name had left the chat and a different user with the new name had entered the chat. This change handles this case more gracefully by updating the contact's id (and name) when this change reason is present One thing that does not yet work with this patch is if you are engaged in a private chat with a person and they change their nick in the middle of the chat. Then the EmpathyContact* that you are chatting with is no longer the EmpathyContact* representing the remote user, so messages won't be delivered properly. When we detect that a user has been 'renamed', we probably need to somehow go through all of the private chats with that person and swap out the old (invalid) EmpathyContact* and replace it with the new one so that the chat can continue without interruption.
Created attachment 138205 [details] [review] Fixed some of Gillaume's review comments from Bug #571642 The following comments should be fixed in this commit: - tp_group_update_members: add "old" and "new" in comments so we known which contact we are talking about. - Always use TpHandle instead of guint when storing handles in variables. - contact_list_store_member_renamed_cb: would be good to share the common code with contact_list_store_members_changed_cb in a common function One comment about the last one: I split out common code into two new functions: contact_list_store_add_contact_and_connect(), and the matching contact_list_store_remove_contact_and_disconnect(). I wondered whether the signal connection/disconnection should just be added to the _add_contact() and _remove_contact() functions directly, but they do seem to be used in other places without connecting to signals, so i thought it would be safer to simply add some wrapper functions.
Comment on attachment 128657 [details] [review] patch Original description was too long for the soon-to-be-upgraded Bugzilla.
*** Bug 590795 has been marked as a duplicate of this bug. ***
Jonathan (and Max?), could you do a quick check to see whether the passage of time (& thus advances in Empathy & tp-idle, etc.) require patch revisions? That way I can push maintainers to review it. :-)
Fortunately, the patches still apply cleanly to master as of today (though I didn't try to build it, just applied it). Guillaume, can we get this in soon? :)
I will look at it. thanks!
Created attachment 148055 [details] [review] I rebased your branch and pused some (trivial) patches. Please review them and I'll (finally) merge the branch. http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/muc-rename-571642 libempathy-gtk/empathy-chat.c | 29 ++++++++ libempathy-gtk/empathy-contact-list-store.c | 94 ++++++++++++++++++++------ libempathy/empathy-contact-list.c | 9 +++ libempathy/empathy-tp-chat.c | 96 +++++++++++++++++++++++++++ 4 files changed, 206 insertions(+), 22 deletions(-)
Review of attachment 148055 [details] [review]: Oh, good catch on dup-ing the message. Everything looks good to me.
Woo, merged to master \o/ Thanks for your work (and your patience :) on this. 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.