*** Original post on bio 1305 at 2012-03-01 01:57:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1305 as attmnt 1209 at 2012-03-01 01:57:00 UTC *** The mode message was broken on JS-IRC, this fixes it with some smarts for handling the current conversation, etc. This also makes a small change to the API available in ircCommands, but having a getConv function to get the JS object behind a conversation.

*** Original post on bio 1305 at 2012-03-01 12:46:25 UTC *** (In reply to comment #0) > This also makes a small change to the API available in ircCommands, but having > a getConv function to get the JS object behind a conversation. Is this related to the mode changes? It seems the mode command would now support setting the mode of several nicks at once (mode +vv instantbot instant-buildbot). Is this right? Should this be visible from the help string?

*** Original post on bio 1305 at 2012-03-01 13:13:31 UTC *** (In reply to comment #1) > (In reply to comment #0) > > > This also makes a small change to the API available in ircCommands, but having > > a getConv function to get the JS object behind a conversation. > > Is this related to the mode changes? No, it's just a bit of clean up to the file. > It seems the mode command would now support setting the mode of several nicks > at once (mode +vv instantbot instant-buildbot). Is this right? Should this be > visible from the help string? I don't think this is right. At least it doesn't seem to be implied by the specification [1] and I don't have any code to handle that specially. Does something imply this? I'm generally unhappy with the complexity of this command and how overridden it is for different use cases: 1. Setting your own server mode (/mode +abc) 2. Setting a channel's mode (/mode <channel> +abc) 3. Setting a user's mode in a channel (/mode <channel> +abc <nick>) 4. Setting a user's mode in the current channel (/mode +abc <nick>) The original command syntax from the libpurple string was [<channel>|<nick>] (+|-)<new mode>, I like this syntax better, but it seems to imply to me to remove being able to do #3 from above. (And the syntax of #4 would become /mode <nick> +abc.) [1] http://tools.ietf.org/html/rfc2812#section-3.2.3

*** Original post on bio 1305 at 2012-03-04 19:44:27 UTC *** (In reply to comment #1) > (In reply to comment #0) > It seems the mode command would now support setting the mode of several nicks > at once (mode +vv instantbot instant-buildbot). Is this right? Should this be > visible from the help string? If we can come up with a fairly easy help string to describe this then yes. I couldn't come up with one that wasn't very confusing. :(

*** Original post on bio 1305 at 2012-03-04 19:57:24 UTC *** > 1. Setting your own server mode (/mode +abc) > 2. Setting a channel's mode (/mode <channel> +abc) > 3. Setting a user's mode in a channel (/mode <channel> +abc <nick>) > 4. Setting a user's mode in the current channel (/mode +abc <nick>) > > The original command syntax from the libpurple string was [<channel>|<nick>] > (+|-)<new mode>, I like this syntax better, but it seems to imply to me to > remove being able to do #3 from above. (And the syntax of #4 would become /mode > <nick> +abc.) Speaking as a non-IRC-expert, I think your suggestion makes things clearer and more consistent. It's quite hard to come up with situations where #3 would be necessary. One could then be more flexible about the order and allow both [<channel>|<nick>]> (+|-)<mode> (+|-)<mode> [<channel>|<nick>]>

*** Original post on bio 1305 as attmnt 1326 at 2012-04-10 23:43:00 UTC *** I checked out this patch again, fixed a small bug in it and unbitrotted it. Florian asked whether this can handle multiple nicks or not. It CANNOT, but it can (fairly easily) be added if this is desired.

Comment on attachment 8352960 [details] [diff] [review] Patch *** Original change on bio 1305 attmnt 1209 at 2012-04-10 23:43:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1305 at 2012-04-11 09:32:01 UTC *** (In reply to comment #5) > Florian asked whether this can handle multiple nicks or not. It CANNOT, but it > can (fairly easily) be added if this is desired. Did the mode command of the libpurple IRC prpl support multiple nicks? If not, don't bother :).

*** Original post on bio 1305 at 2012-04-11 10:30:43 UTC *** (In reply to comment #6) > (In reply to comment #5) > > > Florian asked whether this can handle multiple nicks or not. It CANNOT, but it > > can (fairly easily) be added if this is desired. > > Did the mode command of the libpurple IRC prpl support multiple nicks? If not, > don't bother :). I don't think it did. [1] [1] http://lxr.instantbird.org/pidgin2.6.3/source/libpurple/protocols/irc/cmds.c#229

Comment on attachment 8353079 [details] [diff] [review] Patch (unbitrotted) *** Original change on bio 1305 attmnt 1326 at 2012-04-12 21:55:59 UTC *** >diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm >--- a/chat/protocols/irc/ircCommands.jsm >+++ b/chat/protocols/irc/ircCommands.jsm >@@ -209,17 +209,32 @@ var commands = [ > { > name: "memoserv", > get helpString() _("command.memoserv", "memoserv"), > run: function(aMsg, aConv) privateMessage(aConv, aMsg, "MemoServ") > }, > { > name: "mode", > get helpString() _("command.mode", "mode"), >- run: function(aMsg, aConv) simpleCommand(aConv, "MODE", aMsg) >+ run: function(aMsg, aConv) { >+ let params = aMsg.split(" "); >+ >+ // If only a mode message is given, it's to set your own mode, we also Do you really want that "message" word in here? >+ // have to provide our own nick. >+ if (params.length == 1) >+ params.unshift(aConv.nick); >+ // If a new mode and a nick are given, then it's for the current >+ // conversation. >+ else if (params.length == 2 && !getAccount(aConv).isMUCName(params[0])) >+ params.reverse().unshift(aConv.name); params = [aConv.name, params[1], params[0]]; would be easier to read >+ // Otherwise assume a channel, new mode and nick were given or a channel >+ // and a mode were given. >+ Having a comment before an empty line feels strange but I can see why you did that. Maybe add another sentence to the comment saying nothing needs to be changed to the parameters for either of these cases? Looks good anyway.

*** Original post on bio 1305 as attmnt 1334 at 2012-04-12 22:02:00 UTC *** Carrying your review forward flo, as this only fixes the nits you asked for.

Comment on attachment 8353079 [details] [diff] [review] Patch (unbitrotted) *** Original change on bio 1305 attmnt 1326 at 2012-04-12 22:02:33 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1305 as attmnt 1336 at 2012-04-12 22:14:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353087 [details] [diff] [review] Patch with nits fixed *** Original change on bio 1305 attmnt 1334 at 2012-04-12 22:14:01 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1305 at 2012-04-13 00:06:31 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/b9b845b38806

*** Original post on bio 1305 as attmnt 1342 at 2012-04-13 01:51:00 UTC *** I found a slight bug in this implementation. It fails to handle "/mode #instantbird" properly, which should fetch the mode of #instantbird (not that it's a very useful case, but right now we try to send "MODE clokep #instantbird", which is nonsense).

*** Original post on bio 1305 at 2012-04-13 01:51:29 UTC *** Reopening...

Comment on attachment 8353095 [details] [diff] [review] Handle a different case *** Original change on bio 1305 attmnt 1342 at 2012-04-18 23:35:17 UTC *** This looks good in the narrow sense of the reason this bug was reopened :) I think further improvements will follow in the other mode bug? (e.g. apart from the channel modes in general, checking in the last case there are actually three parameters and not more, and possibly allowing /mode +x nick as well as /mode nick +x)

*** Original post on bio 1305 at 2012-04-19 11:06:41 UTC *** (In reply to comment #12) > Created attachment 8353095 [details] [diff] [review] (bio-attmnt 1342) [details] > Handle a different case > > I found a slight bug in this implementation. It fails to handle "/mode > #instantbird" properly, which should fetch the mode of #instantbird (not that > it's a very useful case, So what does that do exactly? Will there be some user feedback displayed anywhere? Shouldn't one of the comments be updated to take this case into account?

*** Original post on bio 1305 at 2012-04-19 12:02:34 UTC *** (In reply to comment #15) > (In reply to comment #12) > > Created attachment 8353095 [details] [diff] [review] (bio-attmnt 1342) [details] > > Handle a different case > > > > I found a slight bug in this implementation. It fails to handle "/mode > > #instantbird" properly, which should fetch the mode of #instantbird (not that > > it's a very useful case, > > So what does that do exactly? Will there be some user feedback displayed > anywhere? Currently there is not, but the current version of the mode command sends garbage in this case. It would send something like: MODE clokep #instantbird Which isn't even a command. Although actually now that I'm looking at it again...I wonder if we should better handle if you were to do /mode clokep, which is nonsensical too. > Shouldn't one of the comments be updated to take this case into account? Probably. (In reply to comment #14) > Comment on attachment 8353095 [details] [diff] [review] (bio-attmnt 1342) [details] > Handle a different case > > This looks good in the narrow sense of the reason this bug was reopened :) I > think further improvements will follow in the other mode bug? What bug is this? > (e.g. apart from > the channel modes in general, checking in the last case there are actually > three parameters and not more, and possibly allowing /mode +x nick as well as > /mode nick +x) I do not think this is necessary. But file a separate bug if you want it.

*** Original post on bio 1305 at 2012-04-19 13:51:12 UTC *** (In reply to comment #16) > > This looks good in the narrow sense of the reason this bug was reopened :) I > > think further improvements will follow in the other mode bug? > What bug is this? Bug 953761 (bio 318). > Which isn't even a command. Although actually now that I'm looking at it > again...I wonder if we should better handle if you were to do /mode clokep, > which is nonsensical too. Which was a comment of mine on that bug ;)

*** Original post on bio 1305 at 2012-04-20 10:55:01 UTC *** I'm looking over this for a bit more error checking, so not ready for checkin.

*** Original post on bio 1305 as attmnt 1378 at 2012-04-20 21:25:00 UTC *** So this is practically a total rewrite again, I think it has enough comments this time. The help text is updated to what the code actually does too. Also, I hate handling user input.

Comment on attachment 8353095 [details] [diff] [review] Handle a different case *** Original change on bio 1305 attmnt 1342 at 2012-04-20 21:25:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353131 [details] [diff] [review] Rewritten function again :( *** Original change on bio 1305 attmnt 1378 at 2012-04-20 22:26:31 UTC *** "/mode nick " will be counted as two parameters and end up in the wrong if clause. Similarly "/mode +o nick ". I *think* that's due to bug 954813 (bio 1378) not having landed yet. (If not, r-) Two little things which didn't stop me from r+, but you might want to change, or not: "/mode +o +o" (admittedly silly) does not return false, not sure if that matters. "/mode nick" returns false - should it return that nick's current mode instead?

*** Original post on bio 1305 at 2012-04-21 13:04:58 UTC *** (In reply to comment #20) > Comment on attachment 8353131 [details] [diff] [review] (bio-attmnt 1378) [details] > Rewritten function again :( > > "/mode nick " will be counted as two parameters and end up in the wrong if > clause. Similarly "/mode +o nick ". I *think* that's due to bug 954813 (bio 1378) not having > landed yet. (If not, r-) Yes, this is due to bug 954813 (bio 1378)! Thanks for mentioning it though. :) > Two little things which didn't stop me from r+, but you might want to change, > or not: > > "/mode +o +o" (admittedly silly) does not return false, not sure if that > matters. > "/mode nick" returns false - should it return that nick's current mode instead? It does not. It works fine for me. What I tested is below. Should work: /mode /mode #testib2 /mode #testib2 +t /mode #testib2 +v clokep /mode clokep_js /mode clokep_js +x /mode +x /mode +v clokep Should not work: /mode clokep +x /mode -o +v Note that your example /mode +o +o is technically "valid" for a channel of +o and a new mode o. (On moznet, # and + are valid channel prefixes.)

*** Original post on bio 1305 as attmnt 1379 at 2012-04-21 13:05:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353131 [details] [diff] [review] Rewritten function again :( *** Original change on bio 1305 attmnt 1378 at 2012-04-21 13:05:39 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353132 [details] [diff] [review] Rewritten function again v2 *** Original change on bio 1305 attmnt 1379 at 2012-04-21 13:41:00 UTC *** Looks good :) >> "/mode nick" returns false - should it return that nick's current mode >> instead? > It does not. It works fine for me. What I tested is below. I was referring to when the nick is not the user's nick. You do return false in that case.

*** Original post on bio 1305 at 2012-04-23 23:01:48 UTC *** It's landed! Checked in as http://hg.instantbird.org/instantbird/rev/ef44e7b31270.