Closed Bug 954973 Opened 10 years ago Closed 10 years ago

MODE fails to handle when a key is set and we are in the room

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1541 at 2012-06-23 13:45:00 UTC ***

Timestamp: 6/23/2012 9:43:09 AM
Error: Error running command MODE with handler RFC 2812:
{"rawMessage":":clokep_js!Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com MODE #testib2 +k key","command":"MODE","params":["#testib2","+k","key"],"nickname":"clokep_js","user":"Instantbir","host":"moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com","source":"Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com"}
Source File: resource:///modules/ircHandlers.jsm
Line: 101
Attached patch WIP v1 (obsolete) — Splinter Review
*** Original post on bio 1541 as attmnt 1881 at 2012-09-01 15:34:00 UTC ***

I thought this bug was going to be like a 3 line patch, boy was I wrong...turns out we're handling MODE really poorly: for a MODE message, there's a +/- followed by a set of mode characters, then a set of "mode parameters", but not all modes have a parameter...so all modes essentially need to be handled (or at least whether that mode expects a parameter or not).

This needs some more work to handle all the modes from http://tools.ietf.org/html/rfc2811#section-4 and https://www.alien.net.au/irc/chanmodes.html, but I wanted some feedback...

One thing I haven't tested is setting the mode for multiple people at once (or multiple modes on the same person at once).
Attachment #8353639 - Flags: feedback?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353639 [details] [diff] [review]
WIP v1

*** Original change on bio 1541 attmnt 1881 at 2012-09-01 19:30:47 UTC ***

This is very legible actually! In a way it actually simplifies things. The +k handling is nice too.

So the syntax is (+/-)(modes) (stack of parameters to pop from as needed).
The /mode help message (and possibly the parsing for /mode) will need adjusting too...

Would it be more elegant to .reverse the aModeParams array and then use .pop rather than modeParamNum?

|else if (aNewMode[i] == "k") ...| should probably be |else switch(aNewMode[i]) ...| considering there will be quite a few eventually. Unless you dislike switch statements of course.

// Modes that are considered to can have a nickname parameter.
missing words alert!
Attachment #8353639 - Flags: feedback?(bugzilla) → feedback+
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1541 as attmnt 1896 at 2012-09-04 02:34:00 UTC ***

(In reply to comment #2)
> This is very legible actually! In a way it actually simplifies things. The +k
> handling is nice too.
Yeah, I tried to make it straight forward and less magical.

> So the syntax is (+/-)(modes) (stack of parameters to pop from as needed).
> The /mode help message (and possibly the parsing for /mode) will need adjusting too...
I think that's a different bug. This concerns receiving the MODE command only.

> Would it be more elegant to .reverse the aModeParams array and then use .pop
> rather than modeParamNum?
Or just iterate over the array backwards? That's what I chose to do. Do we think there's anyway we need to go through it forwards?

> |else if (aNewMode[i] == "k") ...| should probably be |else switch(aNewMode[i])
> ...| considering there will be quite a few eventually. Unless you dislike
> switch statements of course.
I think some of the statements are more than just an ==. Also, I dislike case statements with many lines in them.

> // Modes that are considered to can have a nickname parameter.
> missing words alert!
Yeah that comment makes no sense. Thanks.

I'm not totally sold on the way we're displaying the mode changes, but maybe that's bug 954855 (bio 1420).
Attachment #8353654 - Flags: review?(bugzilla)
Attached patch Patch v1.1 (obsolete) — Splinter Review
*** Original post on bio 1541 as attmnt 1897 at 2012-09-04 02:43:00 UTC ***

Forgot to qrefresh. :)
Attachment #8353655 - Flags: review?(bugzilla)
Comment on attachment 8353639 [details] [diff] [review]
WIP v1

*** Original change on bio 1541 attmnt 1881 at 2012-09-04 02:43:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353639 - Attachment is obsolete: true
Comment on attachment 8353654 [details] [diff] [review]
Patch v1

*** Original change on bio 1541 attmnt 1896 at 2012-09-04 02:43:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353654 - Attachment is obsolete: true
Attachment #8353654 - Flags: review?(bugzilla)
Comment on attachment 8353655 [details] [diff] [review]
Patch v1.1

*** Original change on bio 1541 attmnt 1897 at 2012-09-04 12:25:25 UTC ***

>+  setMode: function(aNewMode, aModeParams, aSetter) {
>+    const hostMaskExp = /^.+!.+@.+$/;
>+    function getNextParam() {
>+      // If there's no next parameter, throw a warning.
>+      if (!aModeParams.length)
>+        WARN("Channel key set but not channel key given!");
>+      return aModeParams.pop();
>+    }
The warning assumes the missing parameter was a missing parameter of +k rather than some other mode. We can't assume this.

>+          this.hasParticipant(aModeParams.slice(-1)[0])) {
This should use a variant of getNextParam that uses slice instead of pop, so that a check that the parameter exists can also be made for this case.

>+      else if (aNewMode[i] == "R" && aModeParams.slice(-1).match(hostMaskExp)) {
Needs to be slice(-1)[0] (or actually use the new function as above).

Otherwise r+ I think!

>I'm not totally sold on the way we're displaying the mode changes, but maybe that's bug 954855 (bio 1420).
Yes, I don't have any good ideas for that either atm. At least with this it should work and display something.

>Also, I dislike case statements with many lines in them.
I always thought it was a shame they took the syntax unmodified from C. 'else if' chains are ugly too.

>I think that's a different bug. This concerns receiving the MODE command only.
Agreed, but we should file it :)
Attachment #8353655 - Flags: review?(bugzilla) → review-
*** Original post on bio 1541 at 2012-09-04 12:32:07 UTC ***

(In reply to comment #5)
> Comment on attachment 8353655 [details] [diff] [review] (bio-attmnt 1897) [details]
> Patch v1.1
> 
> >+  setMode: function(aNewMode, aModeParams, aSetter) {
> >+    const hostMaskExp = /^.+!.+@.+$/;
> >+    function getNextParam() {
> >+      // If there's no next parameter, throw a warning.
> >+      if (!aModeParams.length)
> >+        WARN("Channel key set but not channel key given!");
> >+      return aModeParams.pop();
> >+    }
> The warning assumes the missing parameter was a missing parameter of +k rather
> than some other mode. We can't assume this.
Doh, I forgot to change this. Also this checks the length and then doesn't care and tries to pop anyway...it should return null or undefined in this case.

> >+          this.hasParticipant(aModeParams.slice(-1)[0])) {
> This should use a variant of getNextParam that uses slice instead of pop, so
> that a check that the parameter exists can also be made for this case.
It probably makes more sense to just have a hasNextParam() method or something and check that first.

> >+      else if (aNewMode[i] == "R" && aModeParams.slice(-1).match(hostMaskExp)) {
> Needs to be slice(-1)[0] (or actually use the new function as above).
Good catch.
*** Original post on bio 1541 at 2012-09-04 16:09:45 UTC ***

Also, maybe if we get sent a mode we don't yet handle, we could print a WARN or ERROR rather than producing some sequence of errors further down in the code which is messier to identify.
*** Original post on bio 1541 at 2012-09-04 16:24:38 UTC ***

(In reply to comment #7)
> Also, maybe if we get sent a mode we don't yet handle, we could print a WARN or
> ERROR rather than producing some sequence of errors further down in the code
> which is messier to identify.
Hmm, I'm not sure this (later errors) still happens with this patch? Unknown modes are simply not handled.
*** Original post on bio 1541 at 2012-09-04 17:16:36 UTC ***

(In reply to comment #8)
> (In reply to comment #7)
> > Also, maybe if we get sent a mode we don't yet handle, we could print a WARN or
> > ERROR rather than producing some sequence of errors further down in the code
> > which is messier to identify.
> Hmm, I'm not sure this (later errors) still happens with this patch? Unknown
> modes are simply not handled.

This is purposefully done. Not all MODEs actually have any action to occur (i.e. MODE a or i), there's nothing to do in this case. I mean I guess we could add an empty if clause and then warn in an else at the bottom.
*** Original post on bio 1541 at 2012-09-04 17:46:01 UTC ***

Right - just wanting to prevent error sequences like that in bug 955091 (bio 1662) in the future if poss. But like I said, I think that's probably already the case with this patch.
Attached patch Patch v2Splinter Review
*** Original post on bio 1541 as attmnt 1899 at 2012-09-05 01:05:00 UTC ***

Fixes the review comments.

Also makes some extra changes to the way _setMode is called and only calls setMode once for each participant.
Attachment #8353657 - Flags: review?(bugzilla)
Comment on attachment 8353655 [details] [diff] [review]
Patch v1.1

*** Original change on bio 1541 attmnt 1897 at 2012-09-05 01:05:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353655 - Attachment is obsolete: true
Comment on attachment 8353657 [details] [diff] [review]
Patch v2

*** Original change on bio 1541 attmnt 1899 at 2012-09-05 23:16:16 UTC ***

Looks good :)

I wonder if, as a small UI improvement, we should change the system message string for channel modes to distinguish it from the string for user modes (eg. "Channel mode +t #channel" and "User mode ..."). Since we distinguish in the code we can make it more easily visible for the user too?
Attachment #8353657 - Flags: review?(bugzilla) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1541 at 2012-09-13 12:13:15 UTC ***

I've no idea of what this patch does (too long to read it), but it's checked-in: http://hg.instantbird.org/instantbird/rev/7f579220e56b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
Depends on: 955149
You need to log in before you can comment on or make changes to this bug.