954804 - Avoid flashing the wrong case when double clicking on a participant with JS-IRC

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2012-04-10 19:30:00 UTC ***

Yeah, it's not supposed to return a normalized name. We should probably expand the comment at http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#125 to make it clear what is expected.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch

*** Original post on bio 1370 as attmnt 1325 at 2012-04-10 22:56:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 2

[Florian Quèze [:florian]]

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

*** Original change on bio 1370 attmnt 1325 at 2012-04-11 09:30:24 UTC ***

These changes look OK, but it would be nice to take the opportunity to add a comment in the idl file so that no other prpl author does the same mistake in the future.

And maybe rephrase the bug summary a bit, so that it makes sense as a commit message. :)

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2012-04-11 10:28:30 UTC ***

(In reply to comment #2)
> Comment on attachment 8353078 [details] [diff] [review] (bio-attmnt 1325) [details]
> Patch
> 
> These changes look OK, but it would be nice to take the opportunity to add a
> comment in the idl file so that no other prpl author does the same mistake in
> the future.
The comment in the idl is:
> The normalized chat buddy name will be suitable for starting a
> private conversation or calling requestBuddyInfo.
I think this is actually fine, I just never saw it as I grabbed most of the parameters from jsProtoHelper. This does accurately describe how it is used.

> And maybe rephrase the bug summary a bit, so that it makes sense as a commit
> message. :)
Done.

Comment 4

[aleth [:aleth]]

*** Original post on bio 1370 at 2012-04-11 16:21:07 UTC ***

This will break http://lxr.instantbird.org/instantbird/source/instantbird/content/buddytooltip.xml#371.

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2012-04-11 18:51:48 UTC ***

(In reply to comment #4)
> This will break
> http://lxr.instantbird.org/instantbird/source/instantbird/content/buddytooltip.xml#371.

So we just need to run this._account.normalize as part of requestBuddyInfo? http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#551

Comment 6

[aleth [:aleth]]

*** Original post on bio 1370 at 2012-04-11 19:07:30 UTC ***

requestBuddyInfo already has normalize calls where necessary.

If this.observedUserInfo does not contain the normalized nick, then for capitalized nicks the observer will not recognize the event carrying the returned information. (It makes no sense to have this mechanism use non-normalized nicks as the correct non-normalized nick might not be available until after the call in other situations where this event is used.)

Comment 7

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2012-04-24 23:53:23 UTC ***

Did we ever decide if there is a solution to this? Or is this WONTFIX?

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Patch v2

*** Original post on bio 1370 as attmnt 2341 at 2013-04-11 02:19:00 UTC ***

This became not so trivial. :(

Note that this includes some general clean up of the WHOIS stuff, it removes a function that was just a simplified version of another one and inlines another function that was only called once.

I tested it with:
/whois of someone online
/whois of someone offline
Tooltip of someone with capital letters
/whois of a crazy case
/whois of a fake nick

Comment 9

[Patrick Cloke [:clokep]]

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

*** Original change on bio 1370 attmnt 1325 at 2013-04-11 02:19:13 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 10

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2013-04-11 14:29:40 UTC ***

A better description is necessary:
The bug is that when you double click on a participant we currently start a conversation with the normalized name, then when we get who is information the capitalization is updated. This is wrong and looks bad.

This patch attempts to open the conversation with the correct capitalization, always. Of course the case still exists that if you open it via /msg you can still give it crazy capitalization.

Comment 11

[aleth [:aleth]]

Comment on attachment 8354108 [details] [diff] [review]
Patch v2

*** Original change on bio 1370 attmnt 2341 at 2013-04-11 15:35:53 UTC ***

Thanks for tackling the long-standing normalize* bugs!

>The comment in the idl is:
>> The normalized chat buddy name will be suitable for starting a
>> private conversation or calling requestBuddyInfo.
>I think this is actually fine, I just never saw it as I grabbed most of the
>parameters from jsProtoHelper. This does accurately describe how it is used.
Since it wasn't completely clear to me when I just read it, how about expanding it to something like
"This normalized chat buddy name will be used by the UI for starting a private conversation or calling requestBuddyInfo (and observing the returned user-info-received event)."
Makes it clear it's nothing to do with logs etc... ;)

>     let nick = this._account.normalize(aData);
>     let nickIndex = this._observedNicks.indexOf(nick);
>     if (nickIndex == -1)
>       return;
>+
>+    // Remove the nick from the list of nicks that are being waited to received.
>     this._observedNicks.splice(nickIndex, 1);
So if we are tidying up whois anyway and this is hard enough to read to warrant a comment, let's just turn _observedNicks into a Set instead and get rid of splice and indexOf == -1.

We could save a ton of hasOwnProperties in the existing code by turning whoisInformation into a Map, but I haven't looked in detail to see if this would be worth it.

>+    if (!hasOwnProperty(account.whoisInformation, nick) ||
>+        Object.keys(account.whoisInformation[nick]).length <= 2) {
>+      this.writeMessage(null, _("message.unknownNick", nick), type);
>+      return;
What is the length <= 2 check for? I think I can guess, but it needs a comment.

>   // Request WHOWAS information on a buddy when the user requests more
>   // information.
>   requestOfflineBuddyInfo: function(aBuddyName) {
>-    this.removeBuddyInfo(aBuddyName);
>     this.sendMessage("WHOWAS", aBuddyName);
This change means requestOfflineBuddyInfo should never be called unless in the context of a requestBuddyInfo call (or unless calling removeBuddyInfo by hand first). That's the case at the moment, but does it mean we should add this to the comment and/or call it _requestOfflineBuddyInfo?

>-  // Set minimal WHOIS entry containing only the capitalized nick,
>-  // if no WHOIS info exists already.
>-  setWhoisFromNick: function(aNick) {
>-    let nick = this.normalize(aNick);
>-    if (!hasOwnProperty(this.whoisInformation, nick))
>-      this.whoisInformation[nick] = {"nick": aNick};
>-  },
Nice to get rid of that :)

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>-      if (hasOwnProperty(this.whoisInformation, nick)) {
>-        Services.obs.notifyObservers(this.getBuddyInfo(nick),
>-                                     "user-info-received", nick);
>-      }
>+      // We always expect two keys: the requested nick capitalization and the
>+      if (hasOwnProperty(this.whoisInformation, nick) &&
>+          Object.keys(this.whoisInformation[nick]).length > 2)
>+        this.notifyWhoIs(nick);
This looks like it wanted to be a comment like the one I requested above, but it must have got cut off.

Nit: if it's setWhois, it should be notifyWhois (capitalization).

There is a potential problem here if two requestBuddyInfo calls happen in quick succession, before we receive the result for the first, and the capitalization in the two calls differed. We should then send two user-info-received events rather than one (as we do at present).

Comment 12

[aleth [:aleth]]

*** Original post on bio 1370 at 2013-04-12 09:09:30 UTC ***

I was thinking about this a bit more last night.

I figured out why I was confused at the flashing when opening a provate conv from the nicklist. We make sure that the nick capitalization is stored in the whois table for all participants.
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#282
The ircConversation constructor then grabs it and uses it. So there should be no flashing. Which means there must be a race condition and the new tab may be created before the ircConversation.

So what do you think of the following:
1) Keep all the whois cleanup and improvements in your patch
2) Keep normalizeNick as it is much more readable to not use getNormalizedChatBuddyName internally
3) Drop the 'requestedNick' hack and define getNormalizedChatBuddyName: function(nick) normalizeNick(nick)
4) Instead, in getConversation(), if creating a new conversation, check if the aName exists in the whois table, and if so, use the nick from there to open the conversation. This should avoid the race condition.

Comment 13

[Patrick Cloke [:clokep]]

Attachment: Patch v3

*** Original post on bio 1370 as attmnt 2366 at 2013-04-16 01:51:00 UTC ***

(In reply to comment #11)
> So what do you think of the following:
> 1) Keep all the whois cleanup and improvements in your patch
> 2) Keep normalizeNick as it is much more readable to not use
> getNormalizedChatBuddyName internally
> 3) Drop the 'requestedNick' hack and define getNormalizedChatBuddyName:
> function(nick) normalizeNick(nick)
> 4) Instead, in getConversation(), if creating a new conversation, check if the
> aName exists in the whois table, and if so, use the nick from there to open the
> conversation. This should avoid the race condition.

I think this is a much better way of doing this! Good thought!

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8354108 [details] [diff] [review]
Patch v2

*** Original change on bio 1370 attmnt 2341 at 2013-04-16 01:51:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[aleth [:aleth]]

Comment on attachment 8354133 [details] [diff] [review]
Patch v3

*** Original change on bio 1370 attmnt 2366 at 2013-04-16 12:09:34 UTC ***

>+  notifyWhois: function(aNick) {
>+    Services.obs.notifyObservers(this.getBuddyInfo(aNick), "user-info-received",
>+                                 this.normalize(aNick));
>+  },
Nit: Should use normalizeNick instead of normalize.

Please include an improved prplnormalizechatbuddy... comment in the idl :D

Looks great otherwise :)

Comment 16

[Patrick Cloke [:clokep]]

Attachment: Patch v4

*** Original post on bio 1370 as attmnt 2375 at 2013-04-16 23:59:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Patrick Cloke [:clokep]]

Comment on attachment 8354133 [details] [diff] [review]
Patch v3

*** Original change on bio 1370 attmnt 2366 at 2013-04-16 23:59:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[aleth [:aleth]]

Comment on attachment 8354142 [details] [diff] [review]
Patch v4

*** Original change on bio 1370 attmnt 2375 at 2013-04-17 11:52:49 UTC ***

>   /* The normalized chat buddy name will be suitable for starting a
>-     private conversation or calling requestBuddyInfo. */
>+     private conversation or calling requestBuddyInfo. Generally this can just
Let's be specific: ...calling createConversation to start a private conversation or...

>+     be aChatBuddyName, but some protocols (e.g. XMPP) use this strip out parts
>+     of the name (e.g. the resource). */
typo: use this *to*

>   observe: function(aSubject, aTopic, aData) {
>     if (aTopic != "user-info-received")
>       return;
> 
>     let nick = this._account.normalize(aData);
Should this be normalizeNick?

>-  setWhois: function(aNick, aFields) {
>+  setWhois: function(aNick, aFields = {}) {
>     let nick = this.normalize(aNick, this.userPrefixes);
^^ ditto

Comment 19

[Patrick Cloke [:clokep]]

Attachment: Patch v5

*** Original post on bio 1370 as attmnt 2417 at 2013-04-27 15:24:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8354142 [details] [diff] [review]
Patch v4

*** Original change on bio 1370 attmnt 2375 at 2013-04-27 15:24:33 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 21

[aleth [:aleth]]

Comment on attachment 8354184 [details] [diff] [review]
Patch v5

*** Original change on bio 1370 attmnt 2417 at 2013-04-27 15:51:46 UTC ***

Thanks!

Comment 22

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2013-05-21 20:00:02 UTC ***

Apparently I never updated the description in the patch. Oh well.

http://hg.instantbird.org/instantbird/rev/e044d6f9acd3

Comment 23

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2013-05-21 20:05:21 UTC ***

Fix bad merge:
http://hg.instantbird.org/instantbird/rev/67f647128510

Comment 24

[aleth [:aleth]]

Attachment: Followup patch to fix bustage

*** Original post on bio 1370 as attmnt 2442 at 2013-05-22 13:42:00 UTC ***

Fix "this.normalizeNick is not a function" errors.

Comment 25

[aleth [:aleth]]

Comment on attachment 8354209 [details] [diff] [review]
Followup patch to fix bustage

*** Original change on bio 1370 attmnt 2442 at 2013-05-22 13:43:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 26

[aleth [:aleth]]

Attachment: Followup to fix bustage

*** Original post on bio 1370 as attmnt 2443 at 2013-05-22 13:47:00 UTC ***

Moves normalizeNick() to the GenericIRCConversation "prototype" instead.

Comment 27

[aleth [:aleth]]

Comment on attachment 8354209 [details] [diff] [review]
Followup patch to fix bustage

*** Original change on bio 1370 attmnt 2442 at 2013-05-22 13:50:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 8354210 [details] [diff] [review]
Followup to fix bustage

*** Original change on bio 1370 attmnt 2443 at 2013-05-22 13:50:48 UTC ***

Looks good, thanks for fixing this.

Comment 29

[Patrick Cloke [:clokep]]

*** Original post on bio 1370 at 2013-05-22 23:08:44 UTC ***

(In reply to comment #21)
> Created attachment 8354210 [details] [diff] [review] (bio-attmnt 2443) [details]
> Followup to fix bustage
> 
> Moves normalizeNick() to the GenericIRCConversation "prototype" instead.

Checked in as http://hg.instantbird.org/instantbird/rev/23c8630dd29e

Hopefully that's the last followup. :)