682682 - Allow for usage of /part command outside of channel tab

Status: VERIFIED FIXED

(Other Applications :: ChatZilla, defect)

Description

[David Vo [:auscompgeek]]

Attachment: one-liner patch

ChatZilla doesn't allow for the usage of the /part command in the server tab. There should not be any reason for this.

Comment 1

[Tony Mechelynck [:tonymec]]

I thought /part (alias for /leave) was for a channel and that for a server you would use /disconnect ?

Comment 2

[James Ross]

Comment on attachment 556372 [details] [diff] [review]
one-liner patch

It is a command that operates on channels but, like a number of commands, it also accepts a <channel-name> argument so there's no good reason you can't run it from the network tab.

r=silver if you've tested it. ;)

Comment 3

[Tony Mechelynck [:tonymec]]

ah, sorry, I had misunderstood: you want to use /leave with a channel name (even from a server tab or a query tab) to leave the specified channel, and change nothing to the current tab.

Comment 4

[David Vo [:auscompgeek]]

In case you guys weren't watching just then, I just tested the patch.
It works brilliantly. :)

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

The patch does not apply cleanly to current CZ trunk, and when corrected, I get this when typing "/leave" in moznet:

[ERROR]	Internal error dispatching command “leave”.
[ERROR]	TypeError: e.channelName is null @ <chrome://chatzilla/content/commands.js> 2420

Removing c-n keyword, please update the patch.

Comment 6

[Tony Mechelynck [:tonymec]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #5)
> The patch does not apply cleanly to current CZ trunk, and when corrected, I
> get this when typing "/leave" in moznet:
[...]

When typing /leave in a network tab, shouldn't you add a channel, name, i.e.
/leave #chatzilla
/leave #developers
etc.?

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Tony Mechelynck [:tonymec] from comment #6)
> When typing /leave in a network tab, shouldn't you add a channel, name

Maybe, but the point is that if I do what I did (which is a valid input, even if not a valid command), without the patch I get a message, but with the patch I get a JavaScript error. That's not clean. IOW, if I were the reviewer, this alone would justify r-.

Comment 8

[James Ross]

Comment on attachment 556372 [details] [diff] [review]
one-liner patch

Not tested enough.

Comment 9

[Donald Sarratt]

Attachment: Slightly longer patch

This patch adds a check to make sure there's a valid channel name, and returns an error if none found. I've tested it in channel, server and user views.

Comment 10

[David Vo [:auscompgeek]]

nit: you forgot to line up the brackets in the first part of your patch.

Comment 11

[Donald Sarratt]

(In reply to auscompgeek from comment #10)
> nit: you forgot to line up the brackets in the first part of your patch.

Lol nice catch. That'd drive me crazy if I had to work with it repeatedly.

Comment 12

[James Ross]

Comment on attachment 646963 [details] [diff] [review]
Slightly longer patch

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

::: xul/content/commands.js
@@ +102,4 @@
>           ["kick",              cmdKick,            CMD_NEED_CHAN | CMD_CONSOLE],
>           ["kick-ban",          cmdKick,            CMD_NEED_CHAN | CMD_CONSOLE],
>           ["knock",             cmdKnock,            CMD_NEED_SRV | CMD_CONSOLE],
> +         ["leave",             cmdLeave,           CMD_NEED_SRV | CMD_CONSOLE],

Nit: Add one more space before CMD_NEED_SRV so the end of the lines line up.

@@ +2398,4 @@
>  
>      if (e.hasOwnProperty("channelName"))
>      {
> +        if (e.channelName == null)

Does work with !e.channelName? I think that would be clearer, but only if it works. :)

Comment 13

[Donald Sarratt]

Attachment: Updated patch

You're right, !e.channelName works fine. This patch should do the trick.

Comment 14

[James Ross]

http://hg.mozilla.org/chatzilla/rev/36f9cda246e3

Comment 15

[Tony Mechelynck [:tonymec]]

After installing the distribution/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}.xpi contained in hourly build http://ftp.mozilla.org/pub/mozilla.org/seamonkey/tinderbox-builds/comm-central-trunk-linux64/1347220767/seamonkey-2.15a1.en-US.linux-x86_64.tar.bz2 which has build ID 20120909125927, the use of "/leave #channelname" in the context of a network or query tab has the desired effect: no error, and after returning to the corresponding channel tab, the last entry at the bottom is "YOU (tonymec) have left #channelname".

=> VERIFIED.

Comment 16

[Tony Mechelynck [:tonymec]]

P.S. From the context of a _different_ channel tab, "/leave #channelname" leaves the channel given as an argument and not the current channel, which is also the desired effect.

Comment 17

[Tony Mechelynck [:tonymec]]

P.P.S. (after rereading comment #5):
"/leave" with no arguments in the context of a network or query tab still gives "[ERROR] Command “leave” must be run in the context of a channel.". In the context of a channel tab it still leaves the current channel.

"/leave #foobar" gives "[ERROR] The channel “#foobar” is not known to ChatZilla.".

No ChatZilla-related Error Console messages whatever I do.

/!\ With no leading # in the channel name, the *current* channel is left, and the argument is displayed as the "reason". This does not correspond to the nesting of brackets in the help:
[USAGE]	leave [<channel-name> [<reason>]]

Comment 18

[Donald Sarratt]

(In reply to Tony Mechelynck [:tonymec] from comment #17)
> P.P.S. (after rereading comment #5):
> "/leave" with no arguments in the context of a network or query tab still
> gives "[ERROR] Command “leave” must be run in the context of a channel.". In
> the context of a channel tab it still leaves the current channel.

This is the intended behavior, it's a gracefully-handled error instead of an unhandled error. I don't see any way for just "/leave" to be valid outside the context of a channel, so it should give an error.


> /!\ With no leading # in the channel name, the *current* channel is left,
> and the argument is displayed as the "reason". This does not correspond to
> the nesting of brackets in the help:
> [USAGE]	leave [<channel-name> [<reason>]]

That's an interesting point. Technically without a # there hasn't been a valid channel specified, so we should assume all arguments are part of the reason. We could attempt to match the first word of the reason to a channel, but that could lead to unintentionally leaving the wrong channel if the first word of the reason is also another channel.

Comment 19

[Donald Sarratt]

(In reply to Donald Sarratt from comment #18)
> > /!\ With no leading # in the channel name, the *current* channel is left,
> > and the argument is displayed as the "reason". This does not correspond to
> > the nesting of brackets in the help:
> > [USAGE]	leave [<channel-name> [<reason>]]
> 
> That's an interesting point. Technically without a # there hasn't been a
> valid channel specified, so we should assume all arguments are part of the
> reason. We could attempt to match the first word of the reason to a channel,
> but that could lead to unintentionally leaving the wrong channel if the
> first word of the reason is also another channel.

Or were you suggesting the brackets should be 
[USAGE] 	leave [<channel-name>] [<reason>]

Comment 20

[Tony Mechelynck [:tonymec]]

(In reply to Donald Sarratt from comment #19)
[...]
> Or were you suggesting the brackets should be 
> [USAGE] 	leave [<channel-name>] [<reason>]

Yes, with this kind of brackets a "reason" argument could appear without a "channel" argument, so anything not prefixed by # & or + must be assumed to be part of the reason. OTOH if the usage is [<channel-name> [reason]] the "reason" argument cannot appear without a "channel" argument. I suppose the change to the help could be handled either by REOPENing this bug, or (maybe preferably since no need to revert the changeset for this fix) in a followup bug.

Comment 21

[Donald Sarratt]

Yeah, that makes sense. Bug 789853 filed to amend the params listing.

Comment 22

[David Vo [:auscompgeek]]

(In reply to Tony Mechelynck [:tonymec] from comment #16)
What happens when you type /part channel (without a #) outside of a channel context?

Comment 23

[Donald Sarratt]

(In reply to david.vo2 from comment #22)
> (In reply to Tony Mechelynck [:tonymec] from comment #16)
> What happens when you type /part channel (without a #) outside of a channel
> context?

Chatzilla interprets the first word as the channel, and any remaining text as the leave reason.

(In reply to Donald Sarratt from comment #18)
> That's an interesting point. Technically without a # there hasn't been a
> valid channel specified, so we should assume all arguments are part of the
> reason. We could attempt to match the first word of the reason to a channel,
> but that could lead to unintentionally leaving the wrong channel if the
> first word of the reason is also another channel.
In testing I found this is completely wrong; Chatzilla will always attempt to interpret the first word as a channel regardless of whether it starts with a #. Only if the first word isn't a valid channel will CZ treat it as part of the leave reason.

Comment 24

[Steve Lis]

Requesting that the changes in this bug be rolled back and the bug REOPENED due to them causing a regression in previously working functionality outlined in bug 792402, comment 4

As I said there, I agree that the changes requested in this bug here should be implemented, but we need to figure out how to not impact previously working functionality. There may be more than one way to go about it, but until it can be figured out, I think that in the short term, it's more important to not get an error when trying to close channel tabs (a fairly common task) than it is to be able to do a "/leave #channel" from a server or query tab as the /leave command usually executed when looking at a channel tab anyway, and it can still be used when you're in a different channel tab than the one you want to close even without this bug being fixed.

One possible way to make this bug play nicely with bug 792402 would be to define something new like CMD_NEED_SRV2 just for cmdLeave which would leave out the:

 if (e.network.state != NET_ONLINE)
{
e.parseError = MSG_ERR_NOT_CONNECTED;
return false;
}

which is what causes the regression seen in bug 792402. (Due to the fact that closing a channel tab executes a cmdLeave if one disconnects from the server without leaving all channels first)

Alternatively, we could make a alternative version of cmdLeave (call it cmdLeaveDisc, for example) that only applies when it detects that you're no longer connected to the server. cmdLeaveDisc would keep the old value of CMD_NEED_CHAN so that it would work while not connected to the server while the regular cmdLeave could keep the new value of CMD_NEED_SRV that would be used only while actually connected to the server. Of course, this approach would involve programming ChatZilla to recognize whether or not one is connected to the server in order to determine if cmdLeave or cmdLeaveDisc should be applied, but maybe it could work.

Anyway, these are the two ideas I have. I trust that the people who actually code ChatZilla know best, so I open the floor to the (probably better) ideas from you folks.

Comment 25

[Steve Lis]

Donald promptly came up with a good possible solution in bug 792402, comment 5 that may eliminate any need to back out the changes from this bug. His proposed fix doesn't seem to break this bug upon an initial test, but it'll need to be examined more thoroughly to make sure. I guess we should see what happens with Donald's proposed fix before touching this bug again.