bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Support WATCH and MONITOR for IRC

RESOLVED FIXED in 1.2

Status

Chat Core
IRC
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
*** 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
(Assignee)

Comment 1

5 years ago
Created attachment 8353220 [details] [diff] [review]
Patch v1

*** 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)
(Assignee)

Comment 2

5 years ago
*** 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 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
*** 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. :)

Comment 5

5 years ago
*** 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?
(Assignee)

Comment 6

5 years ago
*** 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.
(Assignee)

Comment 7

5 years ago
Created attachment 8353222 [details] [diff] [review]
Watch Patch v2

*** 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)
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 9

5 years ago
Created attachment 8353224 [details] [diff] [review]
Monitor Patch v2

*** 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 10

5 years ago
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-

Comment 11

5 years ago
*** 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 12

5 years ago
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-
(Assignee)

Comment 13

5 years ago
Created attachment 8353226 [details] [diff] [review]
Watch/Monitor Patch v3

*** 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)
(Assignee)

Comment 14

5 years ago
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
(Assignee)

Comment 15

5 years ago
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
(Assignee)

Comment 16

5 years ago
Created attachment 8353227 [details] [diff] [review]
Differences from Patch v2

*** 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 17

5 years ago
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-
(Assignee)

Comment 18

5 years ago
Created attachment 8353229 [details] [diff] [review]
Patch v4

*** 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)
(Assignee)

Comment 19

5 years ago
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 20

5 years ago
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+

Updated

5 years ago
Whiteboard: [checkin-needed]
(Assignee)

Comment 21

5 years ago
Created attachment 8353230 [details] [diff] [review]
Patch v4.1

*** 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+
(Assignee)

Comment 22

5 years ago
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
(Assignee)

Comment 23

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
(Assignee)

Updated

5 years ago
Depends on: 954876
(Assignee)

Updated

5 years ago
Depends on: 954899
You need to log in before you can comment on or make changes to this bug.