Closed Bug 954854 Opened 6 years ago Closed 6 years ago

Handle the user's user mode

Categories

(Chat Core :: IRC, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 8 obsolete files)

*** Original post on bio 1419 at 2012-05-03 12:42:00 UTC ***

Sending "/mode" on its own produces no response, since 221 (RPL_UMODEIS) is not handled yet.

The MODE handler improvements in bug 953761 (bio 318) also contain a TODO "Otherwise the user's own mode is being returned to them. // TODO"

329 (RPL_CREATIONTIME) is not handled (I am not sure what it is for, and it might not be important to handle this, but it seems it is also sent in response to /mode)
Depends on: 953761
*** Original post on bio 1419 at 2012-05-03 12:44:09 UTC ***

"499:You're not a channel owner" which may be sent in response to a /mode command.
*** Original post on bio 1419 at 2012-05-03 12:46:30 UTC ***

Example for 329 in response to "/mode #testzz":

irc: Sending:
MODE :#testzz
irc: onTransportStatus(STATUS_SENDING_TO)
irc: onTransportStatus(STATUS_RECEIVING_FROM)
irc: :gravel.mozilla.org 324 adev #testzz +n
irc: Unhandled IRC message: :gravel.mozilla.org 324 adev #testzz +n
irc: :gravel.mozilla.org 329 adev #testzz 1336047900
irc: Unhandled IRC message: :gravel.mozilla.org 329 adev #tczz 1336047900
*** Original post on bio 1419 at 2012-05-03 12:48:23 UTC ***

(In reply to comment #2)
Copy/paste error:

> irc: Unhandled IRC message: :gravel.mozilla.org 329 adev #tczz 1336047900

Should read

irc: Unhandled IRC message: :gravel.mozilla.org 329 adev #testzz 1336047900
Whiteboard: [1.2-wanted]
*** Original post on bio 1419 at 2012-05-03 13:26:45 UTC ***

(In reply to comment #1)
> "499:You're not a channel owner" which may be sent in response to a /mode
> command.

This is sent in response to e.g. "/mode nick +o" when you don't have the permissions.
Whiteboard: [1.2-wanted]
*** Original post on bio 1419 at 2012-05-03 13:52:51 UTC ***

FWIW, libpurple returns the channel mode (not the user mode) in response to "/mode" without parameters. So in the very limited sense of there currently not being any response to this input, it's a regression...
*** Original post on bio 1419 at 2012-05-24 19:25:02 UTC ***

What do you suggest we do with any of this information anyway? I mean if you just want to quiet the messages in the error console, we can flip a bunch of "false" to "true" and we're done. I'm not sure what we can really use the user mode for...

Any suggestions?
*** Original post on bio 1419 at 2012-05-28 13:14:04 UTC ***

How about
- add a handler for 329, but do nothing with it (ignore)
- handle 499 with an error message to the conversation where the user typed the mode command that triggered it
- print the user's mode to the conversation when requested by the user ("Your mode is +ixz"), otherwise ignore it (e.g. when joining a channel)
*** Original post on bio 1419 at 2012-09-01 12:33:28 UTC ***

Other responses for MODE that are unhandled:
- When new ban masks are given (bug 955084 (bio 1655)).
- When a channel key is given.
Summary: Add the remaining missing /mode responses → Add the remaining missing MODE responses
*** Original post on bio 1419 at 2012-10-17 19:19:51 UTC ***

Also 221 (user mode response)
Summary: Add the remaining missing MODE responses → Handle the remaining MODE responses (user mode in particular)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2003 at 2012-10-21 15:10:00 UTC ***

Provides the user mode as a system message in the server tab and handles 221, 329, and 499.
Attachment #8353762 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353762 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2003 at 2012-10-21 15:14:31 UTC ***

Uh, wait...
Attachment #8353762 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2004 at 2012-10-21 16:13:00 UTC ***

We didn't store the name of the server we are connected to anywhere, and the server name is not included in the MODE message, so we can't write to the existing server tab. So this patch adds storing the current server name on connection.
Attachment #8353763 - Flags: review?(clokep)
Comment on attachment 8353762 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2003 at 2012-10-21 16:13:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353762 - Attachment is obsolete: true
Comment on attachment 8353763 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2004 at 2012-10-21 16:21:20 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>@@ -742,6 +744,21 @@ ircAccount.prototype = {
>       this.sendMessage("AWAY"); // Mark as back.
>   },
> 
>+  // The user's user mode.
>+  _userMode: "",
Call this _modes and use _setMode like we do for conversations.

>+  setUserMode: function(aNick, aMode, aServerName) {
>+    if (this.normalize(aNick) != this.normalize(this._nickname))
>+      return false;
Should this also throw an error?
Attachment #8353763 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2005 at 2012-10-21 19:20:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353764 - Flags: review?(clokep)
Comment on attachment 8353763 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2004 at 2012-10-21 19:20:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353763 - Attachment is obsolete: true
Comment on attachment 8353764 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2005 at 2012-10-21 19:38:40 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>+  setUserMode: function(aNick, aNewModes, aSetter, aDisplayFullMode) {
>+    if (this.normalize(aNick) != this.normalize(this._nickname)) {
>+      WARN("Received unexpected mode for " +  aNick);
>+      return false;
>+    }
>+
>+    if (!this._modes.length)
>+      aDisplayFullMode = true;
>+
>+    // Are modes being added or removed?
>+    if (aNewModes[0] != "+" && aNewModes[0] != "-") {
>+      WARN("Invalid mode string: " + aNewModes);
>+      return false;
>+    }
>+    let addNewMode = aNewModes[0] == "+";
>+
>+    _setMode.call(this, addNewMode, aNewModes.slice(1));
>+
>+    if (this._showServerTab) {
>+      let msg;
>+      if (aDisplayFullMode)
>+        msg = _("message.yourmode", aNewModes);
>+      else
>+        msg = _("message.usermode", aNewModes, aNick, aSetter || this._currentServerName);
>+      this.getConversation(this._currentServerName)
>+          .writeMessage(this._currentServerName, msg, {system: true});
If you have to pass in this parameter but then fork the code, shouldn't this just be part of the handlers?

>@@ -777,6 +778,11 @@ var ircBase = {
>+    "329": function(aMessage) { // RPL_CREATIONTIME
>+      // <channel> <time>
>+      // We don't currently have any use for this information.
>+      return true;
>+    },
We already handle this? http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircNonStandard.jsm#51

>@@ -1342,6 +1348,10 @@ var ircBase = {
>+    "499": function(aMessage) { // ERR_CHANOWNPRIVNEEDED
>+      // <channel> :You're not the channel owner (status +q is needed)
>+      return conversationErrorMessage(this, aMessage, "error.notChannelOwner");
>+    },
This needs to go into ircNonStandard.
Attachment #8353764 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2006 at 2012-10-21 19:56:00 UTC ***

(In reply to comment #15)
> >+      if (aDisplayFullMode)
> If you have to pass in this parameter but then fork the code, shouldn't this
> just be part of the handlers?
That would be nice, but unfortunately when we are informed of our mode on connection, we get a MODE and not a 221. So it doesn't map as neatly as one would like.

> >@@ -777,6 +778,11 @@ var ircBase = {
> >+    "329": function(aMessage) { // RPL_CREATIONTIME
> We already handle this?
So we do - I missed that we already ignore it elsewhere ;)
Attachment #8353765 - Flags: review?(clokep)
Comment on attachment 8353764 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2005 at 2012-10-21 19:56:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353764 - Attachment is obsolete: true
*** Original post on bio 1419 at 2012-10-21 20:47:12 UTC ***

I'm no longer happy with the this._modes.length check as a proxy for "are we receiving the mode on connection" as it also fails when reconnecting.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2007 at 2012-10-21 21:07:00 UTC ***

Get rid of the unreliable this._modes.length check.
Attachment #8353766 - Flags: review?(clokep)
Comment on attachment 8353765 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2006 at 2012-10-21 21:07:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353765 - Attachment is obsolete: true
Attachment #8353765 - Flags: review?(clokep)
Comment on attachment 8353766 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2007 at 2012-10-25 00:13:04 UTC ***

>@@ -742,6 +744,44 @@ ircAccount.prototype = {
>+  // The user's user mode.
>+  _modes: [],
>+  _userModeReceived: false,
>+  setUserMode: function(aNick, aNewModes, aSetter, aDisplayFullMode) {
>+    if (this.normalize(aNick) != this.normalize(this._nickname)) {
>+      WARN("Received unexpected mode for " +  aNick);
>+      return false;
>+    }
>+
>+    // When connecting, the server sends us a MODE message informing us of the
>+    // user's mode. We should not report this initial mode message as a mode
>+    // change initiated by the user, but instead display the full mode.
>+    if (!this._userModeReceived) {
>+      this._userModeReceived = true;
>+      aDisplayFullMode = true;
>+    }
Instead of doing this check here, in the "MODE" handler, let's call setUserMode with !this._userModeReceiveed as the last parameter, then after the call set this._userModeReceived to true. Keep the comment though, it's helpful. :)

>+    // Are modes being added or removed?
>+    if (aNewModes[0] != "+" && aNewModes[0] != "-") {
>+      WARN("Invalid mode string: " + aNewModes);
>+      return false;
>+    }
>+    let addNewMode = aNewModes[0] == "+";
I'm tempted to make you not check whether aNewmodes[0] == "+" twice, but eh...that's a nit.

>+    _setMode.call(this, addNewMode, aNewModes.slice(1));
>+
>+    if (this._showServerTab) {
>+      let msg;
>+      if (aDisplayFullMode)
>+        msg = _("message.yourmode", aNewModes);
Shouldn't this display ALL of the modes? E.g. this._modes.join("")?

>diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm
>-function conversationErrorMessage(aAccount, aMessage, aError) {
>-  let conv = aAccount.getConversation(aMessage.params[1]);
>-  conv.writeMessage(aMessage.servername, _(aError, aMessage.params[1]),
>-                    {error: true, system: true});
>-  delete conv._pendingMessage;
>-  return true;
>-}
>-
I wish we didn't have to export this anywhere...
Attachment #8353766 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2013 at 2012-10-25 14:50:00 UTC ***

(In reply to comment #19)
> Instead of doing this check here, in the "MODE" handler, let's call setUserMode
> with !this._userModeReceiveed as the last parameter, then after the call set
> this._userModeReceived to true.
Good idea.

> >+    let addNewMode = aNewModes[0] == "+";
> I'm tempted to make you not check whether aNewmodes[0] == "+" twice, but
> eh...that's a nit.
Also, it was actually your code ;) But I changed it.

> >+    if (this._showServerTab) {
> >+      let msg;
> >+      if (aDisplayFullMode)
> >+        msg = _("message.yourmode", aNewModes);
> Shouldn't this display ALL of the modes? E.g. this._modes.join("")?
When it is used, the two are the same, but I changed it as it is clearer and more robust to future changes.
Attachment #8353773 - Flags: review?(clokep)
Comment on attachment 8353766 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2007 at 2012-10-25 14:50:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353766 - Attachment is obsolete: true
Summary: Handle the remaining MODE responses (user mode in particular) → Handle the user's user mode
Comment on attachment 8353773 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2013 at 2012-11-25 15:00:41 UTC ***

Sorry for letting this bitrot slightly, it is just two instances (one in irc.js and one in ircBase.jsm), both are really easy to fix.

>diff --git a/components/irc.js b/components/irc.js
>+    // When connecting, the server sends us a MODE message informing us of the
>+    // user's mode. We should not report this initial mode message as a mode
>+    // change initiated by the user (but instead display the full mode).
>+    this._userModeReceived = true;
I think this comment and setting of _userModeReceived should be part of the handler in ircBase.jsm, is there a good reason for it being here?

I think besides that I'll be OK with the changes in here.
Attachment #8353773 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2132 at 2012-11-25 16:37:00 UTC ***

(In reply to comment #21)
> I think this comment and setting of _userModeReceived should be part of the
> handler in ircBase.jsm, is there a good reason for it being here?
I moved the comment (and I think you're right about that), but kept the setting of _userModeReceived in the setMode function. Otherwise it unnecessarily complicates the handlers and is duplication.
Attachment #8353893 - Flags: review?(clokep)
Comment on attachment 8353773 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2013 at 2012-11-25 16:37:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353773 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1419 as attmnt 2133 at 2012-11-25 16:41:00 UTC ***

Fixes strange indentation error in comment.
Attachment #8353894 - Flags: review?(clokep)
Comment on attachment 8353893 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2132 at 2012-11-25 16:41:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353893 - Attachment is obsolete: true
Attachment #8353893 - Flags: review?(clokep)
Comment on attachment 8353773 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2013 at 2012-11-26 21:51:37 UTC ***

After discussing this with aleth, we've decided this is the most prudent way forward:
3:41:53 PM - clokep_work: aleth: So I don't understand why you can put this._userModeReceived = true; after line 272, the other time you call setUserMode you always pass in true.
3:42:42 PM - aleth: That would work, but we'd need it after the other call too.
3:43:09 PM - aleth: In case some server out there decides to use the more obvious message code to tell us our mode.
3:44:03 PM - aleth: Just to be on the safe side really...
3:44:30 PM - clokep_work: OK, so to be explicit, you're concerned about a server sending us 221 without us requesting our mode?
3:45:02 PM - aleth: Yes, or the server sending us nothing automatically and then the user requesting the mode for themselves. You'd want the flag to be set then too.
3:45:39 PM - aleth: Maybe there is some RFC that says the server always sends MODE though, idk. I couldn't find one.
4:34:18 PM - clokep_work: So you should never receive a numeric response without requesting one: http://tools.ietf.org/html/rfc2812#section-5
4:34:30 PM - clokep_work: The second option...well...hm...
4:38:31 PM - clokep_work: Then yes, please set review on the patch that was that way (or upload a new one if you had fixed something else).
4:38:34 PM - clokep_work: And I can r+ it.

It is not required to send MODE during connection.
Attachment #8353773 - Attachment is obsolete: false
Attachment #8353773 - Flags: review- → review+
Comment on attachment 8353894 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2133 at 2012-11-26 21:52:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353894 - Flags: review?(clokep)
Whiteboard: [checkin-needed]
Attached patch PatchSplinter Review
*** Original post on bio 1419 as attmnt 2137 at 2012-11-27 14:18:00 UTC ***

Unbitrotted, carried the r+ forward.
Attachment #8353898 - Flags: review+
Comment on attachment 8353773 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2013 at 2012-11-27 14:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353773 - Attachment is obsolete: true
Comment on attachment 8353894 [details] [diff] [review]
Patch

*** Original change on bio 1419 attmnt 2133 at 2012-11-27 14:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353894 - Attachment is obsolete: true
*** Original post on bio 1419 at 2012-11-27 23:49:01 UTC ***

http://hg.instantbird.org/instantbird/rev/6b6ab71745dc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.