Closed Bug 954753 Opened 8 years ago Closed 8 years ago

Display name is lower case for IRC DMs

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(2 files, 11 obsolete files)

*** Original post on bio 1321 at 2012-03-06 13:55:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached image Screenshot
*** Original post on bio 1321 as attmnt 1225 at 2012-03-06 13:55:00 UTC ***

See screenshot:

- The displayed nick is normalized to lowercase

- I doubt the status icon (unknown) is accurate for a contact we know to be online. Maybe it should be "Unavailable" if the nick is away, "Available" if not?

- Sure, the status message is "Unknown", but do we have to say so? Why not just leave it blank?

- No way to discover which IRC server/account we are chatting on (e.g. in the account tooltip it only says "morian (IRC)")
*** Original post on bio 1321 at 2012-03-06 14:09:01 UTC ***

Re the last point, channels already say "#room via user@server (IRC)" in the tooltip, this would be sufficient.
*** Original post on bio 1321 at 2012-03-06 14:16:24 UTC ***

(In reply to comment #0)
> - I doubt the status icon (unknown) is accurate for a contact we know to be
> online. Maybe it should be "Unavailable" if the nick is away, "Available" if
> not?
This is bug 954730 (bio 1298).

> - Sure, the status message is "Unknown", but do we have to say so? Why not just
> leave it blank?
This is not IRC code.

> - No way to discover which IRC server/account we are chatting on (e.g. in the
> account tooltip it only says "morian (IRC)")
I have no idea where this tooltip is generated, but yes...that doesn't seem correct.
*** Original post on bio 1321 at 2012-03-06 14:25:29 UTC ***

(In reply to comment #0)

> - Sure, the status message is "Unknown", but do we have to say so? Why not just
> leave it blank?

The status message is indeed left blank. When there's no status message, the UI displays instead a text version of the status icon, so this point is actually the same as the previous one about the unknown status icon.
*** Original post on bio 1321 at 2012-03-06 14:41:21 UTC ***

(In reply to comment #3)
> The status message is indeed left blank. When there's no status message, the UI
> displays instead a text version of the status icon, so this point is actually
> the same as the previous one about the unknown status icon.

Great, that reduces this bug down to fixing the tooltip :)
Summary: Improve IRC DM convtop → Account tooltip does not display account/server for IRC DMs
*** Original post on bio 1321 at 2012-03-06 14:44:06 UTC ***

(and the display name)
Summary: Account tooltip does not display account/server for IRC DMs → Account tooltip and display name not quite correct for IRC DMs
*** Original post on bio 1321 at 2012-03-06 21:46:33 UTC ***

For the tooltip, I would propose removing the distinction between Chat and IM in http://lxr.instantbird.org/instantbird/source/instantbird/locales/en-US/chrome/instantbird/instantbird.properties#85, then http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#1201 would also include this._conv.account.name. This would also be helpful if one has grouped e.g. two buddies on different XMPP accounts under the same contact.

Is this wanted?
*** Original post on bio 1321 at 2012-03-06 22:02:44 UTC ***

(In reply to comment #6)
> For the tooltip, I would propose removing the distinction between Chat and IM
> in [...]

This distinction is here to "hide" bug 953652 (bio 206).

> Is this wanted?

I would say no (until we have a solution for bug 953652 (bio 206)).
*** Original post on bio 1321 at 2012-03-06 23:49:11 UTC ***

(In reply to comment #7)
> I would say no (until we have a solution for bug 953652 (bio 206)).

I'm not sure it wouldn't be useful to provide a way to discover the account used for the conversation, even if there is no way to change it.
*** Original post on bio 1321 at 2012-03-06 23:50:24 UTC ***

The display name is set to be the normalized nick rather than just the nick in
http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#1365
which explains the lower case.

I don't know what the reason for that was, but there will have been one ;)
*** Original post on bio 1321 at 2012-03-06 23:53:13 UTC ***

(In reply to comment #9)
> The display name is set to be the normalized nick rather than just the nick in
> http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#1365
> which explains the lower case.

I don't see how this code is related to the display name.
*** Original post on bio 1321 at 2012-03-07 00:07:11 UTC ***

(In reply to comment #10)
> (In reply to comment #9)
> > The display name is set to be the normalized nick rather than just the nick in
> > http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#1365
> > which explains the lower case.
> 
> I don't see how this code is related to the display name.

createConversation leads to here, where the aName passed becomes the conversation display name:
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#709
*** Original post on bio 1321 at 2012-03-07 00:27:16 UTC ***

(In reply to comment #11)
01:12:39 AM - flo: the point of "getNormalizedChatBuddyName" (which is probably undocumented) is to convert a nickname that is valid only inside a chatroom into a buddy name that is valid for using directly on the account
01:12:48 AM - flo: it's useful for XMPP MUCs

Therefore the code in conversation.xml shouldn't be changed. So either irc.js:getNormalizedChatBuddyName should return the non-normalized name, or irc.js:getConversation has to find a way to "unnormalize" the name it is passed. But getConversation seems to expect an unnormalized name as its parameter, so I suspect the former is correct. But I think clokep had better decide what to do here ;)
*** Original post on bio 1321 at 2012-03-07 01:14:21 UTC ***

(In reply to comment #8)
> (In reply to comment #7)
> I'm not sure it wouldn't be useful to provide a way to discover the account
> used for the conversation, even if there is no way to change it.

Just noticed it's given in the tab tooltip.
Summary: Account tooltip and display name not quite correct for IRC DMs → Display name is lower case for IRC DMs
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1229 at 2012-03-07 14:31:00 UTC ***

Not quite perfect as e.g. "/msg nickserv" will still lead to a conversation with display name "nickserv" rather than "NickServ".
Attachment #8352982 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1230 at 2012-03-07 22:35:00 UTC ***

A bit less duplication. Still can't see a neat way to check whether a nick exists on the server apart from the channel's participant list.
Attachment #8352983 - Flags: review?(clokep)
Comment on attachment 8352982 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1229 at 2012-03-07 22:35:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352982 - Attachment is obsolete: true
Attachment #8352982 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1231 at 2012-03-07 22:49:00 UTC ***

Noticed a little bug, as always just after submitting ;) Didn't check whether /msg or /query was sent from a MUC.
Attachment #8352984 - Flags: review?(clokep)
Comment on attachment 8352983 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1230 at 2012-03-07 22:49:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352983 - Attachment is obsolete: true
Attachment #8352983 - Flags: review?(clokep)
*** Original post on bio 1321 at 2012-03-08 02:01:18 UTC ***

(In reply to comment #16)
> Created attachment 8352984 [details] [diff] [review] (bio-attmnt 1231) [details]
> Patch
> 
> Noticed a little bug, as always just after submitting ;) Didn't check whether
> /msg or /query was sent from a MUC.

I would prefer if the getNormalizedNick was called something else (normalizeNick perhaps?)

I'm confused at the changes to ircCommands.jsm, why were these made?

OK, so I think what we need to do is...just remove the definition of getNormalizedChatBuddyName and fall back to the jsProto version [1] and add a comment to the idl file about what getNormalizedChatBuddyName is for. As far as I can tell, everything should just "work" after that, as IRC always normalizes names before we create conversations, etc. We then generate the conversation w/ the unnormalized name...that's my guess at least.

[1] http://lxr.instantbird.org/instantbird/source/chat/modules/jsProtoHelper.jsm#527
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1232 at 2012-03-08 10:31:00 UTC ***

(In reply to comment #17)
> I'm confused at the changes to ircCommands.jsm, why were these made?

It is possible to start a DM via the query or msg commands. In this case, currently the display name of the new conversation is simply whatever the user typed in the command. The changes attempt to get the correct case version of the nick from the participant list.

> OK, so I think what we need to do is...just remove the definition of
> getNormalizedChatBuddyName and fall back to the jsProto version [1] and add a
> comment to the idl file about what getNormalizedChatBuddyName is for. As far as
> I can tell, everything should just "work" after that, as IRC always normalizes
> names before we create conversations, etc. We then generate the conversation w/
> the unnormalized name...that's my guess at least.

That's also why this change, which I had tried initially, is not enough. (Note getNormalizedChatBuddyName does what the jsProto version does if it can't find the nick in the participant list).
Attachment #8352985 - Flags: review?(clokep)
Comment on attachment 8352984 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1231 at 2012-03-08 10:31:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352984 - Attachment is obsolete: true
Attachment #8352984 - Flags: review?(clokep)
Comment on attachment 8352985 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1232 at 2012-03-08 12:42:11 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>-  getNormalizedChatBuddyName: function(aNick)
>+  normalizeNick: function(aNick)
>     this._account.normalize(aNick, this._account.userPrefixes),
After further discussion on IRC, I think this does make sense to just call normalize.

So after thinking on this more, I think the best way to handle this is in the constructor of the conversation. We can still do checks similar to what you did (if we have the buddy, we can get their name), otherwise I think we should do a WHOIS on the nick and in the response check if we have a conversation with the nick, if so...update the title of the conversation. (Plus this will initially cache the WHOIS information for us...if that's useful.)
Attachment #8352985 - Flags: review?(clokep) → review-
Depends on: 954764
*** Original post on bio 1321 at 2012-03-15 11:47:42 UTC ***

I marked this 1.2-blocking as it's very visible, but I guess we should still ship without it, so it's just really-really-wanted ;).
Whiteboard: [1.2-blocking]
*** Original post on bio 1321 at 2012-04-03 12:31:40 UTC ***

Assigning to aleth.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1299 at 2012-04-05 14:47:00 UTC ***

- Fetches conversation name via whois.
Attachment #8353052 - Flags: review?(clokep)
Comment on attachment 8352985 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1232 at 2012-04-05 14:47:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352985 - Attachment is obsolete: true
*** Original post on bio 1321 at 2012-04-05 15:11:58 UTC ***

Comment on attachment 8353052 [details] [diff] [review] (bio-attmnt 1299)
Patch

>diff --git a/components/irc.js b/components/irc.js
>index 72b45e0..f74bc06 100644
>--- a/components/irc.js
>+++ b/components/irc.js
>@@ -227,20 +227,22 @@ ircChannel.prototype = {
>-  waitForBuddyInfo: function(aNick) {
>+  requestBuddyInfo: function(aNick) {
>     if (!this._observedNicks.length)
>       Services.obs.addObserver(this, "user-info-received", false);
>-    this._observedNicks.push(this._account.normalize(aNick));
>+    let nick = this._account.normalize(aNick);
>+    this._observedNicks.push(nick);

>+    this._account.requestBuddyInfo(nick);
This doesn't need the normalized buddy name.

>@@ -298,16 +300,22 @@ ircParticipant.prototype = {
> function ircConversation(aAccount, aName) {
>   if (aAccount.hasBuddy(aName))
>     this.buddy = aAccount.getBuddy(aName);
>
>   this._init(aAccount, aName);
>   this._observedNicks = [];
>+
>+  // Fetch correctly capitalized name.
>+  if (hasOwnProperty(this._account.whoisInformation, this.normalizedName))
>+    this.updateNick(this._account.whoisInformation[this.normalizedName]["nick"]);
>+  this._fetchName = true;
This isn't defined on the prototype. Why do we even need this?

>+  this.requestBuddyInfo(aName);
If we already have the information (with the updated nick above), do we want to request this info again?


You've forked the observer! :(
> ircConversation.prototype = {
[...]
>   observe: function(aSubject, aTopic, aData) {
>     if (aTopic != "user-info-received")
>       return;
>
>-    let nickIndex = this._observedNicks.indexOf(this._account.normalize(aData));
>+    aData = this._account.normalize(aData);
I would prefer this being set to a better name ("normalizedNick" or even just "nick" probably).

>+    let nickIndex = this._observedNicks.indexOf(aData);
>     if (nickIndex == -1)
>       return;
>     this._observedNicks.splice(nickIndex, 1);
>     if (!this._observedNicks.length)
>       Services.obs.removeObserver(this, "user-info-received");
>+    if (this._fetchName && aData == this.normalizedName) {
Can't we just check if the name matches and if we have whois info and set the nick? What does _fetchName do? If it's simply to not print out the results...a comment is needed somewhere.
>+      if (hasOwnProperty(this._account.whoisInformation, aData))
>+        this.updateNick(this._account.whoisInformation[aData]["nick"]);
>+      delete this._fetchName;
>+      return;
>+    }
>     this._account.writeWhois(this, aData,
>                              aSubject.QueryInterface(Ci.nsISimpleEnumerator));
>   }
*** Original post on bio 1321 at 2012-04-05 16:00:35 UTC ***

(In reply to comment #23)
> >+  // Fetch correctly capitalized name.
> >+  if (hasOwnProperty(this._account.whoisInformation, this.normalizedName))
> >+    this.updateNick(this._account.whoisInformation[this.normalizedName]["nick"]);
> >+  this._fetchName = true;
> This isn't defined on the prototype. Why do we even need this?

It's not defined on the prototype as it's deleted after use.
You need it to let the observer know not to print the whois entry for the conversation name as a system message in this case.

> >+  this.requestBuddyInfo(aName);
> If we already have the information (with the updated nick above), do we want to
> request this info again?

Yes, in the (admittedly really very unlikely) case the nick info we had was from somebody else who had the same normalized nick and left the conversation earlier.

> You've forked the observer! :(
Not really, as this._fetchName will simply be undefined for the chat case. I just didn't change the chat one.

> >+    if (this._fetchName && aData == this.normalizedName) {
> Can't we just check if the name matches and if we have whois info and set the
> nick? What does _fetchName do? If it's simply to not print out the results...a
> comment is needed somewhere.

See above.
*** Original post on bio 1321 at 2012-04-05 16:18:45 UTC ***

(In reply to comment #24)
> (In reply to comment #23)
> > >+  // Fetch correctly capitalized name.
> > >+  if (hasOwnProperty(this._account.whoisInformation, this.normalizedName))
> > >+    this.updateNick(this._account.whoisInformation[this.normalizedName]["nick"]);
> > >+  this._fetchName = true;
> > This isn't defined on the prototype. Why do we even need this?
> 
> It's not defined on the prototype as it's deleted after use.
> You need it to let the observer know not to print the whois entry for the
> conversation name as a system message in this case.
Right, so you define it in the prototype as false and then delete it. That's the normal way we handle this. Please add a comment about what _fetchName is used for (and I'm not sure fetchName is the best name...but nothing better is coming to me).

> > >+  this.requestBuddyInfo(aName);
> > If we already have the information (with the updated nick above), do we want to
> > request this info again?
> 
> Yes, in the (admittedly really very unlikely) case the nick info we had was
> from somebody else who had the same normalized nick and left the conversation
> earlier.
Ah, yes! Please add a comment about that?

> > You've forked the observer! :(
> Not really, as this._fetchName will simply be undefined for the chat case. I
> just didn't change the chat one.
Try, but it'll throw a warning about accessing this._fetchName, which is undefined. (See above. ;))
Comment on attachment 8353052 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1299 at 2012-04-05 16:19:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353052 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1301 at 2012-04-05 18:39:00 UTC ***

(In reply to comment #25)
> Right, so you define it in the prototype as false and then delete it. That's
> the normal way we handle this.
> 
> > > You've forked the observer! :(
> > Not really, as this._fetchName will simply be undefined for the chat case. I
> > just didn't change the chat one.
> Try, but it'll throw a warning about accessing this._fetchName, which is
> undefined. (See above. ;))

I don't think it throws a warning as I never access the value, but I've changed it to have a default and work via true/false instead.
Attachment #8353054 - Flags: review?(clokep)
Comment on attachment 8353052 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1299 at 2012-04-05 18:39:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353052 - Attachment is obsolete: true
*** Original post on bio 1321 at 2012-04-05 18:51:02 UTC ***

Comment on attachment 8353054 [details] [diff] [review] (bio-attmnt 1301)
Patch

>+    // If we are waiting for the conversation name, set it.
>+    if (this._waitingForName && nick == this.normalizedName) {
>+      if (hasOwnProperty(this._account.whoisInformation, nick))
>+        this.updateNick(this._account.whoisInformation[nick]["nick"]);
>+      this._waitingForName = false;
Usually we would delete this._waitingForName.  I'll fix this before committing it if I get commit access...or ask flo to do it. I need to test this though, it looks good! Thanks for renaming those variables.
Attached patch Patch with fixed paths & delete (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1303 at 2012-04-06 00:41:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353056 - Flags: review?(bugzilla)
Comment on attachment 8353054 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1301 at 2012-04-06 00:42:49 UTC ***

This looks good! Only change I made was using delete instead of setting the _waitingForName back to false.
Attachment #8353054 - Flags: review?(clokep) → review+
Comment on attachment 8353056 [details] [diff] [review]
Patch with fixed paths & delete

*** Original change on bio 1321 attmnt 1303 at 2012-04-06 00:43:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353056 - Flags: review?(bugzilla) → review+
*** Original post on bio 1321 at 2012-04-06 00:52:14 UTC ***

Please check in attachment 8353056 [details] [diff] [review] (bio-attmnt 1303)! Thanks. :)
Whiteboard: [1.2-blocking] → [1.2-blocking][checkin-needed]
Comment on attachment 8353056 [details] [diff] [review]
Patch with fixed paths & delete

*** Original change on bio 1321 attmnt 1303 at 2012-04-06 23:22:20 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+  // Fetch correctly capitalized name.
>+  // Always request the info as it may be out of date.
>+  if (hasOwnProperty(this._account.whoisInformation, this.normalizedName))
>+    this.updateNick(this._account.whoisInformation[this.normalizedName]["nick"]);
>+  this._waitingForName = true;
>+  this.requestBuddyInfo(aName);

It seems this will cause some avoidable flickering when starting a conversation with someone (not in the buddy list) from the participant list of a channel.

r- as per IRC discussion it seems this is easily fixable. If it turns out to be more complicated than expected, I'm not completely opposed to checking in this patch as is.
Attachment #8353056 - Flags: review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1318 at 2012-04-07 12:47:00 UTC ***

- Maintain (minimal) whois entries for all participants.
Attachment #8353071 - Flags: review?(clokep)
Comment on attachment 8353054 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1301 at 2012-04-07 12:47:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353054 - Attachment is obsolete: true
Comment on attachment 8353056 [details] [diff] [review]
Patch with fixed paths & delete

*** Original change on bio 1321 attmnt 1303 at 2012-04-07 12:47:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353056 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1319 at 2012-04-07 12:48:00 UTC ***

Fix diff.
Attachment #8353072 - Flags: review?(clokep)
Comment on attachment 8353071 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1318 at 2012-04-07 12:48:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353071 - Attachment is obsolete: true
Attachment #8353071 - Flags: review?(clokep)
Comment on attachment 8353072 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1319 at 2012-04-07 14:06:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353072 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1320 at 2012-04-07 16:12:00 UTC ***

- Clear whois table on disconnect
- Don't remove whois info just because a participant is leaving a channel
Attachment #8353073 - Flags: review?(clokep)
Comment on attachment 8353072 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1319 at 2012-04-07 16:12:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353072 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1321 as attmnt 1321 at 2012-04-08 13:23:00 UTC ***

- Remove some duplication
Attachment #8353074 - Flags: review?(clokep)
Comment on attachment 8353073 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1320 at 2012-04-08 13:23:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353073 - Attachment is obsolete: true
Attachment #8353073 - Flags: review?(clokep)
Comment on attachment 8353074 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1321 at 2012-04-10 13:52:50 UTC ***

Overall I'm pretty happy with this patch, but I have a couple of nits / questions. 

>diff --git a/components/irc.js b/components/irc.js
>@@ -298,20 +303,28 @@ ircParticipant.prototype = {
> function ircConversation(aAccount, aName) {
>   if (aAccount.hasBuddy(aName))
>     this.buddy = aAccount.getBuddy(aName);
>
>   this._init(aAccount, aName);
>   this._observedNicks = [];
>+
>+  // Fetch correctly capitalized name.
>+  // Always request the info as it may be out of date.
>+  if (hasOwnProperty(this._account.whoisInformation, this.normalizedName))
>+    this.updateNick(this._account.whoisInformation[this.normalizedName]["nick"]);
Instead of doing this here, why not use this as "name" above and init the conversation with the proper nick? (And while you're changing things in this file...you can get rid of the |if (aAccount.hasBuddy(aName))| check if you'd like.)

>+  this._waitingForName = true;
Please use waitingForNick or waitingForNickname to be consistent! :)

>+  this.requestBuddyInfo(aName);
> }

>@@ -576,16 +601,23 @@ ircAccount.prototype = {
>   // Remove a WHOIS entry.
>   removeBuddyInfo: function(aNick) {
I'm not crazy about our usage of "BuddyInfo" in some places and "Whois" in other places, but I'm not sure if it makes sense to care / fix it.

>     let nick = this.normalize(aNick);
>     if (hasOwnProperty(this.whoisInformation, nick))
>       delete this.whoisInformation[nick];
>   },
>+  // Set minimal WHOIS entry containing only the capitalized nick,
>+  // if no WHOIS info exists already.
>+  setWhoisNick: function(aNick) {
>+    let nick = this.normalize(aNick);
>+    if (!hasOwnProperty(this.whoisInformation, nick))
>+      this.whoisInformation[nick] = {"nick": aNick};
>+  },
Looks fine.

>@@ -950,16 +986,19 @@ ircAccount.prototype = {
>     // Mark all contacts on the account as having an unknown status.
>     for each (let buddy in this._buddies)
>       buddy.setStatus(Ci.imIStatusInfo.STATUS_UNKNOWN, "");
>
>+    // Clear whois table.
>+    this.whoisInformation = {};
>+
Good!

>     this.reportDisconnected();
>   },

>diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm
>@@ -306,16 +306,18 @@ var ircBase = {
>       // Loop over every conversation with the user and display that they quit.
>       for each (let conversation in this._conversations) {
>         if (conversation.isChat &&
>             conversation.hasParticipant(aMessage.nickname)) {
>           conversation.writeMessage(aMessage.servername, msg, {system: true});
>           conversation.removeParticipant(aMessage.nickname, true);
>         }
>       }
>+      // Remove from the whois table.
>+      this.removeBuddyInfo(aMessage.nickname);
Can we get a line break above and below this? Spread out this code a bit.

>       // If the leaver is a buddy, mark as offline.
>       let buddy = this.getBuddy(aMessage.nickname);
>       if (buddy)
>         buddy.setStatus(Ci.imIStatusInfo.STATUS_OFFLINE, "");
>       return true;
>     },

>diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm
>@@ -133,18 +133,17 @@ function ctcpCommand(aConv, aTarget, aCommand, aMsg) {
> function whoisCommand(aMsg, aConv) {
>   aMsg = aMsg.trim();
>   if (!aMsg || aMsg.indexOf(" ") != -1)
>     return false;
>-  getConv(aConv).waitForBuddyInfo(aMsg);
>-  getAccount(aConv).requestBuddyInfo(aMsg);
>+  getConv(aConv).requestBuddyInfo(aMsg);
>   return true;
> }
Good change! :)

Are we absolutely guaranteed to have the "nick" property of whois info set if we have any whois info? We don't check if "nick" exists before accessing it (only that the whois object has a property for the nick).

I think that's all my comments. Thanks for doing this. :)
Attachment #8353074 - Flags: review?(clokep) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1321 as attmnt 1324 at 2012-04-10 17:28:00 UTC ***

(In reply to comment #36)
> >   removeBuddyInfo: function(aNick) {
> I'm not crazy about our usage of "BuddyInfo" in some places and "Whois" in
> other places, but I'm not sure if it makes sense to care / fix it.

The rough principle I followed was that "BuddyInfo" methods are either in the interface or OK to be called from the outside (they are non-IRC specific), while "whois" ones generally are for internal use.

> Are we absolutely guaranteed to have the "nick" property of whois info set if
> we have any whois info? We don't check if "nick" exists before accessing it
> (only that the whois object has a property for the nick).

Yes. Whois entries are set via setWhois (ircBase.jsm) which always sets the nick field.
Attachment #8353077 - Flags: review?
Comment on attachment 8353074 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1321 at 2012-04-10 17:28:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353074 - Attachment is obsolete: true
Comment on attachment 8353077 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1324 at 2012-04-10 18:03:50 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>@@ -298,20 +303,28 @@ ircParticipant.prototype = {
> function ircConversation(aAccount, aName) {
>-  if (aAccount.hasBuddy(aName))
>-    this.buddy = aAccount.getBuddy(aName);
>+  this.buddy = aAccount.getBuddy(aName);
>+  let nick = this._account.normalize(aName);
>+  if (hasOwnProperty(this._account.whoisInformation, nick))
>+    aName = this._account.whoisInformation[nick]["nick"];
Normally we don't touch the inputs to functions. I don't really care too much here though -- do you have an opinion flo? (And please look over the rest of it!)
Attachment #8353077 - Flags: review? → review+
Attachment #8353077 - Flags: review?(florian)
Comment on attachment 8353077 [details] [diff] [review]
Patch

*** Original change on bio 1321 attmnt 1324 at 2012-04-10 19:24:48 UTC ***

Seems ok. The only nit I've found is on this line:
+      this.whoisInformation[nick] = {"nick": aNick};

Should be {nick: aNick} without the quotes.

(In reply to comment #38)
> Comment on attachment 8353077 [details] [diff] [review] (bio-attmnt 1324) [details]
> Patch
> 
> >diff --git a/components/irc.js b/components/irc.js
> >@@ -298,20 +303,28 @@ ircParticipant.prototype = {
> > function ircConversation(aAccount, aName) {
> >-  if (aAccount.hasBuddy(aName))
> >-    this.buddy = aAccount.getBuddy(aName);
> >+  this.buddy = aAccount.getBuddy(aName);
> >+  let nick = this._account.normalize(aName);
> >+  if (hasOwnProperty(this._account.whoisInformation, nick))
> >+    aName = this._account.whoisInformation[nick]["nick"];
> Normally we don't touch the inputs to functions. I don't really care too much
> here though -- do you have an opinion flo? (And please look over the rest of
> it!)

Seems ok in this case :).
Attachment #8353077 - Flags: review?(florian) → review+
*** Original post on bio 1321 at 2012-04-13 00:09:35 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/81717e6f64bd

Thanks for fixing this!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-blocking][checkin-needed] → [1.2-blocking]
Target Milestone: --- → 1.2
*** Original post on bio 1321 at 2012-04-13 09:41:57 UTC ***

Pushed a follow-up to fix some bustage:
https://hg.instantbird.org/instantbird/rev/ff06c4e90002
You need to log in before you can comment on or make changes to this bug.