954746 - Own nickname (for pings) detection broken

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Benedikt Pfeifer]

*** Original post on bio 1314 at 2012-03-03 20:34:00 UTC ***

There's at least two cases where the own nickname detection is broken:

* detection is case sensitive ("mic" is not spotted when my nickname is "Mic")
* nickname is detected wherever it occurs, even at the beginning and inside of strings (i.e. I'm receiving a ping for "Microsoft", "aMicBlablabla", ..).

flo pointed here as possible cause of this bug: 

http://lxr.instantbird.org/instantbird/source/chat/modules/jsProtoHelper.jsm#529

> 529   writeMessage: function (aWho, aText, aProperties) {
> 530     aProperties.containsNick = aText.indexOf(this.nick) != -1;
> 531     GenericConversationPrototype.writeMessage.apply(this, arguments);
> 532   }

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 1314 at 2012-03-04 14:42:36 UTC ***

Out of curiosity, I wanted to see what libpurple does: http://lxr.instantbird.org/instantbird/source/purple/libpurple/util.c#4753

It seems like it's essentially searching for \b<this.nick>\b. We should probably build this regexp and use it instead. (Alternately you could do \b(<this.nick>|<this.nick.toLowerCase()>)\b, the issue I see with this is if both Mic and mic are in the room (which might be valid in certain protocols? I don't really know). Of course you could then say match Mic or mic unless mic is a different user. ;)

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 1314 at 2012-03-04 14:43:11 UTC ***

Also, this is not an IRC bug.

Comment 3

[aleth [:aleth]]

*** Original post on bio 1314 at 2012-03-04 22:03:05 UTC ***

It will be useful to check it still matches Mic1 (or whatever is used when the desired nick is currently not available on login).

Comment 4

[Patrick Cloke [:clokep]]

*** Original post on bio 1314 at 2012-03-05 13:15:47 UTC ***

Possibly this should also only apply to incoming messages (or at they very least should not apply to outgoing messages), to avoid self-pinging.

Comment 5

[aleth [:aleth]]

Attachment: Ping

*** Original post on bio 1314 as attmnt 1276 at 2012-03-28 21:39:00 UTC ***

Matches the nick plus any number of numbers (so that if there was a nick collision, there are no problems, at least once bug 954780 (bio 1346) is fixed). Also ignores case (I think it is best to be generous here, as you don't want to miss a ping because somebody pressed Shift too early or late or was lazy).

Comment 6

[aleth [:aleth]]

Attachment: Ping

*** Original post on bio 1314 as attmnt 1277 at 2012-03-28 21:48:00 UTC ***

Forgot to add clokep's suggestion from comment #4 ;)

Comment 7

[aleth [:aleth]]

Comment on attachment 8353029 [details] [diff] [review]
Ping

*** Original change on bio 1314 attmnt 1276 at 2012-03-28 21:48:51 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[aleth [:aleth]]

Comment on attachment 8353030 [details] [diff] [review]
Ping

*** Original change on bio 1314 attmnt 1277 at 2012-03-28 22:38:39 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Patrick Cloke [:clokep]]

*** Original post on bio 1314 at 2012-03-29 00:44:10 UTC ***

So I think this'll break if the nick contains any regexp special characters, you'd need to escape a few characters [1] seems to have a way to do this.

I'm not sure of my thoughts about including digits at the end. I need to think about that more...

[1] http://lxr.instantbird.org/instantbird/source/chat/modules/imSmileys.jsm#153

Comment 10

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1280 at 2012-03-29 19:04:00 UTC ***

- Adds a setter for the nick, which also sets the regexp to match for pings.
- Include regexp to escape special characters in the nick string.
- Have the IRC Chat conversation notice the nick change.

Clokep may have some additional improvements to add to the third part.

Comment 11

[aleth [:aleth]]

Comment on attachment 8353030 [details] [diff] [review]
Ping

*** Original change on bio 1314 attmnt 1277 at 2012-03-29 19:04:39 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1281 at 2012-03-29 19:16:00 UTC ***

Just a nit fix.

Comment 13

[aleth [:aleth]]

Comment on attachment 8353033 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1280 at 2012-03-29 19:16:12 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8353034 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1281 at 2012-03-29 19:55:45 UTC ***

>diff --git a/components/irc.js b/components/irc.js

>@@ -489,50 +489,60 @@ const GenericConvIMPrototype = {
> const GenericConvChatPrototype = {
>   __proto__: GenericConversationPrototype,
>   _interfaces: [Ci.prplIConversation, Ci.prplIConvChat],
>   classDescription: "generic ConvChat object",
>
>   _nick: null,
>   _topic: null,
>   _topicSetter: null,
>+  _pingRegexp: null,
>   setTopic: function(aTopic, aTopicSetter) {

Nit: This doesn't seem really needed, but I don't really mind if you want to add it (or remove the _nick default too), but if you add it, don't separate the topic setTopic method and the defaults it replaces.

If you want to keep the _nick, I think it should be moved closer to the nick setter.


>   get left() false,

This area of the file has changed recently, your patch is already bitrotted :(.

>   writeMessage: function (aWho, aText, aProperties) {
>-    aProperties.containsNick = aText.indexOf(this.nick) != -1;
>+    if (aWho != this.nick)
>+      aProperties.containsNick = aText.match(this._pingRegexp) != null;
>+    else
>+      aProperties.containsNick = false;

Another nit: What about:
aProperties.containsNick =
  aWho != this.nick && !!aText.match(this._pingRegexp);

I wonder if we should check for |"incoming" in aProperties| instead of |aWho != this.nick|.


Another alternative is creating the regexp lazily when you are about to write an incoming message to the conversation (in this case the nick setter will only |delete this._pingRegexp|, and the null default is useful if you don't want to check for .hasOwnProperty.

(I hesitated between - and + for the flags, as this looks mostly good, but I'd like to look at the next iteration :))

Comment 15

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1282 at 2012-03-29 20:39:00 UTC ***

Testing for "incoming" is not good if we want to be pinged by system messages.

I decided against the lazy regexp creation as the creation will only happen once for twitter, whereas the checks whether it has been created would be much more frequent.

Removed the default for _nick as a warning would actually be useful in the unlikely event that this is not set.

Comment 16

[aleth [:aleth]]

Comment on attachment 8353034 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1281 at 2012-03-29 20:39:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Benedikt Pfeifer]

*** Original post on bio 1314 at 2012-03-29 20:49:57 UTC ***

I think this 

 aWho != this.nick && !!aText.match(this._pingRegexp);

could be replaced by 

 aWho != this.nick && this._pingRegexp.test(aText);


I took that hint from the docs for match:

"If you need to know if a string matches a regular expression regexp, use regexp.test(string)."

Comment 18

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1283 at 2012-03-29 20:56:00 UTC ***

Turns out there are actually no system messages currently where it would be useful to be pinged (in fact, the response to /nick seems to be the only one).

Comment 19

[aleth [:aleth]]

Comment on attachment 8353035 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1282 at 2012-03-29 20:56:15 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 20

[Florian Quèze [:florian]]

*** Original post on bio 1314 at 2012-03-29 21:04:35 UTC ***

I think I agree with comment 12 (a potential down side is that if _pingRegexp isn't initialized because a protocol plugin overrides _init (or doesn't call it) without setting the nick, we will fail to display the message because of a JS error. I guess this will force protocol plugin authors to write better code :-P)

My only other nit is that I think the _init method and the isChat getter should not be that far down in the implementation. I would prefer them to be before the topic and nick blocks. Having them after the default values was quite ok, and then we added the setTOpic function, and now some more... Probably time to move these 2 :).

Comment 21

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1284 at 2012-03-29 21:23:00 UTC ***

Thanks for the tip Mic :)

Comment 22

[aleth [:aleth]]

Comment on attachment 8353036 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1283 at 2012-03-29 21:23:52 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 8353037 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1284 at 2012-03-29 23:13:16 UTC ***

(In reply to comment #11)
> Created attachment 8353035 [details] [diff] [review] (bio-attmnt 1282) [details]
> Testing for "incoming" is not good if we want to be pinged by system messages.
I do not think we should ping for system messages. I could be convinced otherwise though.

>diff --git a/components/irc.js b/components/irc.js
>@@ -172,16 +172,20 @@ ircChannel.prototype = {
>   updateNick: function(aOldNick, aNewNick) {
>     if (!this.hasParticipant(aOldNick)) {
>       ERROR("Trying to rename nick that doesn't exist! " + aOldNick + " to " +
>             aNewNick);
>       return;
>     }
> 
>+    // If this is the user's nick, change it.
>+    if (aOldNick == this.nick)
>+      this.nick = aNewNick;
These should be normalized I think.

>diff --git a/modules/jsProtoHelper.jsm b/modules/jsProtoHelper.jsm
>@@ -488,41 +488,47 @@ const GenericConvIMPrototype = {
>+  set nick(aNick) {
>     this._nick = aNick;
>-    GenericConversationPrototype._init.call(this, aAccount, aName);
>+    let escapedNick = this._nick.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, "\\$&");
>+    this._pingRegexp = new RegExp("\\b" + escapedNick + "\\d*\\b", "i");
>   },
I'm still not convinced about the \d* here, is everyone else OK with this?

Should we have a general function somewhere which escapes a string for use in Regular expressions? I dislike seeing code (which is as cryptic as this) in multiple places? Also, should we make this a constant regular expression somewhere instead of regenerating it for each conversation?

Comment 24

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1314 as attmnt 1286 at 2012-03-30 12:41:00 UTC ***

- Compare normalized nicks for the off-chance that the server returns them in some normalized fashion.
- Remove /d: leave the possibility of matching nick collision variants for bug 954780 (bio 1346).

Comment 25

[aleth [:aleth]]

Comment on attachment 8353037 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1284 at 2012-03-30 12:41:15 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 26

[Patrick Cloke [:clokep]]

Comment on attachment 8353039 [details] [diff] [review]
Patch

*** Original change on bio 1314 attmnt 1286 at 2012-03-30 13:37:08 UTC ***

This looks good to me. Thanks for fixing this annoying bug!

Comment 27

[Patrick Cloke [:clokep]]

*** Original post on bio 1314 at 2012-04-04 10:38:10 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/a0d791471fe6

Thanks!