After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 591667 - Improve highlighting messages containing your nickname.
Improve highlighting messages containing your nickname.
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Multi User Chat
2.27.x
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
Depends on:
Blocks: 576912
 
 
Reported: 2009-08-13 11:03 UTC by Will Thompson
Modified: 2012-01-25 16:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use /\b<nickname>\b/ to decide whether to highlight (4.31 KB, patch)
2009-08-13 11:04 UTC, Will Thompson
reviewed Details | Review
Use /\b<nickname>\b/ to decide whether to highlight (3.49 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
chat_view_append_message: take a should_highlight argument (11.99 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
EmpathyChat: include should_highlight in ::new-message (2.56 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
Move empathy_message_should_highlight to EmpathyChat (6.30 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
Don't highlight messages the user sent themself (894 bytes, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
Don't highlight messages in 1-1 chats. (809 bytes, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
TpChat: add a :self-contact property. (2.23 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
accepted-commit_now Details | Review
EmpathyChat: track self contact. (3.94 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
reviewed Details | Review
EmpathyChat: cache highlight regex. (4.06 KB, patch)
2012-01-18 18:01 UTC, Will Thompson
reviewed Details | Review
EmpathyChat: cache highlight regex. (3.95 KB, patch)
2012-01-25 15:45 UTC, Will Thompson
none Details | Review

Description Will Thompson 2009-08-13 11:03:49 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.
Comment 1 Will Thompson 2009-08-13 11:04:30 UTC
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.
Comment 2 Guillaume Desmottes 2009-10-27 10:34:24 UTC
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?)?
Comment 3 Will Thompson 2012-01-18 18:01:18 UTC
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.
Comment 4 Will Thompson 2012-01-18 18:01:23 UTC
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.
Comment 5 Will Thompson 2012-01-18 18:01:25 UTC
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.
Comment 6 Will Thompson 2012-01-18 18:01:28 UTC
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!)
Comment 7 Will Thompson 2012-01-18 18:01:31 UTC
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?
Comment 8 Will Thompson 2012-01-18 18:01:34 UTC
Created attachment 205553 [details] [review]
Don't highlight messages in 1-1 chats.

https://bugzilla.gnome.org/show_bug.cgi?id=576912
Comment 9 Will Thompson 2012-01-18 18:01:37 UTC
Created attachment 205554 [details] [review]
TpChat: add a :self-contact property.
Comment 10 Will Thompson 2012-01-18 18:01:40 UTC
Created attachment 205555 [details] [review]
EmpathyChat: track self contact.
Comment 11 Will Thompson 2012-01-18 18:01:42 UTC
Created attachment 205556 [details] [review]
EmpathyChat: cache highlight regex.
Comment 12 Will Thompson 2012-01-18 18:03:50 UTC
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.
Comment 13 Guillaume Desmottes 2012-01-25 15:12:10 UTC
Review of attachment 205548 [details] [review]:

++
Comment 14 Guillaume Desmottes 2012-01-25 15:17:43 UTC
Review of attachment 205549 [details] [review]:

++
Comment 15 Guillaume Desmottes 2012-01-25 15:18:22 UTC
Review of attachment 205550 [details] [review]:

++
Comment 16 Guillaume Desmottes 2012-01-25 15:21:04 UTC
Review of attachment 205551 [details] [review]:

++
Comment 17 Guillaume Desmottes 2012-01-25 15:24:44 UTC
Review of attachment 205552 [details] [review]:

++
Comment 18 Guillaume Desmottes 2012-01-25 15:24:54 UTC
Review of attachment 205553 [details] [review]:

++
Comment 19 Guillaume Desmottes 2012-01-25 15:25:08 UTC
Review of attachment 205553 [details] [review]:

++
Comment 20 Guillaume Desmottes 2012-01-25 15:26:48 UTC
Review of attachment 205554 [details] [review]:

++
Comment 21 Guillaume Desmottes 2012-01-25 15:30:02 UTC
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).
Comment 22 Guillaume Desmottes 2012-01-25 15:30:38 UTC
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.
Comment 23 Will Thompson 2012-01-25 15:33:33 UTC
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().
Comment 24 Will Thompson 2012-01-25 15:45:53 UTC
Created attachment 206099 [details] [review]
EmpathyChat: cache highlight regex.
Comment 25 Will Thompson 2012-01-25 16:18:29 UTC
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.