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)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.3
People
(Reporter: clokep, Assigned: clokep)
References
Details
Attachments
(1 file, 3 obsolete files)
10.82 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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
Assignee | ||
Comment 1•10 years ago
|
||
*** 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 | ||
Updated•10 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
*** 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)
Assignee | ||
Comment 4•10 years ago
|
||
*** Original post on bio 1541 as attmnt 1897 at 2012-09-04 02:43:00 UTC *** Forgot to qrefresh. :)
Attachment #8353655 -
Flags: review?(bugzilla)
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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-
Assignee | ||
Comment 8•10 years ago
|
||
*** 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.
Comment 9•10 years ago
|
||
*** 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.
Comment 10•10 years ago
|
||
*** 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.
Assignee | ||
Comment 11•10 years ago
|
||
*** 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.
Comment 12•10 years ago
|
||
*** 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.
Assignee | ||
Comment 13•10 years ago
|
||
*** 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)
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 16•10 years ago
|
||
*** 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
You need to log in
before you can comment on or make changes to this bug.
Description
•