568439 - Can't /part multiple channels simultaneously

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[David Vo [:auscompgeek]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100524 BetterPrivacy-1.46 Minefield/3.7a5pre AutoPager/0.6.1.6 AutoPager/0.6.1.6
Build Identifier: ChatZilla 0.9.86-rdmsoft [XULRunner 1.9.3a5pre/20100524030807]

It is impossible to part multiple channels in the one command, without using /raw.

Reproducible: Always

Steps to Reproduce:
1. Join multiple channels.
2. Try to /part those channels, separating them using commas, like /join.
Actual Results:  
ChatZilla says you aren't in the channel #qa,#testday,... (or whatever channels you tried to /part).

Expected Results:  
ChatZilla should send the part message to the server, closing the channels' views if specified.

Comment 1

[Svetlana Tkachenko]

It reproduces here. Status is being set to NEW.

Comment 2

[:Gijs (he/him)]

Attachment: /part should work on multiple channels,

This seems to work.

Comment 3

[James Ross]

Comment on attachment 811494 [details] [diff] [review]
/part should work on multiple channels,

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

Seems generally okay, though the error-case (breaking out of the loop) is not explained in the comments.

::: xul/content/commands.js
@@ +2491,5 @@
> +
> +        var commaPosition = e.channelName.indexOf(",");
> +
> +        // If there's a comma in the middle of the channel name somewhere:
> +        if (commaPosition > 0 && commaPosition != e.channelName.length - 1)

This seems like a load of unnecessary conditioning to me. Why not just split e.channelName, iterate, skipping blanks?

Comment 4

[:Gijs (he/him)]

Attachment: /part should work on multiple channels,

I think this is what you meant wrt the comment, but I'll just re-request review to be sure... :-)

Comment 5

[James Ross]

Comment on attachment 813291 [details] [diff] [review]
/part should work on multiple channels,

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

Thanks, that looks better. It's still a bit weird, if you do "/leave #chatzilla,bye!" on #addons, you'll leave #chatzilla (with no message) and #addons (with "bye!"), but trying to support comma-lists and optional channel names is confusing at best.

Comment 6

[:Gijs (he/him)]

https://hg.mozilla.org/chatzilla/rev/c2da17fbbbf7

Comment 7

[:Gijs (he/him)]

Attachment: unbreak the context menu 'leave' option,

Someone on IRC pointed out I broke something. Oops.

Comment 8

[James Ross]

Comment on attachment 815755 [details] [diff] [review]
unbreak the context menu 'leave' option,

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

::: xul/content/commands.js
@@ +2491,5 @@
>          return;
>      }
>  
> +    if (!e.hasOwnProperty("channelName") && e.channel)
> +        e.channelName = e.channel.unicodeName;

It kind of feels bad going from a channel object to a name and then back again, but it should be safe.

Comment 9

[:Gijs (he/him)]

(In reply to James Ross from comment #8)
> Comment on attachment 815755 [details] [diff] [review]
> unbreak the context menu 'leave' option,
> 
> Review of attachment 815755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xul/content/commands.js
> @@ +2491,5 @@
> >          return;
> >      }
> >  
> > +    if (!e.hasOwnProperty("channelName") && e.channel)
> > +        e.channelName = e.channel.unicodeName;
> 
> It kind of feels bad going from a channel object to a name and then back
> again, but it should be safe.

https://hg.mozilla.org/chatzilla/rev/25a4bc20b148