Created attachment 331131 [details] [review]
room: Set self-nick in the offline case

We keep track of the connection's self-contact property to set
the nick we use to decide whether a particular message should be
highlighted. Obviously this can only work while we are connected,
so if we want to also start highlighting messages that are loaded
while disconnected, we need to use a different property to match
messages against - the account's nickname is probably the best we
have in this case.

Created attachment 331132 [details] [review]
room: Split out nick matching from should_highlight

We'll soon reuse the nick matching part of the should_highlight()
method, so split it out into a separate helper function.

Created attachment 331133 [details] [review]
room: Move casefolding into match function

On systems where strcasestr() is not available, we need to casefold
the text we search in for a case insensitive result. As we are about
to re-use the match helper function, it makes sense to move the
casefolding there instead of leaving it to all callers. It also
means casefolding is not longer tied to a TpMessage parameter that
soon will go away.

Created attachment 331134 [details] [review]
room: Use nick matching to filter out mentions from self

We do not want to highlight mentions in messages we sent ourselves.
To filter those out, we currently compare the message's :sender
property with the connection's :self-contact. This is another case
where our message highlighting is tied to being connected, so in
order to enable us to highlight mentions in logs as well, try to
match the sender's :alias property against the self-nick instead.

Created attachment 331135 [details] [review]
room: Change should_highlight() parameters

The should_highlight() method currently takes a TpMessage parameter,
which we get from the channel when a message is sent or received.
We don't get TpMessages from the logger though, so change method to
take sender and message text separately as strings instead, to make
it usable for log messages as well.

Created attachment 331136 [details] [review]
chatView: Highlight mentions in the backlog

We generally try to minimize differences between messages that we
sent or received, and messages that we fetched from the logs.
In other words, when restarting polari, the state after the restart
should match the one before as closely as possible. Besides status
messages that are not logged, the most glaring difference for now is
that we don't restore message highlights in the backlog. However
highlighting mentions is not only useful in the active session, it
can be just as helpful while skimming through the backlogs, so not
having them in logs is actually worse than introducing some visual
inconsistency. Luckily PolariRoom's should_highlight_message()
is now capable of handling this case, so we can finally fix this.

Review of attachment 331131 [details] [review]:

c is not my mother tongue but i read it through and things seems to make sense, just one question:

::: src/lib/polari-room.c
@@ +264,1 @@
+      nick = polari_util_get_basenick (tp_contact_get_alias (self));

is it necessary to polari_util_get_basenick() here if you are doing it below?

Review of attachment 331132 [details] [review]:

looks good to me, one question:

::: src/lib/polari-room.c
@@ +148,3 @@
+      gboolean starts_word, ends_word;
+
+      /* assume ASCII nicknames, so no complex pango-style breaks */

so i was reading that there is no real character encoding standard in irc[1] but most IRC clients speak UTF-8, does break if you used unicode characters in your nickname then?

[1]: https://en.wikipedia.org/wiki/Internet_Relay_Chat#Character_encoding

Review of attachment 331133 [details] [review]:

looks good to me

Review of attachment 331134 [details] [review]:

looks good to me

Review of attachment 331135 [details] [review]:

looks good to me

Review of attachment 331136 [details] [review]:

looks good to me

I also went over the patches and they look good to me.
As Bastian mentioned in #c7, shouldn't it be something like:
nick = tp_contact_get_alias (self);

(i.e. without the call to polari_util_get_basenick) ?

(In reply to Bastian Ilsø from comment #7)
> is it necessary to polari_util_get_basenick() here if you are doing it below?

Nope. It's not only redundant, it's leaking memory - thanks for the catch! Fixed locally.


(In reply to Bastian Ilsø from comment #8)
> so i was reading that there is no real character encoding standard in irc[1]
> but most IRC clients speak UTF-8, does break if you used unicode characters
> in your nickname then?

It shouldn't - it's looking at the character before and after the match, not at the nick itself. If anything, the use of g_ascii_isalnum() in polari_util_get_basenick() is probably more problematic. I'm not aware of any issues with this though, and at least the original RFC is quite specific about nicknames[0]:
   <nick>       ::= <letter> { <letter> | <number> | <special> }
   [...]
   <letter>     ::= 'a' ... 'z' | 'A' ... 'Z'
   <number>     ::= '0' ... '9'
   <special>    ::= '-' | '[' | ']' | '\' | '`' | '^' | '{' | '}'

So using arbitrary UTF-8 characters in nicknames is at least fishy - *messages* can use any character enconding (hopefully UTF-8), but as long as the encoding is compatible in the ASCII range, we should be fine.


[0] https://tools.ietf.org/html/rfc1459#section-2.3.1

Attachment 331131 [details] pushed as 3d588bb - room: Set self-nick in the offline case
Attachment 331132 [details] pushed as 931b2d9 - room: Split out nick matching from should_highlight
Attachment 331133 [details] pushed as ff1857f - room: Move casefolding into match function
Attachment 331134 [details] pushed as ad93983 - room: Use nick matching to filter out mentions from self
Attachment 331135 [details] pushed as 49f4254 - room: Change should_highlight() parameters
Attachment 331136 [details] pushed as 451f22c - chatView: Highlight mentions in the backlog