Closed Bug 954764 Opened 10 years ago Closed 10 years ago

Implement /whois and /whowas commands

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(2 files, 11 obsolete files)

*** Original post on bio 1332 at 2012-03-10 01:58:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Blocks: 954299, 954753
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1241 at 2012-03-10 01:58:00 UTC ***

- Prints whois and whowas information as system messages. 
- Implements corresponding error handlers 401 and 406.
- Stores properly capitalized nickname in the account's whois table.
Attachment #8352994 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1242 at 2012-03-10 09:43:00 UTC ***

- Add an error message printed to the conversation when the nick does not exist.
Attachment #8352995 - Flags: review?(clokep)
Comment on attachment 8352994 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1241 at 2012-03-10 09:43:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352994 - Attachment is obsolete: true
Attachment #8352994 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1244 at 2012-03-10 16:05:00 UTC ***

- Put all /whois output in a single message: IRC may not handle line breaks, but system messages can ;)
Attachment #8352997 - Flags: review?(clokep)
Comment on attachment 8352995 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1242 at 2012-03-10 16:05:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352995 - Attachment is obsolete: true
Attachment #8352995 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1249 at 2012-03-13 10:36:00 UTC ***

Unbitrotted.
Attachment #8353002 - Flags: review?(clokep)
Comment on attachment 8352997 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1244 at 2012-03-13 10:36:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352997 - Attachment is obsolete: true
Attachment #8352997 - Flags: review?(clokep)
Attached patch Alternative patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1252 at 2012-03-13 20:42:00 UTC ***

Alternative patch: uses requestBuddyInfo for whois in the command handler, adds a corresponding method for whowas (requestOfflineBuddyInfo).
Attachment #8353005 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353002 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1249 at 2012-03-15 12:06:30 UTC ***

I definitely like the alternative version better than this.
Attachment #8353002 - Flags: review?(clokep) → review-
*** Original post on bio 1332 at 2012-03-15 13:55:06 UTC ***

(In reply to comment #4)
> Created attachment 8353005 [details] [diff] [review] (bio-attmnt 1252) [details]
> Alternative patch
> 
> Alternative patch: uses requestBuddyInfo for whois in the command handler, adds
> a corresponding method for whowas (requestOfflineBuddyInfo).

I'm fairly certain this patch will be r+, but I need to test it first. :)

My only other question is about whether we need to worry about adding the observers and never removing them (if we close the conversation before a response is received, if we never receive a response for some reason or if we receive the error responses). I'd like flo to consider this though, as I'm unsure when this causes a leak.
*** Original post on bio 1332 at 2012-03-15 14:18:33 UTC ***

(In reply to comment #6)
> My only other question is about whether we need to worry about adding the
> observers and never removing them (if we close the conversation before a
> response is received, if we never receive a response for some reason or if we
> receive the error responses). I'd like flo to consider this though, as I'm
> unsure when this causes a leak.

The error responses are not a problem, as according to RFC you always get an ENDOFWHOIS as well. For the other cases, all I can think of is adding a removeObserver call to the conversation destructor.
*** Original post on bio 1332 at 2012-03-15 14:41:43 UTC ***

Comment on attachment 8353005 [details] [diff] [review] (bio-attmnt 1252)
Alternative patch

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

>+  },
>+
>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic != "user-info-received")
>+      return;
>+    Services.obs.removeObserver(this, "user-info-received");
>+    this._account.writeWhois(this, aData,
>+                             aSubject.QueryInterface(Ci.nsISimpleEnumerator));
>   }

Shouldn't this be checking that the received info is for the nick that we have just requested?

>-    for (let field in whoisInformation) {
>-      let value = whoisInformation[field];
>-      tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>-    }
>+    for (let field in whoisInformation)
>+      if (field != "nick") {
>+        let value = whoisInformation[field];
>+        tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>+      }

Nit: The for needs {} here, as the block contains several lines.

>diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm

>+    name: "whois",
>+    get helpString() _("command.whois", "whois"),
>+    run: function(aMsg, aConv) {
>+      if (!aMsg.trim())
>+        return false;
>+      let account = getAccount(aConv);
>+      Services.obs.addObserver(account.getConversation(aConv.name),
>+                               "user-info-received", false);
>+      account.requestBuddyInfo(aMsg);
>+      return true;
>+    }
>+  },
>+  {
>     name: "whowas",
>     get helpString() _("command.whowas", "whowas"),
>-    run: function(aMsg, aConv) simpleCommand(aConv, "WHOWAS", aMsg)
>+    run: function(aMsg, aConv) {
>+      if (!aMsg.trim())
>+        return false;
>+      let account = getAccount(aConv);
>+      Services.obs.addObserver(account.getConversation(aConv.name),
>+                               "user-info-received", false);
>+      account.requestOfflineBuddyInfo(aMsg);
>+      return true;
>+    }
>   }

This feels a lot like duplicated code.

I'm not sure of what account.getConversation(aConv.name) does, but I looks like it could be a more complicated equivalent to account.wrappedJSObject.

Adding an observer on the conversation object from the command code is a bit strange. Wouldn't be be cleaner to add the observer by calling a waitForUserInfo(aUsername) method of the conversation object? That method would add the observer, and store the expected username of the notification. If that username is not null at the time unInit is called, the conversation will know it needs to remove an observer (otherwise, yes, there would be a possible leak).
Comment on attachment 8353005 [details] [diff] [review]
Alternative patch

*** Original change on bio 1332 attmnt 1252 at 2012-03-15 21:04:51 UTC ***

I like these suggestions, thanks for the review flo!
Attachment #8353005 - Flags: review?(clokep)
Attached patch getConv patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1256 at 2012-03-15 22:08:00 UTC ***

(In reply to comment #8)
> Comment on attachment 8353005 [details] [diff] [review] (bio-attmnt 1252) [details]
> I'm not sure of what account.getConversation(aConv.name) does, but I looks like
> it could be a more complicated equivalent to account.wrappedJSObject.

account.getConversation(aConv.name) gets you back to the JS account object, which is the same as aConv.wrappedJSObject, I have a local patch for this I think we should take that I haven't gotten around to convince you about. This adds a getConv method which grabs the wrappedJSObject of the conversation. Attaching that here if we want it.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1257 at 2012-03-16 00:58:00 UTC ***

- Addresses flo's review comments.
- Includes clokep's getConv patch.
- Remove old whois entry before adding new one (e.g. in case user has gone offline and so is no longer in the listed channels).
- Run WHOWAS if WHOIS fails.
- Set "offline" flag in whois table if entry comes from WHOWAS.
Attachment #8353010 - Flags: review?(clokep)
Comment on attachment 8353002 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1249 at 2012-03-16 00:58:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353002 - Attachment is obsolete: true
Comment on attachment 8353005 [details] [diff] [review]
Alternative patch

*** Original change on bio 1332 attmnt 1252 at 2012-03-16 00:58:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353005 - Attachment is obsolete: true
Comment on attachment 8353009 [details] [diff] [review]
getConv patch

*** Original change on bio 1332 attmnt 1256 at 2012-03-16 00:58:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353009 - Attachment is obsolete: true
*** Original post on bio 1332 at 2012-03-16 01:02:19 UTC ***

Note this will also fix bug 954016 (bio 579). (We don't need to handle the offline flag for buddy tooltips as in that case the status is taken care of separately.)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1258 at 2012-03-16 01:22:00 UTC ***

- Remove some unnecessary duplication by running the whois handler for both /whois and /whowas. Benefit: Informs the user the nick is actually online if he runs /whowas on an online nick.
Attachment #8353011 - Flags: review?(clokep)
Comment on attachment 8353010 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1257 at 2012-03-16 01:22:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353010 - Attachment is obsolete: true
Attachment #8353010 - Flags: review?(clokep)
Comment on attachment 8353011 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1258 at 2012-03-20 14:20:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353011 - Flags: review?(clokep) → review-
*** Original post on bio 1332 at 2012-03-20 14:20:42 UTC ***

Comment on attachment 8353011 [details] [diff] [review] (bio-attmnt 1258)
Patch

>diff --git a/chrome/en-US/locale/en-US/chat/irc.properties b/chrome/en-US/locale/en-US/chat/irc.properties
>@@ -105,27 +106,35 @@ message.topicCleared=%1$S has cleared the topic.
>+#    %S is the nickname of the user whose WHOIS information follows this message.
>+message.whois=WHOIS information for %S:
>+#    %1$S is the nickname of the (offline) user whose WHOWAS information follows this message.
>+message.whowas=%1$S is offline.\nWHOWAS information for %1$S:
I'm not sure if putting line breaks in localized strings is normal or not. We'll need input from flo.

>+#    %S is the nickname that is not known to the server.
>+message.unknownnick=%S is an unknown nickname.
[...]
>+error.nosuchnick=There is no nickname or channel: %S
>+error.wasnosuchnick=There was no nickname: %S
I'm wondering whether we need to show these all as separate messages. Maybe a single "%S is an unknown nickname." is sufficient?

>diff --git a/components/irc.js b/components/irc.js
>@@ -203,16 +205,33 @@ ircChannel.prototype = {
>+  waitForBuddyInfo: function (aNick) {
>+    Services.obs.addObserver(this, "user-info-received", false);
>+    this._observedNick = this._account.normalize(aNick);
>+  },
What happens if we're already waiting for a nick's WHOIS information?

>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic != "user-info-received")
>+      return;
>+
>+    if (!this._observedNick ||
>+        this._observedNick != this._account.normalize(aData))
>+      return;
>+    Services.obs.removeObserver(this, "user-info-received");
>+    this._account.writeWhois(this, aData,
>+                             aSubject.QueryInterface(Ci.nsISimpleEnumerator));
>+  }
Shouldn't we now say that we're no longer waiting for this nick's WHOIS information? (Preferably by adding _observerNick to the prototype and then deleting it, otherwise the unInit method will try to re-remove the observer.)

>@@ -275,21 +294,40 @@ ircConversation.prototype = {
>                                              aProperties);
>   },
[...]
> };
I greatly dislike the amount of code re-use between ircChannel and ircConversation. Outside the scope of this bug, but I wonder if JavaScript supports multiple inheritance (or if we can dynamically set a prototype's __proto__ or something).

>   requestBuddyInfo: function(aBuddyName) {
>+    this.removeBuddyInfo(aBuddyName);
Glad you added this! It's definitely necessary to avoid out of date information.

>   // Return an nsISimpleEnumerator of imITooltipInfo for a given nick.
>   getBuddyInfo: function(aNick) {
[...]
>+      if (field != "nick" && field != "offline") {
>+        let value = whoisInformation[field];
>+        tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>+      }
I dislike this hard coded ignoring the name & offline status, but I think it's a reasonable way to store the information...why are we ignoring the offline

>+  // Remove a WHOIS entry.
>+  removeBuddyInfo: function(aNick) {
>+    let nick = this.normalize(aNick);
>+    if (!hasOwnProperty(this.whoisInformation, nick))
>+      return;
>+    delete this.whoisInformation[nick];
>+  },
(We discussed this on IRC) style nit: just delete the variable, don't return early as it's only one line of code.

>+  // Write WHOIS information to a conversation.
>+  writeWhois: function(aConv, aNick, aTooltipInfo) {
>+    if (!hasOwnProperty(this.whoisInformation, aNick)) {
>+      aConv.writeMessage(null, _("message.unknownnick", aNick), {system: true});
>+      return;
>+    }
Please add a comment above this if-block. Also, should we be using the normalized nick in here?

>+    let msgType = "message.whois";
>+    if ("offline" in this.whoisInformation[aNick])
>+      msgType = "message.whowas";
And one above this.

>+    let msg = _(msgType, this.whoisInformation[aNick]["nick"]) + "\n";
>+    while (aTooltipInfo.hasMoreElements()) {
>+      let elt = aTooltipInfo.getNext().QueryInterface(Ci.prplITooltipInfo);
>+      switch (elt.type) {
>+        case Ci.prplITooltipInfo.pair:
>+        case Ci.prplITooltipInfo.sectionHeader:
>+          msg += "    " + elt.label + ": " + elt.value + "\n";
>+          break;
Should we make section headers bold?
>+        case Ci.prplITooltipInfo.sectionBreak:
>+          break;
Do we want to add an extra line break here or just not care?

(I trust your judgement with these past two comments! Just bringing up the questions. Feel free to just say "No." :))

>+      }
>+    }
>+    aConv.writeMessage(null, msg, {system: true});
>+  },
I'd like this method to be part of ircChannel and ircConversation...assuming we could somehow make the prototypes work out. Otherwise I think it's fine to stay here.

>diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm
>@@ -132,16 +132,19 @@ function errorMessage(aAccount, aMessage, aError)
> function setWhoIs(aAccount, aMessage, aFields) {
[...]
>+  // Set non-normalized nickname field.
>+  aAccount.whoisInformation[buddyName]["nick"] = aMessage.params[1];
This is what blocks bug 954753 (bio 1321) (for reference).

>@@ -626,34 +629,35 @@ var ircBase = {
>     /*
>      * WHOIS
>      */
>     "311": function(aMessage) { // RPL_WHOISUSER
>       // <nick> <user> <host> * :<real name>
>       // <username>@<hostname>
>       let source = aMessage.params[2] + "@" + aMessage.params[3];
>-      return setWhoIs(this, aMessage, {connectedFrom: source,
>-                                       realname: aMessage.params[5]});
>+      return setWhoIs(this, aMessage, {realname: aMessage.params[5],
>+                                       connectedFrom: source});
This is from bug 954725 (bio 1293), please remove it (or hopefully that will be checked in!).

>@@ -665,21 +669,25 @@ var ircBase = {
>     "318": function(aMessage) { // RPL_ENDOFWHOIS
[...]
>-      let buddyName = this.normalize(aMessage.params[1]);
>+      let nick = this.normalize(aMessage.params[1]);
Good, fix my terminology. :)


>-      // Notify the tooltip.
>-      Services.obs.notifyObservers(this.getBuddyInfo(buddyName),
>-                                   "user-info-received", buddyName);
>+      // If there is no whois information stored now, the nick is offline.
OK, they're offline? So what...add something like "request their WHOWAS information.
>+      if (!hasOwnProperty(this.whoisInformation, nick))
>+        this.requestOfflineBuddyInfo(nick);
>+      else {
>+        Services.obs.notifyObservers(this.getBuddyInfo(nick),
>+                                     "user-info-received", nick);
>+      }

>@@ -849,18 +857,22 @@ var ircBase = {
>     },
>     "368": function(aMessage) { // RPL_ENDOFBANLIST
>       // <channel> :End of channel ban list
>       // TODO
>       return false;
>     },
>     "369": function(aMessage) { // RPL_ENDOFWHOWAS
>       // <nick> :End of WHOWAS
>-      // TODO
>-      return false;
>+      // We've received everything about WHOWAS, tell the tooltip that is waiting
>+      // for this information.
>+      let nick = this.normalize(aMessage.params[1]);
>+      Services.obs.notifyObservers(this.getBuddyInfo(nick),
>+                                   "user-info-received", nick);
>+      return true;
Looks good.

>@@ -942,18 +954,19 @@ var ircBase = {
>       // Error messages, Implement Section 5.2 of RFC 2812
>     "401": function(aMessage) { // ERR_NOSUCHNICK
>       // <nickname> :No such nick/channel
>-      // TODO Parse & display an error to the user.
>-      return false;
>+      // TODO Still needs to be handled for /mode, /invite, /kill, /msg.
>+      return errorMessage(this, aMessage,
>+                          _("error.nosuchnick", aMessage.params[1]));
I'm not sure I love the comment change here...we might want to add a comment saying what ever reply that can generate this is.

>@@ -967,18 +980,18 @@ var ircBase = {
>     "406": function(aMessage) { // ERR_WASNOSUCHNICK
>       // <nickname> :There was no such nickname
>-      // TODO Error saying the nick never existed.
>-      return false;
>+      return errorMessage(this, aMessage,
>+                          _("error.wasnosuchnick", aMessage.params[1]));
Same as above.

>diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm
This file looks good.

Sorry this took so long to review! Thanks for looking at this.
*** Original post on bio 1332 at 2012-03-20 16:14:48 UTC ***

(In reply to comment #14)
> I'm not sure if putting line breaks in localized strings is normal or not.
> We'll need input from flo.

It looks better without the line break actually.
 
> >+#    %S is the nickname that is not known to the server.
> >+message.unknownnick=%S is an unknown nickname.
> [...]
> >+error.nosuchnick=There is no nickname or channel: %S
> >+error.wasnosuchnick=There was no nickname: %S
> I'm wondering whether we need to show these all as separate messages. Maybe a
> single "%S is an unknown nickname." is sufficient?

The error.* messages are simply the RFC error descriptions and get sent to the server tab. The message.unknownnick gets sent to the conversation. That's why I kept them separate. The 406 one is strictly speaking unnecessary (as it is only triggered when message.unknownnick is also printed) but it seemed consistent with some other errors to log to the server tab. The 401 one should definitely be kept at least until all the different (non-whois) ways it can arise are handled elsewhere. Note it can also arise for an unknown channel.

> >diff --git a/components/irc.js b/components/irc.js
> >@@ -203,16 +205,33 @@ ircChannel.prototype = {
> >+  waitForBuddyInfo: function (aNick) {
> >+    Services.obs.addObserver(this, "user-info-received", false);
> >+    this._observedNick = this._account.normalize(aNick);
> >+  },
> What happens if we're already waiting for a nick's WHOIS information?

It's rather unlikely (you'd have to type very fast) but I can turn this._observedNick into an array to be safe.

> >+  observe: function(aSubject, aTopic, aData) {
> >+    if (aTopic != "user-info-received")
> >+      return;
> >+
> >+    if (!this._observedNick ||
> >+        this._observedNick != this._account.normalize(aData))
> >+      return;
> >+    Services.obs.removeObserver(this, "user-info-received");
> >+    this._account.writeWhois(this, aData,
> >+                             aSubject.QueryInterface(Ci.nsISimpleEnumerator));
> >+  }
> Shouldn't we now say that we're no longer waiting for this nick's WHOIS
> information? (Preferably by adding _observerNick to the prototype and then
> deleting it, otherwise the unInit method will try to re-remove the observer.)

Good catch! I forgot the delete here, it was intended to be there.

> I greatly dislike the amount of code re-use between ircChannel and
> ircConversation. Outside the scope of this bug, but I wonder if JavaScript
> supports multiple inheritance (or if we can dynamically set a prototype's
> __proto__ or something).

Yes, it would be great if this were possible. Like you say, it's a separate issue as it applies to other methods too.

> >+      if (field != "nick" && field != "offline") {
> >+        let value = whoisInformation[field];
> >+        tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
> >+      }
> I dislike this hard coded ignoring the name & offline status, but I think it's
> a reasonable way to store the information...why are we ignoring the offline

We are ignoring the offline because you don't want it in the tooltip: Participants that are offline don't have tooltips, and buddies that are offline have their status displayed separately from the textual info, so you don't need it. (It's possible this might be done differently in the course of a later overall tooltip redesign, but that's a separate project.)

> >+  writeWhois...
> I'd like this method to be part of ircChannel and ircConversation...assuming we
> could somehow make the prototypes work out. Otherwise I think it's fine to stay
> here.

Yes, it's there to avoid duplication as far as possible.

> This is from bug 954725 (bio 1293), please remove it (or hopefully that will be checked
> in!).

+1

> >-      let buddyName = this.normalize(aMessage.params[1]);
> >+      let nick = this.normalize(aMessage.params[1]);
> Good, fix my terminology. :)

They're just not always buddies in this method ;)

> >+      // If there is no whois information stored now, the nick is offline.
> OK, they're offline? So what...add something like "request their WHOWAS
> information.
> >+      if (!hasOwnProperty(this.whoisInformation, nick))
> >+        this.requestOfflineBuddyInfo(nick);

I thought that was implicit in the name "requestOfflineBuddyInfo", but I'll add a comment.

> >@@ -942,18 +954,19 @@ var ircBase = {
> >       // Error messages, Implement Section 5.2 of RFC 2812
> >     "401": function(aMessage) { // ERR_NOSUCHNICK
> >       // <nickname> :No such nick/channel
> >-      // TODO Parse & display an error to the user.
> >-      return false;
> >+      // TODO Still needs to be handled for /mode, /invite, /kill, /msg.
> >+      return errorMessage(this, aMessage,
> >+                          _("error.nosuchnick", aMessage.params[1]));
> I'm not sure I love the comment change here...we might want to add a comment
> saying what ever reply that can generate this is.

I'm not sure I understand what you are asking for here. I just thought I'd add a comment listing the known commands for which this error is not handled yet. Alternatively I could add a comment that it *is* handled for whois. 

> >@@ -967,18 +980,18 @@ var ircBase = {
> >     "406": function(aMessage) { // ERR_WASNOSUCHNICK
> >       // <nickname> :There was no such nickname
> >-      // TODO Error saying the nick never existed.
> >-      return false;
> >+      return errorMessage(this, aMessage,
> >+                          _("error.wasnosuchnick", aMessage.params[1]));
> Same as above.

?

I'll redo the patch when you've replied to this.
*** Original post on bio 1332 at 2012-03-20 16:32:57 UTC ***

(In reply to comment #15)
> (In reply to comment #14)
> > >+#    %S is the nickname that is not known to the server.
> > >+message.unknownnick=%S is an unknown nickname.
> > [...]
> > >+error.nosuchnick=There is no nickname or channel: %S
> > >+error.wasnosuchnick=There was no nickname: %S
> > I'm wondering whether we need to show these all as separate messages. Maybe a
> > single "%S is an unknown nickname." is sufficient?
> 
> The error.* messages are simply the RFC error descriptions and get sent to the
> server tab. The message.unknownnick gets sent to the conversation. That's why I
> kept them separate. The 406 one is strictly speaking unnecessary (as it is only
> triggered when message.unknownnick is also printed) but it seemed consistent
> with some other errors to log to the server tab. The 401 one should definitely
> be kept at least until all the different (non-whois) ways it can arise are
> handled elsewhere. Note it can also arise for an unknown channel.
OK, this seems to make sense then. I'm not sure if we should add any comments explaining that those two errors appear in the server tab?

> > >diff --git a/components/irc.js b/components/irc.js
> > >@@ -203,16 +205,33 @@ ircChannel.prototype = {
> > >+  waitForBuddyInfo: function (aNick) {
> > >+    Services.obs.addObserver(this, "user-info-received", false);
> > >+    this._observedNick = this._account.normalize(aNick);
> > >+  },
> > What happens if we're already waiting for a nick's WHOIS information?
> 
> It's rather unlikely (you'd have to type very fast) but I can turn
> this._observedNick into an array to be safe.
Or have a really slow connection. :) Seems plausible to type /whois Even <Ctrl+z> 1, very quickly to check whois for both Even and Even1.

> > Shouldn't we now say that we're no longer waiting for this nick's WHOIS
> > information? (Preferably by adding _observerNick to the prototype and then
> > deleting it, otherwise the unInit method will try to re-remove the observer.)
> Good catch! I forgot the delete here, it was intended to be there.
You'll need to splice it from the array now, not just delete it.

> > >+      // If there is no whois information stored now, the nick is offline.
> > OK, they're offline? So what...add something like "request their WHOWAS
> > information.
> > >+      if (!hasOwnProperty(this.whoisInformation, nick))
> > >+        this.requestOfflineBuddyInfo(nick);
> 
> I thought that was implicit in the name "requestOfflineBuddyInfo", but I'll add
> a comment.
I'm fine with not adding it if you protest, just seemed like the comment needed a little bit more.

> > >@@ -942,18 +954,19 @@ var ircBase = {
> > >       // Error messages, Implement Section 5.2 of RFC 2812
> > >     "401": function(aMessage) { // ERR_NOSUCHNICK
> > >       // <nickname> :No such nick/channel
> > >-      // TODO Parse & display an error to the user.
> > >-      return false;
> > >+      // TODO Still needs to be handled for /mode, /invite, /kill, /msg.
> > >+      return errorMessage(this, aMessage,
> > >+                          _("error.nosuchnick", aMessage.params[1]));
> > I'm not sure I love the comment change here...we might want to add a comment
> > saying what ever reply that can generate this is.
> 
> I'm not sure I understand what you are asking for here. I just thought I'd add
> a comment listing the known commands for which this error is not handled yet.
> Alternatively I could add a comment that it *is* handled for whois. 
I meant it would be helpful to list both the ones it's handled for and not handled for! (E.g. to list ALL situations it can happen, which ones we know it works well for, and which we don't.)

> > >@@ -967,18 +980,18 @@ var ircBase = {
> > >     "406": function(aMessage) { // ERR_WASNOSUCHNICK
> > >       // <nickname> :There was no such nickname
> > >-      // TODO Error saying the nick never existed.
> > >-      return false;
> > >+      return errorMessage(this, aMessage,
> > >+                          _("error.wasnosuchnick", aMessage.params[1]));
> > Same as above.
> 
> ?
Please apply a similar comment as for the 401 response here.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1265 at 2012-03-20 20:59:00 UTC ***

Addresses review comments.

(In reply to comment #14)
> >+        case Ci.prplITooltipInfo.pair:
> >+        case Ci.prplITooltipInfo.sectionHeader:
> >+          msg += "    " + elt.label + ": " + elt.value + "\n";
> >+          break;
> Should we make section headers bold?
> >+        case Ci.prplITooltipInfo.sectionBreak:
> >+          break;
> Do we want to add an extra line break here or just not care?

Currently neither of these arise in the tooltips whois generates. I kept the cases from the buddytooltip.xml tooltip display code in there in case we decide to make use of them in the future, so nothing breaks.

(In reply to comment #16)
> OK, this seems to make sense then. I'm not sure if we should add any comments
> explaining that those two errors appear in the server tab?

All error.* messages (currently) get written to the server tab. I've amended the comment in irc.properties to reflect this.
Attachment #8353018 - Flags: review?(clokep)
Comment on attachment 8353011 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1258 at 2012-03-20 20:59:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353011 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1267 at 2012-03-20 23:28:00 UTC ***

- RFC -> RFC 2812
- only add/remove observers when necessary
- create new instance of _observedNicks on construction
- Save some \n.

Thanks for the review comments.
Attachment #8353020 - Flags: review?(clokep)
Comment on attachment 8353018 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1265 at 2012-03-20 23:28:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353018 - Attachment is obsolete: true
Attachment #8353018 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1332 as attmnt 1268 at 2012-03-20 23:46:00 UTC ***

- Don't fail when there are trailing spaces after in /whois nick
- Don't run whois if it received multiple parameters
Attachment #8353021 - Flags: review?(clokep)
Comment on attachment 8353020 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1267 at 2012-03-20 23:46:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353020 - Attachment is obsolete: true
Attachment #8353020 - Flags: review?(clokep)
Comment on attachment 8353021 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1268 at 2012-03-29 00:35:58 UTC ***

This looks fine and seems to work well. :)
Attachment #8353021 - Flags: review?(clokep) → review+
Attached patch Fixed pathsSplinter Review
*** Original post on bio 1332 as attmnt 1278 at 2012-03-29 00:36:00 UTC ***

I fixed a bunch of the paths, etc. Aleth can you look over this and make sure everything seems fine?
Attachment #8353031 - Flags: review?(bugzilla)
Comment on attachment 8353031 [details] [diff] [review]
Fixed paths

*** Original change on bio 1332 attmnt 1278 at 2012-03-29 00:54:40 UTC ***

Some unrelated changes slipped in here...
Attachment #8353031 - Flags: review?(bugzilla) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1332 as attmnt 1285 at 2012-03-29 22:58:00 UTC ***

Fix flo's review comments, in particular: localize the whois entry format.
Attachment #8353038 - Flags: review?(florian)
Comment on attachment 8353021 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1268 at 2012-03-29 22:58:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353021 - Attachment is obsolete: true
*** Original post on bio 1332 at 2012-03-29 23:53:07 UTC ***

(In reply to comment #23)
> Created attachment 8353038 [details] [diff] [review] (bio-attmnt 1285) [details]
> Patch
> 
> Fix flo's review comments, in particular: localize the whois entry format.

This is an r+ for me.
Comment on attachment 8353038 [details] [diff] [review]
Patch

*** Original change on bio 1332 attmnt 1285 at 2012-03-30 08:50:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353038 - Flags: review?(florian) → review+
*** Original post on bio 1332 at 2012-04-04 10:34:31 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/07ece750914b

Thanks for fixing this. :)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.