1105660 - /mode command fails in some cases

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[momiga]

The /mode command either sends the wrong command or does not allow the command to be run in a few cases.

If the command is simply "/mode" or has only a modestring argument such as "/mode +x", pressing enter does nothing and the command stays in the textbox (in Thunderbird).

If the command is "/mode +x momiga", it seems to send "MODE <current window> +x". For example, doing it in the server window (levin.mozilla.org), the server replies with: "levin.mozilla.org is not online." Doing it in a channel causes nothing to be printed.

Comment 1

[momiga]

Steps to reproduce "/mode" and "/mode +x" not executing:
1) Go to any window.
2) Type "/mode" or "/mode +x".
3) Press enter.
4) Nothing happens; command remains in text box.

Steps to reproduce "/mode +x momiga" setting a channel mode instead of a user mode:
1) Go to any non-channel window. (If you do it in a channel, you get no error message, but the command still fails.)
2) type "/mode +x momiga", replacing "momiga" with your nick.
3) Press enter.
4) Server responds: "<name of window> is not online."

Comment 2

[aleth [:aleth]]

> Steps to reproduce "/mode +x momiga" setting a channel mode instead of a
> user mode:
> 1) Go to any non-channel window. (If you do it in a channel, you get no
> error message, but the command still fails.)

The next step here is to figure out what exactly you mean by "the command fails". Is it 
- an error in the error console during the handling of the command
- no MODE command is sent to the server 
- MODE is sent, but the reply is not handled
- the reply is handled, but no system message is displayed in the conversation for the user

Comment 3

[aleth [:aleth]]

(In reply to momiga from comment #1)
> Steps to reproduce "/mode +x momiga" setting a channel mode instead of a
> user mode:
> 1) Go to any non-channel window. (If you do it in a channel, you get no
> error message, but the command still fails.)
> 2) type "/mode +x momiga", replacing "momiga" with your nick.
> 3) Press enter.
> 4) Server responds: "<name of window> is not online."

Why would you expect "/mode +x momiga" sent from a non-channel window to set a channel mode? It's not at all clear from your STR what you expect to happen vs what does happen.

Comment 4

[Matt A. Tobin]

(In reply to aleth [:aleth] from comment #3)
> Why would you expect "/mode +x momiga" sent from a non-channel window to set
> a channel mode? It's not at all clear from your STR what you expect to
> happen vs what does happen.

some special usermodes have nothing to do with a channel. For instance mode +x relates to host cloaking. This is a server wide user mode.

Comment 5

[momiga]

Attachment: Patch /mode bug.

Changes the syntax and semantics of /mode somewhat. /mode will no longer infer the current user to be the target if the target is omitted. Instead, /umode should be used for that.

The reason for this is twofold: it greatly simplifies the handler code, and it makes the command more intuitive and in-line with existing IRC clients.

Here's an example of the unintuitive behavior of the previous implementation: "/mode +v momiga" generates a command to change the channel mode, but "/mode +m" generates a command to change the server-wide user mode.

The new proposed syntax is:
/mode - Show the mode for the current conversation target.
/mode <channel|user> - Show the mode for the given channel/user.
/mode <mode> [args] - Change the mode for the current conversation target.
/mode (channel|user) <mode> [args] - Change the mode for the given channel/user.

This could be made much more concise. It's expanded into four descriptions here for clarity.

Some usage example:
/mode - Will simply print the channel mode (or user mode if in a private conversation).
/mode momiga - Will print momiga's user mode.
/mode #sheep - Will print #sheep's channel mode.
/mode momiga +x - Give momiga the x user mode.
/mode #sheep +m - Give #sheep the m channel mode.
/mode #sheep +o momiga - Give momiga mode o on #sheep.

Comment 6

[momiga]

(In reply to aleth [:aleth] from comment #3)
> Why would you expect "/mode +x momiga" sent from a non-channel window to set
> a channel mode? It's not at all clear from your STR what you expect to
> happen vs what does happen.

Sorry, mode x is a user mode, so I'd expect it to set the user mode. Moreover, I'd expect it to set the user mode according to the syntax specified in /help mode:

> mode (+|-)<new mode> [<nick>]: Set or unset a user's mode.

Comment 7

[Patrick Cloke [:clokep]]

O I've found this pretty confusing, but I think what you're proposing is using only the second usage of the current mode command, the help message would be:
mode [<channel>] (+|-)<new mode> [<parameter>][,<parameter>]*]: Get, set or unset a channel mode.

Where the <channel> would be assumed to be the current channel (if not given)?

I'm still confused at why you have user in comment 5.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8529735 [details] [diff] [review]
Patch /mode bug.

Review of attachment 8529735 [details] [diff] [review]:
-----------------------------------------------------------------

I kind of hate the current command, so I'm (currently) thinking this is a reasonable change.

Could you please add tests to ensure different mode syntaxes send the right command? Thanks!

::: chat/protocols/irc/ircCommands.jsm
@@ +266,4 @@
>      get helpString() _("command.modeUser", "mode") + "\n" +
>                       _("command.modeChannel", "mode"),
>      run: function(aMsg, aConv) {
> +      function isMode(aString) aString && "+-".contains(aString[0]);

Why the added aString check? When is this called where it's possible for aString to be empty?

@@ +272,5 @@
>  
> +      // Infer command target based on context, if necessary.
> +      if (!aMsg)
> +        params = [target];
> +      else if (isMode(params[0]) && !isMode(params[1]))

Can you add a comment above each section about what it's trying to handle?

@@ +278,2 @@
>  
> +      // The mode string must be the second argument.

I think "Ensure the second argument is the mode string." is clearer.

Comment 9

[aleth [:aleth]]

I'd still like an answer to comment #2 ;)

Comment 10

[momiga]

(In reply to aleth [:aleth] from comment #2)
> The next step here is to figure out what exactly you mean by "the command
> fails". Is it 
> - an error in the error console during the handling of the command
> - no MODE command is sent to the server 
> - MODE is sent, but the reply is not handled
> - the reply is handled, but no system message is displayed in the
> conversation for the user

In the first case ("/mode" and "/mode +x"), no command gets sent because an exception is thrown in the handler. In the second case (all the others), a command is sent, but it has the wrong arguments (name of current window, when current user's nickname is expected).

>mode [<channel>] (+|-)<new mode> [<parameter>][,<parameter>]*]: Get, set or unset a channel mode.
>
>Where the <channel> would be assumed to be the current channel (if not given)?

Yes, that's right.

>I'm still confused at why you have user in comment 5.

Because it's still possible to set the mode of a user, if you supply a nick name. This could include yourself, or other users if you're a sysop. The actual server command accepts either, so I figured this command should as well.

This is also necessary because /umode doesn't work for setting the mode of other users, only oneself. So a sysop trying to change a user's mode would have to use /mode regardless.

>Could you please add tests to ensure different mode syntaxes send the right command? Thanks!

Sure, though I'm not sure where to do that.

>Why the added aString check? When is this called where it's possible for aString to be empty?

aString can be empty only if aMsg is empty, and doing the check in isMode handles it without extra checks elsewhere. Really, this is weird behavior from splitInput (splitting "" -> [""]), and should be addressed in splitInput, but the rest of the code in the file expects it like that and I wanted to touch as little code as possible.

>Can you add a comment above each section about what it's trying to handle?
>I think "Ensure the second argument is the mode string." is clearer.

Sure, I'll make those changes.

Comment 11

[Patrick Cloke [:clokep]]

(In reply to momiga from comment #10)
> >I'm still confused at why you have user in comment 5.
> 
> Because it's still possible to set the mode of a user, if you supply a nick
> name. This could include yourself, or other users if you're a sysop. The
> actual server command accepts either, so I figured this command should as
> well.
> 
> This is also necessary because /umode doesn't work for setting the mode of
> other users, only oneself. So a sysop trying to change a user's mode would
> have to use /mode regardless.

Does it make more sense to allow umode to accept parameters instead of forcing it in here?

> >Could you please add tests to ensure different mode syntaxes send the right command? Thanks!
> 
> Sure, though I'm not sure where to do that.

Likely in a separate file called test_ircCommands.js.

> >Why the added aString check? When is this called where it's possible for aString to be empty?
> 
> aString can be empty only if aMsg is empty, and doing the check in isMode
> handles it without extra checks elsewhere. Really, this is weird behavior
> from splitInput (splitting "" -> [""]), and should be addressed in
> splitInput, but the rest of the code in the file expects it like that and I
> wanted to touch as little code as possible.
splitInput does exactly what it was designed to do: it trims whitespace, splits on whitespace and *always* returns an array. This allows callers to easily know what is being returned. You'll need to explain more about what you mean by "weird behavior".

I don't see how this is possible, if aMsg is empty it will be caught by the if-statement before it (!aMsg) and not go into this else-if. The second case, we ensure there are at least two parameters before feeding in the parameter to check.

Comment 12

[aleth [:aleth]]

(In reply to momiga from comment #10)
> (In reply to aleth [:aleth] from comment #2)
> > The next step here is to figure out what exactly you mean by "the command
> > fails". Is it 
> > - an error in the error console during the handling of the command
> > - no MODE command is sent to the server 
> > - MODE is sent, but the reply is not handled
> > - the reply is handled, but no system message is displayed in the
> > conversation for the user
> 
> In the first case ("/mode" and "/mode +x"), no command gets sent because an
> exception is thrown in the handler. In the second case (all the others), a
> command is sent, but it has the wrong arguments (name of current window,
> when current user's nickname is expected).

So I was hoping for a little more detailed information, like:

"/mode" in a channel sends "MODE nick", in response to which we receive a 221 message which is handled without errors. No message is currently displayed to the user. It should probably print the mode in a system message, file a separate bug for this if you like.

"/mode" in a PM indeed leads to

Error: TypeError: trailing is undefined
Source File: resource://gre/components/irc.js
Line: 1599

This is indeed a bug, please file it separately unless you are fixing it here in this bug.

That way the individual issues become individual, actionable bugs.

Comment 13

[aleth [:aleth]]

(In reply to momiga from comment #5)
> The new proposed syntax is:
> /mode - Show the mode for the current conversation target.
> /mode <channel|user> - Show the mode for the given channel/user.
> /mode <mode> [args] - Change the mode for the current conversation target.
> /mode (channel|user) <mode> [args] - Change the mode for the given
> channel/user.

With these changes, would /mode still be available as a command in PMs?

If it is, isn't it just as confusing that /mode <mode> will change a channel mode in a channel, and the mode of the person you are speaking to in a PM? The command would suddenly switch from defaulting to "apply to the user" to "apply to the target".

> Some usage example:
> /mode - Will simply print the channel mode (or user mode if in a private
> conversation).

How about making this print the channel mode *and* the user mode, to avoid disappointment? 

> /mode momiga - Will print momiga's user mode.
> /mode #sheep - Will print #sheep's channel mode.
> /mode momiga +x - Give momiga the x user mode.
> /mode #sheep +m - Give #sheep the m channel mode.
> /mode #sheep +o momiga - Give momiga mode o on #sheep.

So if I understand your proposal right, "/mode momiga +o" sets a global user mode, while "/mode #sheep +o momiga" makes momiga a channel op, and "/mode #sheep +o" fails and prints a help message? Is this less confusing than the current syntax, or just different?

I think I would probably prefer to have /mode only ever set channel modes.

Comment 14

[momiga]

(In reply to Patrick Cloke [:clokep] from comment #11)
> Does it make more sense to allow umode to accept parameters instead of
> forcing it in here?

(In reply to aleth [:aleth] from comment #13)
> If it is, isn't it just as confusing that /mode <mode> will change a channel
> mode in a channel, and the mode of the person you are speaking to in a PM?
> The command would suddenly switch from defaulting to "apply to the user" to
> "apply to the target".

> How about making this print the channel mode *and* the user mode, to avoid
> disappointment? 

> So if I understand your proposal right, "/mode momiga +o" sets a global user
> mode, while "/mode #sheep +o momiga" makes momiga a channel op, and "/mode
> #sheep +o" fails and prints a help message? Is this less confusing than the
> current syntax, or just different?
> 
> I think I would probably prefer to have /mode only ever set channel modes.

To answer all of these, the behavior follows from how the server interprets the the MODE command. If you give it a channel, it sets (or shows) modes on the channel. If you give it a user, it sets (or shows) modes on the user.

This change makes the /mode command more in line with the MODE server command, while allowing you to omit the target if you want to target the current window. It also makes the code much simpler and easier to reason about, with fewer edge cases to handle.

Forcing /mode to only change channel modes and forcing /umode to only change user modes is additional code that needs to be included to check the type of the target for no gain in convenience or usability. It's also surprising for anyone who has used other IRC clients. Lastly, it's fighting with the spec. The spec says MODE takes a channel or a user, so why not /mode?

It makes more sense I think for /mode to change the mode on any server object (inferring from the current window, if no target is given), and /umode to be provided essentially as a convenient alias for "/mode my_nick".

(In reply to Patrick Cloke [:clokep] from comment #11)
> splitInput does exactly what it was designed to do: it trims whitespace,
> splits on whitespace and *always* returns an array. This allows callers to
> easily know what is being returned. You'll need to explain more about what
> you mean by "weird behavior".

The weird behavior is when it's passed an empty string (i.e. zero arguments). It returns an array with an empty string in it, rather than an empty array. To me it makes more sense for zero arguments to yield a zero-length array, rather than an array with "" inside it.

Existing code in the file would seem to agree. :) E.g.:

>// Check if we have any params, we can't just check params.length, since
>// that will always be at least 1 (but params[0] would be empty).
>let hasParams = !/^\s*$/.test(aMsg);

> I don't see how this is possible, if aMsg is empty it will be caught by the
> if-statement before it (!aMsg) and not go into this else-if. The second
> case, we ensure there are at least two parameters before feeding in the
> parameter to check.

You're right, I was mistaken. It can happen when params[1] is undefined, e.g. "/mode #sheep". Having isMode handle undefineds means we don't need extra checks for params.length.

(In reply to aleth [:aleth] from comment #12)
> So I was hoping for a little more detailed information, like:

I can't provide much more detailed information. For "/mode" and "/mode +x", there is no debugging output. It simply doesn't let me execute the command (in Thunderbird). I inferred it was because of an uncaught exception in the command handler, but that was after messing around with the code myself. This happens in any IRC window, irrespective of whether it's a channel, a user, the server window, etc.

As for "/mode +x momiga", I'm not sure exactly what it's sending or what the server is replying on the wire. Only that it prints this in the server window: "levin.mozilla.org is not online." From this, I infer it must be sending "MODE levin.mozilla.org +x momiga", since that is what would produce that output. It makes sense that it would send that, but that's not what the syntax for the command according to "/help mode" says it should send.

Comment 15

[aleth [:aleth]]

(In reply to momiga from comment #14)
> (In reply to aleth [:aleth] from comment #12)
> > So I was hoping for a little more detailed information, like:
> 
> I can't provide much more detailed information. For "/mode" and "/mode +x",
> there is no debugging output. It simply doesn't let me execute the command
> (in Thunderbird). I inferred it was because of an uncaught exception in the
> command handler, but that was after messing around with the code myself.
> This happens in any IRC window, irrespective of whether it's a channel, a
> user, the server window, etc.
> 
> As for "/mode +x momiga", I'm not sure exactly what it's sending or what the
> server is replying on the wire. Only that it prints this in the server
> window: "levin.mozilla.org is not online." From this, I infer it must be
> sending "MODE levin.mozilla.org +x momiga", since that is what would produce
> that output. It makes sense that it would send that, but that's not what the
> syntax for the command according to "/help mode" says it should send.

I can help with that -- here's how I got the info I pasted in the bug:

For errors, there is the error console (available from the Tools menu), where you can see if a particular action causes new errors/warnings/messages.

To see what is actually sent/received to the server, look at the debug log for the account (available from the context menu of the account in the account manager). In Instantbird, there is now a built-in debug log viewer, in Thunderbird, you get it in the clipboard.

Generally, for debugging it helps to have your own build and add debugging output where necessary (e.g. using dump() or Components.utils.reportError() commands).

Comment 16

[momiga]

(In reply to aleth [:aleth] from comment #15)
> I can help with that -- here's how I got the info I pasted in the bug:
> 
> For errors, there is the error console (available from the Tools menu),
> where you can see if a particular action causes new errors/warnings/messages.
> 
> To see what is actually sent/received to the server, look at the debug log
> for the account (available from the context menu of the account in the
> account manager). In Instantbird, there is now a built-in debug log viewer,
> in Thunderbird, you get it in the clipboard.
> 
> Generally, for debugging it helps to have your own build and add debugging
> output where necessary (e.g. using dump() or Components.utils.reportError()
> commands).

Thanks, I should have known to check there. :) I'll be more thorough in future bug reports.

Comment 17

[Patrick Cloke [:clokep]]

(In reply to momiga from comment #14)
> To answer all of these, the behavior follows from how the server interprets
> the the MODE command. If you give it a channel, it sets (or shows) modes on
> the channel. If you give it a user, it sets (or shows) modes on the user.

What data is sent over the server should have pretty much nothing to do with how the user interface is implemented. We're not constrained by someone defining silly messages in the protocol. That being said, go ahead with your proposal. I look forward to seeing an updated patch.

> (In reply to Patrick Cloke [:clokep] from comment #11)
> > splitInput does exactly what it was designed to do: it trims whitespace,
> > splits on whitespace and *always* returns an array. This allows callers to
> > easily know what is being returned. You'll need to explain more about what
> > you mean by "weird behavior".
> 
> The weird behavior is when it's passed an empty string (i.e. zero
> arguments). It returns an array with an empty string in it, rather than an
> empty array. To me it makes more sense for zero arguments to yield a
> zero-length array, rather than an array with "" inside it.

Hmm...I'm unsure if this is done on purpose or not, but I suspect it isn't.

> (In reply to aleth [:aleth] from comment #12)
> > So I was hoping for a little more detailed information, like:
> As for "/mode +x momiga", I'm not sure exactly what it's sending or what the
> server is replying on the wire. Only that it prints this in the server
> window: "levin.mozilla.org is not online." From this, I infer it must be
> sending "MODE levin.mozilla.org +x momiga", since that is what would produce
> that output. It makes sense that it would send that, but that's not what the
> syntax for the command according to "/help mode" says it should send.

I disagree; this is exactly what the syntax says it will do. But there's no point in discussing this if we're going to change the syntax.

To summarize, I think we've decided on the syntax. Please ensure you update the help string also (found under chat/locales/en-US/irc.properties) as well. Thanks!

Comment 18

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #13)

OK, some comments on how I understand this proposal, as I'm still a bit confused about the details.

> (In reply to momiga from comment #5)
> > The new proposed syntax is:
> > /mode - Show the mode for the current conversation target.

I find this intuitive in a channel, and not intuitive in a PM.
I would expect "/mode" to show me my own modes as well in either case, but maybe I'm the only one.
After all, unless I'm an IRC op, I can't affect other people's user modes anyway.

> > /mode <channel|user> - Show the mode for the given channel/user.

OK.

In a channel, does "/mode user" show the channel modes of that user (e.g. channel voice, channel operator) or the global user modes of that user or both? (again, if possible, I'd suggest always showing as much as possible)

> > /mode <mode> [args] - Change the mode for the current conversation target.

This means that to change my own mode, I always have to specify my nick, or use /umode. Are we OK with this?

> > /mode (channel|user) <mode> [args] - Change the mode for the given
> > channel/user.

I think it would be *really* helpful if "mode +h nhnt11" and "mode nhnt11 +h" always did the same thing, as the order of those arguments is one of the hard to remember things about this.

In a channel, this command would mean "mode #channel +h nhnt11" - i.e. sets the channel mode.

In a PM, it would refer to the user mode. Only IRC ops would generally be able to use this.

This means that in a channel, this command would offer no way to set global user modes.

Am I missing something?

Comment 19

[momiga]

(In reply to aleth [:aleth] from comment #18)
> I find this intuitive in a channel, and not intuitive in a PM.
> I would expect "/mode" to show me my own modes as well in either case, but
> maybe I'm the only one.
> After all, unless I'm an IRC op, I can't affect other people's user modes
> anyway.

I'm not sure what else a user would expect from using /mode in a PM. Generally one only has the need to use /mode in a channel context anyway. I don't think it's intuitive that /mode's behavior would change in a PM, and it also adds complexity to the implementation to check that.

But yes, "/mode" showing both channel modes and one's own mode is useful and easy to do. Although, it raises a small question: what about "/umode" with no arguments?

> In a channel, does "/mode user" show the channel modes of that user (e.g.
> channel voice, channel operator) or the global user modes of that user or
> both? (again, if possible, I'd suggest always showing as much as possible)

It would show the user's global modes. As far as I know, MODE can't be used to get the channel modes for a user in a channel. You have to use NAMES or perhaps WHOIS to do that. I'm not sure if overloading the command and adding complexity to the handler to do that is a good idea.

> This means that to change my own mode, I always have to specify my nick, or
> use /umode. Are we OK with this?

I think that's the intention behind /umode in the first place. :)

> I think it would be *really* helpful if "mode +h nhnt11" and "mode nhnt11
> +h" always did the same thing, as the order of those arguments is one of the
> hard to remember things about this.
> 
> In a channel, this command would mean "mode #channel +h nhnt11" - i.e. sets
> the channel mode.
> 
> In a PM, it would refer to the user mode. Only IRC ops would generally be
> able to use this.
>
> This means that in a channel, this command would offer no way to set global
> user modes.
> 
> Am I missing something?

One of the things that has to be considered is how much more complex these concessions make the handler. It's easy to say "make it accept arbitrary ordering", but what that entails is having to inspect the arguments to determine what they are, define how handle ambiguities, anticipate edge cases, and also communicate all this to the user.

Not the mention the fact that modes can take parameters, and whether or not a mode takes a parameter is not known by the client in advance. So now /mode, in order to interpret and rearrange the arguments, will need to differentiate between the argument specifying the target of the command and arguments to individual modes.

All in all, I feel loosening the ordering greatly increases the complexity of the handler and the number of cases that need to be considered. I think it's best to opt for simplicity in this case.


Just as a general point for the whole conversation, it should be noted that /mode is somewhat of a poweruser command. Normal users will just use /op, /ban, etc. Specifying modes using /mode is typically something a long-time IRC user will be doing. They're probably familiar with the ins and outs of using /mode and would expect a basic and familair syntax. Just my perspective on it.

If everyone agrees, I'll finish up the patches (including tests and help messages). I'd do it tonight, but I'm a bit busy, so I'll do it this weekend.

Comment 20

[Patrick Cloke [:clokep]]

(In reply to momiga from comment #19)
> One of the things that has to be considered is how much more complex these
> concessions make the handler. It's easy to say "make it accept arbitrary
> ordering", but what that entails is having to inspect the arguments to
> determine what they are, define how handle ambiguities, anticipate edge
> cases, and also communicate all this to the user.
> 
> Not the mention the fact that modes can take parameters, and whether or not
> a mode takes a parameter is not known by the client in advance. So now
> /mode, in order to interpret and rearrange the arguments, will need to
> differentiate between the argument specifying the target of the command and
> arguments to individual modes.

This is a separate enhancement request. Please do not consider it for this bug.

> If everyone agrees, I'll finish up the patches (including tests and help
> messages). I'd do it tonight, but I'm a bit busy, so I'll do it this weekend.

Please go ahead and put up a new patch. I think that will drive discussion in a clearer direction.

Comment 21

[Patrick Cloke [:clokep]]

momiga, any chance of finishing off this patch? I actually ran into some of these issues again last night. :)

Comment 22

[momiga]

(In reply to Patrick Cloke [:clokep] from comment #21)
> momiga, any chance of finishing off this patch? I actually ran into some of
> these issues again last night. :)

No, I don't think so. I lost the changes I had made to the code. It was mostly just revised comments and a half finished test case. Someone else can take the assignment and/or use the attached patch as they desire.

Comment 23

[Patrick Cloke [:clokep]]

Removing assignment. If you're still working on this, please feel free to comment!

Comment 24

[Pavan Karthik [:matrixisreal]]

Hi Cloke,
This looks like a long comment section.
Can you summarize what changes are made to the functionality of the /mode command?(if any).
Thanks.

Comment 25

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

Comment 26

[Pavan Karthik [:matrixisreal]]

This patch has tests and changes for both /umode and /mode commands.

Comment 27

[Patrick Cloke [:clokep]]

Thanks! I reworked the tests to remove some unnecessary code / fix some styling issues.

https://hg.mozilla.org/comm-central/rev/05be743c0ebcecaa8cfc9327cbc04e1a26262134

Comment 29

[Patrick Cloke [:clokep]]

Comment on attachment 8842458 [details] [diff] [review]
v1.patch

Oops. Forgot to r+. As I mentioned above, this was r+ with some nits fixed!

Comment 30

[Pavan Karthik [:matrixisreal]]

Awesome :D