GNOME Bugzilla – Bug 591667
Improve highlighting messages containing your nickname.
Last modified: 2012-01-25 16:18:29 UTC
When using Empathy for IRC, it's really annoying that it doesn't always highlight me when people say my name. The patch at the top of <http://git.collabora.co.uk/?p=user/wjt/empathy.git;a=shortlog;h=refs/heads/highlight-regex> (about to be attached) replaces the somewhat ad-hoc matching algorithm with a simple regex, which works a lot better.
Created attachment 140643 [details] [review] Use /\b<nickname>\b/ to decide whether to highlight The current highlighting code finds the first occurrence of the nickname, then checks whether the characters before or after are a space, a comma, a colon or a full stop (or the start or end of the string). This means that someone saying “no! That's wjt’s coffee!” didn’t highlight me, because the apostrophe isn't in the whitelist. It also means that saying “borrow some Sudafed from daf” would not highlight daf, since the first match is in the middle of a word. We’re trying to check whether the nickname occurs as a complete word within the message. The regex metacharacter \b matches word boundaries, so /\b<nickname>\b/ is what we actually want. It gets the above cases right, including Unicode punctuation.
Review of attachment 140643 [details] [review]: Thanks for your patch and sorry for the late review. You should early return if g_regex_new returns NULL; if not you'll add a NULL value to the hash table which is useless as g_hash_table_lookup will return NULL; so your comment is false. I don't really like that regexep are never freed. Maybe it should be a cache as suggested (using GCache?)?
Created attachment 205548 [details] [review] Use /\b<nickname>\b/ to decide whether to highlight The current highlighting code finds the first occurrence of the nickname, then checks whether the characters before or after are a space, a comma, a colon or a full stop (or the start or end of the string). This means that someone saying “no! That's wjt’s coffee!” didn’t highlight me, because the apostrophe isn't in the whitelist. It also means that saying “borrow some Sudafed from daf” would not highlight daf, since the first match is in the middle of a word. We’re trying to check whether the nickname occurs as a complete word within the message. The regex metacharacter \b matches word boundaries, so /\b<nickname>\b/ is what we actually want. It gets the above cases right, including Unicode punctuation.
Created attachment 205549 [details] [review] chat_view_append_message: take a should_highlight argument Rather than the EmpathyChatView implementations calling empathy_message_should_highlight() themselves, this patch makes EmpathyChat take responsibility for doing so. The theme preview in Preferences, whether deliberately or otherwise, highlights the line in which Juliet mentions Romeo. This behaviour is preserved.
Created attachment 205550 [details] [review] EmpathyChat: include should_highlight in ::new-message This allows EmpathyChatWindow to use this rather than calling empathy_message_should_highlight() itself.
Created attachment 205551 [details] [review] Move empathy_message_should_highlight to EmpathyChat This will give us a place to cache the GRegex object. (Of course, this also depends on monitoring changes to the TpChat's self contact's alias, and changes to the TpChat's self contact!)
Created attachment 205552 [details] [review] Don't highlight messages the user sent themself If a message is outgoing, then why on earth would we check whether it mentions our own nick and highlight it?
Created attachment 205553 [details] [review] Don't highlight messages in 1-1 chats. https://bugzilla.gnome.org/show_bug.cgi?id=576912
Created attachment 205554 [details] [review] TpChat: add a :self-contact property.
Created attachment 205555 [details] [review] EmpathyChat: track self contact.
Created attachment 205556 [details] [review] EmpathyChat: cache highlight regex.
This branch moves responsibility for deciding whether to highlight a message into EmpathyChat, which can then own the GRegex. Wow, that was a lot of code churn! But I fixed bug 576912 in passing. http://cgit.collabora.com/git/user/wjt/empathy.git/log/?h=highlight-regex for the cgit-inclined.
Review of attachment 205548 [details] [review]: ++
Review of attachment 205549 [details] [review]: ++
Review of attachment 205550 [details] [review]: ++
Review of attachment 205551 [details] [review]: ++
Review of attachment 205552 [details] [review]: ++
Review of attachment 205553 [details] [review]: ++
Review of attachment 205554 [details] [review]: ++
Review of attachment 205555 [details] [review]: ::: libempathy-gtk/empathy-chat.c @@ +3255,3 @@ } + if (priv->self_contact) { + g_object_unref (priv->self_contact); g_clear_object() ftw! (I know this object doesn't use it but best to get it right in newly written code).
Review of attachment 205556 [details] [review]: ::: libempathy-gtk/empathy-chat.c @@ +1420,3 @@ + EmpathyChatPriv *priv = GET_PRIV (chat); + +static void tp_clear_pointer() would make this sexier. @@ +3300,3 @@ + if (priv->highlight_regex != NULL) { + g_regex_unref (priv->highlight_regex); same here.
Review of attachment 205555 [details] [review]: ::: libempathy-gtk/empathy-chat.c @@ +3255,3 @@ } + if (priv->self_contact) { + g_object_unref (priv->self_contact); Actually, the next patch adds a call to g_signal_handlers_disconnect_by_func() in this block, which is why I didn't use g_clear_object().
Created attachment 206099 [details] [review] EmpathyChat: cache highlight regex.
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.