Closed Bug 954803 Opened 10 years ago Closed 10 years ago

Support WATCH and MONITOR for IRC

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

*** Original post on bio 1369 at 2012-04-10 18:15:00 UTC ***

We should support the WATCH and MONITOR exchanges for IRC. They allow us to ask the server to tell us when a list of users sign on or off, etc. And to be notified when they go away.

WATCH is supported by UnrealIRCd and Bahamut (used by DAL.net), thus being a huge bulk of IRC servers.

MONITOR is support by Charybdis (thus Freenode, since they used ircd-seven, a fork of Charybdis).

I've started work on this already.

[1] http://www.stack.nl/~jilles/cgi-bin/hgwebdir.cgi/irc-documentation-jilles/file/tip/reference/draft-meglio-irc-watch-00.txt
[2] http://hg.atheme.org/charybdis/file/tip/doc/monitor.txt
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1467 at 2012-05-14 00:16:00 UTC ***

The WATCH command seems to work, but I haven't done MONITOR yet. Before I work on MONITOR I'd like some feedback on if this approach seems reasonable aleth.

Probably about 1/3rd of this patch is clean up...getting ISUPPORT to actually work. :( Apparently it was only partially working, although the code wasn't even being called. :-/
Attachment #8353220 - Flags: feedback?(bugzilla)
*** Original post on bio 1369 at 2012-05-14 00:16:56 UTC ***

I should note that WATCH is used on moznet: should be easy to test.
Comment on attachment 8353220 [details] [diff] [review]
Patch v1

*** Original change on bio 1369 attmnt 1467 at 2012-05-14 11:45:01 UTC ***

>+  trackBuddy: function(aNick) {
>+    // Put the username as the first to be checked on the next ISON call.
>+    this._isOnQueue.unshift(aNick);
>+  },

As you mentioned, there is currently no untrackBuddy. Probably not a big issue at the moment, may become more so when we also temporarily track people we have DMs with (to watch their status).

I wonder if _isOnQueue should be renamed _trackQueue or _trackedNicks (as there are multiple implementations now, ISON, WATCH, MONITOR...).

Also, should ISON be moved to a module too? (I don't know, since it is the fallback after all.)

>+  commands: {
>+    "WATCH": function(aMessage) {
>+      if (!aMessage.isupport.useDefault)
>+        this.nickListSize = 128;
>+      else {
>+        let size = parseInt(aMessage.isupport.value, 10);
>+        if (isNaN(size))
>+          return false;
>+        this.nickListSize = size;
>+      }

It doesn't appear this.nickListSize is actually used anywhere yet?

>+      this.trackBuddy = function(aNick) {
>+        this.sendMessage("WATCH", "+" + aNick);
>+      };

Does this need an "A" parameter?

>+function setStatus(aAccount, aNick, aStatus) {
>+  if (!aAccount.watchEnabled)
>+    return false;
>+
>+  let buddy = aAccount.getBuddy(aNick);
>+  if (buddy)
>+    buddy.setStatus(Ci.imIStatusInfo["STATUS_" + aStatus], "");
>+  return true;
>+}

We should probably set "Away" and "offline" in the whois entry of a nick when the status changes too for consistency, though I don't think its absence will break anything. (I think we decided in bug 954796 (bio 1362) not to make use of RPL_AWAY in the ISON case anyway?)

>+    "512": function(aMessage) { // ERR_TOOMANYWATCH
>+      // Maximum size for WATCH-list is <watchlimit> entries
>+    },

Should probably throw an error?

This should be good :)
Attachment #8353220 - Flags: feedback?(bugzilla) → feedback+
*** Original post on bio 1369 at 2012-05-14 11:55:19 UTC ***

(In reply to comment #3)
> Comment on attachment 8353220 [details] [diff] [review] (bio-attmnt 1467) [details]
> >+  trackBuddy: function(aNick) {
> >+    // Put the username as the first to be checked on the next ISON call.
> >+    this._isOnQueue.unshift(aNick);
> >+  },
> 
> As you mentioned, there is currently no untrackBuddy. Probably not a big issue
> at the moment, may become more so when we also temporarily track people we have
> DMs with (to watch their status).
Yes, there actually doesn't even seem to be a way for buddies to be removed right now?

> I wonder if _isOnQueue should be renamed _trackQueue or _trackedNicks (as there
> are multiple implementations now, ISON, WATCH, MONITOR...).
Yes, probably.

> Also, should ISON be moved to a module too? (I don't know, since it is the
> fallback after all.)
No, it's part of RFC 2812. (WATCH and MONITOR are extensions, so they should be kept separate.)

> >+  commands: {
> >+    "WATCH": function(aMessage) {
> >+      if (!aMessage.isupport.useDefault)
> >+        this.nickListSize = 128;
> >+      else {
> >+        let size = parseInt(aMessage.isupport.value, 10);
> >+        if (isNaN(size))
> >+          return false;
> >+        this.nickListSize = size;
> >+      }
> 
> It doesn't appear this.nickListSize is actually used anywhere yet?
No, we currently don't use this, we should check the amount of nicks in the queue before trying to add more though.

> >+      this.trackBuddy = function(aNick) {
> >+        this.sendMessage("WATCH", "+" + aNick);
> >+      };
> 
> Does this need an "A" parameter?
Yes, good catch. :)

> >+function setStatus(aAccount, aNick, aStatus) {
> >+  if (!aAccount.watchEnabled)
> >+    return false;
> >+
> >+  let buddy = aAccount.getBuddy(aNick);
> >+  if (buddy)
> >+    buddy.setStatus(Ci.imIStatusInfo["STATUS_" + aStatus], "");
> >+  return true;
> >+}
> 
> We should probably set "Away" and "offline" in the whois entry of a nick when
> the status changes too for consistency, though I don't think its absence will
> break anything. (I think we decided in bug 954796 (bio 1362) not to make use of RPL_AWAY in
> the ISON case anyway?)
I know we set away, but do we also set offline in the WHOIS entry? Where is this done?

> >+    "512": function(aMessage) { // ERR_TOOMANYWATCH
> >+      // Maximum size for WATCH-list is <watchlimit> entries
> >+    },
> 
> Should probably throw an error?
Yes, probably.

Thanks for the insight. :)
*** Original post on bio 1369 at 2012-05-14 13:54:28 UTC ***

(In reply to comment #4)
> > We should probably set "Away" and "offline" in the whois entry of a nick when
> > the status changes too for consistency, though I don't think its absence will
> > break anything. (I think we decided in bug 954796 (bio 1362) not to make use of RPL_AWAY in
> > the ISON case anyway?)
> I know we set away, but do we also set offline in the WHOIS entry? Where is
> this done?

We set "offline" when a whowas entry exists:
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#707

Otherwise the whois entry should be deleted.

We might want to request the user's Away message when a buddy changes status to Away.

Btw "Away notification is only available if the WATCHOPTS RPL_ISUPPORT 494 token (See Section 5) contains the 'A' character" - is it a problem if the A flag is set when this is not the case?
*** Original post on bio 1369 at 2012-05-14 15:47:31 UTC ***

(In reply to comment #5)
> Btw "Away notification is only available if the WATCHOPTS RPL_ISUPPORT 494
> token (See Section 5) contains the 'A' character" - is it a problem if the A
> flag is set when this is not the case?

I'm not sure what the 494 token refers to, but...

Yes, Section 5.2 states:
> The WATCHOPTS token defines OPTIONAL features of the WATCH command.
> A client SHOULD NOT assume that any of the features specified with
> this token are present if the feature is not mentioned in the
> WATCHOPTS token, or the WATCHOPTS token is not present. The presents
> of the WATCHOPTS token in the RPL_ISUPPORT message [6] is OPTIONAL.
Attached patch Watch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1469 at 2012-05-15 00:18:00 UTC ***

This only supports WATCH currently, but I think it's ready for review, I'll do MONITOR as a second patch in this bug. I think this takes into account most of your review comments.
Attachment #8353222 - Flags: review?(bugzilla)
Comment on attachment 8353220 [details] [diff] [review]
Patch v1

*** Original change on bio 1369 attmnt 1467 at 2012-05-15 00:18:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353220 - Attachment is obsolete: true
Attached patch Monitor Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1471 at 2012-05-15 01:44:00 UTC ***

This implements Monitor and depends on the Watch patch.
Attachment #8353224 - Flags: review?(bugzilla)
Comment on attachment 8353222 [details] [diff] [review]
Watch Patch v2

*** Original change on bio 1369 attmnt 1469 at 2012-05-15 10:15:27 UTC ***

> Cu.import("resource:///modules/ircUtils.jsm");
> 
>+/*
>+ *
>+ */
> function isupportMessage(aMessage, aToken) {

I am intrigued by this comment. Is there something missing here, or do you use that style as a marker for something?

>+  let newWatchLength = this.watchLength + nicks.length;
>+  if (newWatchLength > this.maxWatchLength) {
>+    WARN("Attempting to WATCH " + newWatchLength + " nicks; maximum size is " +
>+         this.maxWatchLength + ".");
>+  }
>+  this.watchLength = newWatchLength;

Apart from throwing a warning, shouldn't we crop nicks to the maximum possible before continuing, so we don't just run into errors further down?

>+    "598": function(aMessage) { // RPL_GONEAWAY
>+      // <nickname> <username> <hostname> <awaysince> :is now away
>+      this.requestBuddyInfo(aMessage.params[1]);
>+      return setStatus(this, aMessage.params[1], "AWAY");
>+    },

Please move requestBuddyInfo to setStatus. It's duplicated further down, and that way you can avoid a clash with removeBuddyInfo in setStatus if you only call one or the other depending on the new status. You'll have to check that the previous status/away message wasn't the same or you may end up with an infinite loop due to the 301 override above (which would be worse than the deletion of the away message from whois you have at present).

flo might want you to only actually set the status (in setStatus) if it has changed, but I'm not sure that is necessary (or even the way it is done elsewhere in the code) ;)
Attachment #8353222 - Flags: review?(bugzilla) → review-
*** Original post on bio 1369 at 2012-05-15 10:19:45 UTC ***

PS I didn't include the nits in the Watch patch you already fixed in the Monitor patch in my review comments, but if you'd like the two patches to be checked in one after the other (but not necessarily on the same day) you might want to move them to the Watch patch.
Comment on attachment 8353224 [details] [diff] [review]
Monitor Patch v2

*** Original change on bio 1369 attmnt 1471 at 2012-05-15 10:26:22 UTC ***

This patch looks good to me, modulo similar comments as those for the WATCH patch, where applicable.

>// Watch away as well as online.

This comment does not seem to apply to MONITOR, sadly ;)

If both MONITOR and WATCH are supported by the server, we should pick WATCH I guess (as it supports Away). Do we do this?
Attachment #8353224 - Flags: review?(bugzilla) → review-
Attached patch Watch/Monitor Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1473 at 2012-05-15 22:44:00 UTC ***

(In reply to comment #9)
> Comment on attachment 8353222 [details] [diff] [review] (bio-attmnt 1469) [details]
> >+/*
> >+ *
> >+ */
> > function isupportMessage(aMessage, aToken) {
> 
> I am intrigued by this comment. Is there something missing here, or do you use
> that style as a marker for something?
Added a description of the ISUPPORT message here. :)

> >+  let newWatchLength = this.watchLength + nicks.length;
> >+  if (newWatchLength > this.maxWatchLength) {
> >+    WARN("Attempting to WATCH " + newWatchLength + " nicks; maximum size is " +
> >+         this.maxWatchLength + ".");
> >+  }
> >+  this.watchLength = newWatchLength;
> 
> Apart from throwing a warning, shouldn't we crop nicks to the maximum possible
> before continuing, so we don't just run into errors further down?
I added a verbose comment here saying that we should add the overflowed nicks to ISON, but for now it makes sense to just send them and hope the server doesn't ignore them.

> >+    "598": function(aMessage) { // RPL_GONEAWAY
> >+      // <nickname> <username> <hostname> <awaysince> :is now away
> >+      this.requestBuddyInfo(aMessage.params[1]);
> >+      return setStatus(this, aMessage.params[1], "AWAY");
> >+    },
> 
> Please move requestBuddyInfo to setStatus. It's duplicated further down, and
> that way you can avoid a clash with removeBuddyInfo in setStatus if you only
> call one or the other depending on the new status. You'll have to check that
> the previous status/away message wasn't the same or you may end up with an
> infinite loop due to the 301 override above (which would be worse than the
> deletion of the away message from whois you have at present).
I moved some of this around to avoid a loop as you suggested. :)

> flo might want you to only actually set the status (in setStatus) if it has
> changed, but I'm not sure that is necessary (or even the way it is done
> elsewhere in the code) ;)
jsProtoHelper already does this.
Attachment #8353226 - Flags: review?(bugzilla)
Comment on attachment 8353222 [details] [diff] [review]
Watch Patch v2

*** Original change on bio 1369 attmnt 1469 at 2012-05-15 22:44:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353222 - Attachment is obsolete: true
Comment on attachment 8353224 [details] [diff] [review]
Monitor Patch v2

*** Original change on bio 1369 attmnt 1471 at 2012-05-15 22:44:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353224 - Attachment is obsolete: true
Attached patch Differences from Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1474 at 2012-05-15 22:48:00 UTC ***

(In reply to comment #11)
> If both MONITOR and WATCH are supported by the server, we should pick WATCH I
> guess (as it supports Away). Do we do this?

We prefer WATCH if both are set, but will still use MONITOR if available. I hope no servers send both. :(

I should also note that this requires each handler to have another method, isEnabled. Inside of it 'this' is bound to the account and it is used to decide if a handler should be run at all for an account or not (i.e. the entire ircWATCH handler is now ignored if WATCH is not enabled). This was cleaned than adding a check in every single method. Let me know if this API seems silly.
Comment on attachment 8353226 [details] [diff] [review]
Watch/Monitor Patch v3

*** Original change on bio 1369 attmnt 1473 at 2012-05-16 00:04:56 UTC ***

>+  // Clear the WHOIS information.
>+  aAccount.removeBuddyInfo(aNick);
>+  if (aStatus == "AWAY") {
>+    // We need to request the away message.
>+    aAccount.requestBuddyInfo(aNick);
>+  }

This would be better like so:

>+  if (aStatus == "AWAY") {
>+    // We need to request the away message.
>+    aAccount.requestBuddyInfo(aNick);
>+  }
>+  else {
>+    // Clear the WHOIS information.
>+    aAccount.removeBuddyInfo(aNick);
>+  }


>I should also note that this requires each handler to have another method,
>isEnabled. Inside of it 'this' is bound to the account and it is used to decide
>if a handler should be run at all for an account or not (i.e. the entire
>ircWATCH handler is now ignored if WATCH is not enabled). This was cleaned than
>adding a check in every single method. Let me know if this API seems silly.

I can't think of a better way of doing this either atm.

This patch looks good and seems to work really well :) 
(For future reference, mozilla.org supports WATCH, freenode supports MONITOR)

r+ with that trivial change.
Attachment #8353226 - Flags: review?(bugzilla) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
*** Original post on bio 1369 as attmnt 1476 at 2012-05-16 01:04:00 UTC ***

With the trivial change, please take one last look over this! :)
Attachment #8353229 - Flags: review?(bugzilla)
Comment on attachment 8353226 [details] [diff] [review]
Watch/Monitor Patch v3

*** Original change on bio 1369 attmnt 1473 at 2012-05-16 01:04:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353226 - Attachment is obsolete: true
Comment on attachment 8353229 [details] [diff] [review]
Patch v4

*** Original change on bio 1369 attmnt 1476 at 2012-05-16 08:40:17 UTC ***

Looks good!
Attachment #8353229 - Flags: review?(bugzilla) → review+
Whiteboard: [checkin-needed]
Attached patch Patch v4.1Splinter Review
*** Original post on bio 1369 as attmnt 1477 at 2012-05-16 10:35:00 UTC ***

Coding style nit from flo on IRC [1]:
> I just have a coding style nit. var blahblah = { ... }*;* <-- ; at the end of the assignment

Carrying the r+ forward as the only changes are the semi-colons and a slight rewording of the initial comment.

[1] http://log.bezut.info/instantbird/120516/#m164
Attachment #8353230 - Flags: review+
Comment on attachment 8353227 [details] [diff] [review]
Differences from Patch v2

*** Original change on bio 1369 attmnt 1474 at 2012-05-16 10:35:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353227 - Attachment is obsolete: true
Comment on attachment 8353229 [details] [diff] [review]
Patch v4

*** Original change on bio 1369 attmnt 1476 at 2012-05-16 10:35:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353229 - Attachment is obsolete: true
*** Original post on bio 1369 at 2012-05-16 15:59:53 UTC ***

https://hg.instantbird.org/instantbird/rev/3abdc0efb06d

Thanks for this enhancement, it sounds really great!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
Depends on: 954876
Depends on: 954899
Blocks: ircv3
You need to log in before you can comment on or make changes to this bug.