Closed Bug 955442 Opened 10 years ago Closed 10 years ago

The /invite command should support taking more than one nick as parameter

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2006 at 2013-06-17 10:54:00 UTC ***

Suggested syntax:
/invite <nick>[ <nick>]* [<channel>]

We can just check the last parameter to see if it's a channel or not.
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 2006 as attmnt 2519 at 2013-06-26 10:55:00 UTC ***

I debated whether the syntax should be <nick> <nick> <channel> or <nick>,<nick> <channel>, but the first places better w/ tab complete.

Untested (my build is busted).
Attachment #8354287 - Flags: review?(aleth)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 2006 at 2013-06-26 11:09:01 UTC ***

Comment on attachment 8354287 [details] [diff] [review] (bio-attmnt 2519)
Patch v1


>-command.invite=%S &lt;nick&gt; [&lt;room&gt;]: Invite someone to join you in the specified channel, or the current channel.
>+command.invite2=%S &lt;nick&gt;[ &lt;nick&gt;]* [&lt;room&gt;]: Invite someone to join you in the specified channel, or the current channel.

What about:
Invite one or more nicks to join you in the current channel, or to join the specified channel.

Do we want the word "room" or "channel"? The syntax says "room" but the help sentence says "channel" :-S.


>-      return simpleCommand(aConv, "INVITE", params);
>+      // Default to using the current conversation as the channel to invite to.
>+      let channel = aConv.name;
>+      let account = getAccount(aConv);
>+      // Find the first param that could be a channel name.
>+      for (let i = 0; i < params.length; ++i) {
>+        if (account.isMUCName(params[i])) {
>+          // Remove that parameter and store it.
>+          channel = params.splice(i, 1);
>+          break;
>+        }
>+      }

A few edge cases that don't seem to be handled here:
[1] /invite #channel
[2] /invite nick #channel1 #channel2

[1] Is obviously a wrong syntax. [2] could likely be supported, but I don't think it's worth it.

In both cases I think we could just return false.

I think the way I would implement this check is:
- if the last parameters looks like a MUC name, use it as the channel and remove it from the list (channel = params.splice)
- check all remaining parameters; if any of them is a MUC name, or if the list is empty, return false.

(This assumes that isMUCName doesn't have false positives.)
Comment on attachment 8354287 [details] [diff] [review]
Patch v1

*** Original change on bio 2006 attmnt 2519 at 2013-06-26 12:03:30 UTC ***

The help string could be improved (I think flo already has a suggestion for that), but the code looks fine to me. (I didn't test it though)
Attachment #8354287 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2006 at 2013-06-26 12:04:34 UTC ***

Florian already gave some good suggestions and we're not going to check in an untested patch.
Whiteboard: [checkin-needed]
*** Original post on bio 2006 at 2013-06-26 12:06:21 UTC ***

(In reply to comment #2)
I thought isMUCName could have false positives (i.e. it was possible to have unprefixed channel names on some servers), so there is no perfect way to handle this?
Whiteboard: [checkin-needed]
Comment on attachment 8354287 [details] [diff] [review]
Patch v1

*** Original change on bio 2006 attmnt 2519 at 2013-06-26 12:13:44 UTC ***

>I thought isMUCName could have false positives (i.e. it was possible to have
>unprefixed channel names on some servers), so there is no perfect way to handle
>this?
So I was completely wrong about that, in which case this can be improved a lot :)
Attachment #8354287 - Flags: review+ → review-
Whiteboard: [checkin-needed]
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 2006 as attmnt 2528 at 2013-06-28 01:12:00 UTC ***

This changes the behavior to reject situations where there are multiple channels given or if there are no parameters besides the channel. It also handles another error message, when you invite someone to a channel they're already in.
Attachment #8354296 - Flags: review?(aleth)
Comment on attachment 8354287 [details] [diff] [review]
Patch v1

*** Original change on bio 2006 attmnt 2519 at 2013-06-28 01:12:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354287 - Attachment is obsolete: true
Comment on attachment 8354296 [details] [diff] [review]
Patch v2

*** Original change on bio 2006 attmnt 2528 at 2013-06-29 08:28:09 UTC ***

>     "443": function(aMessage) { // ERR_USERONCHANNEL
>       // <user> <channel> :is already on channel
>-      // TODO
>+      this.getConversation(aMessage.params[2])
>+          .writeMessage(aMessage.servername,
>+                        _("message.alreadyInChannel", aMessage.params[1],
>+                          aMessage.params[2]), {system: true});
>       return false;

I think you want to return true here.

>+          // Remove that parameter and store it.
>+          channel = params.splice(i, 1)[0];
>+          break;

If you break here, you will never detect the presence of two channel names.
Attachment #8354296 - Flags: review?(aleth) → review-
Attached patch Patch v3Splinter Review
*** Original post on bio 2006 as attmnt 2539 at 2013-06-29 15:44:00 UTC ***

Thanks aleth, both silly mistakes.
Attachment #8354307 - Flags: review?(aleth)
Comment on attachment 8354296 [details] [diff] [review]
Patch v2

*** Original change on bio 2006 attmnt 2528 at 2013-06-29 15:44:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354296 - Attachment is obsolete: true
Comment on attachment 8354307 [details] [diff] [review]
Patch v3

*** Original change on bio 2006 attmnt 2539 at 2013-06-29 15:48:00 UTC ***

Thanks!
Attachment #8354307 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2006 at 2013-06-30 13:03:59 UTC ***

http://hg.instantbird.org/instantbird/rev/cd5947be952a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.