As mentioned on IRC, I think initially it's good enough to just tab-complete commands - there's a good chance can already complete for just '/' + character, so the description of only really useful in corner cases anyway ...

Created attachment 320041 [details]
mockup based on current implementation

(In reply to Florian Müllner from comment #1)
> As mentioned on IRC, I think initially it's good enough to just tab-complete
> commands - there's a good chance can already complete for just '/' +
> character, so the description of only really useful in corner cases anyway
> ...

You're thinking of keeping it similar to nick completion in the lines of this?
or literally just tab-complete?

(In reply to Bastian Ilsø from comment #2)
> You're thinking of keeping it similar to nick completion in the lines of
> this?
> or literally just tab-complete?

I mean instead of completing on ['nick', 'nick-foo', 'nick-bar'], we can complete on ['nick', 'nick-foo', 'nick-bar', '/cmd', '/cmd-foo']. We'll need to adjust what we insert a bit(*), but that should be far easier than dealing with different completions competing for the same events.


(*) on line start, 'f<tab>' completes to 'fmuellner: ', and 'f<tab>b<tab>' to 'fmuellner, bastianilso: ', which doesn't work for commands.

So I started working on this. Moving in direction as discussed with Florian on IRC. 

Couple of questions :

1.) Should we use the same list as used by nicks and just add '/' commands to the list? I am in favor of this, as we can reuse all the code in tabCompletion.js :-) 

2.) If yes for same list, can IRC nicks start with '/'? As this can be problem for us, since on each event(nick change, user joins/leaves) we recreate the list of nicks from scratch. Rather than adding all the '/' commands on each update, we can add during init() and when updating, recreate the list except adding again the '/' commands.

(In reply to Kunaal Jain from comment #4)
> So I started working on this. Moving in direction as discussed with Florian
> on IRC. 
> 
> Couple of questions :
> 
> 1.) Should we use the same list as used by nicks and just add '/' commands
> to the list? I am in favor of this, as we can reuse all the code in
> tabCompletion.js :-) 

I'm in favor of this as well.


> 2.) If yes for same list, can IRC nicks start with '/'?

Not according to the original specification[0], and I don't think later additions changed that.

[0] https://tools.ietf.org/html/rfc1459#section-2.3.1


> on each event(nick change, user joins/leaves) we
> recreate the list of nicks from scratch. 

Yeah, I don't think we hit quite the right balance between not doing unnecessary work and having tab completion work instantly. Maybe one option would be to store completions as a list of strings and then only create widgets for those that are actually needed (e.g. with 'a<tab>', we filter the list for entries that start with 'a', make sure we have a widget for it and fill the popup). But that's not quite in the scope of this bug :-)


> Rather than adding all the '/'
> commands on each update, we can add during init() and when updating,
> recreate the list except adding again the '/' commands.

Mmh, it's a bit ugly that the completion starts caring about what it completes, but I guess that's something we'll have to do anyway for the insertion part ...

(In reply to Florian Müllner from comment #5)
> (In reply to Kunaal Jain from comment #4)
> > So I started working on this. Moving in direction as discussed with Florian
> > on IRC. 
> > 
> > Couple of questions :
> > 
> > 1.) Should we use the same list as used by nicks and just add '/' commands
> > to the list? I am in favor of this, as we can reuse all the code in
> > tabCompletion.js :-) 
> 
> I'm in favor of this as well.
> 

:-) 

> 
> > 2.) If yes for same list, can IRC nicks start with '/'?
> 
> Not according to the original specification[0], and I don't think later
> additions changed that.
> 
> [0] https://tools.ietf.org/html/rfc1459#section-2.3.1
> 
> 
> > on each event(nick change, user joins/leaves) we
> > recreate the list of nicks from scratch. 
> 
> Yeah, I don't think we hit quite the right balance between not doing
> unnecessary work and having tab completion work instantly. Maybe one option
> would be to store completions as a list of strings and then only create
> widgets for those that are actually needed (e.g. with 'a<tab>', we filter
> the list for entries that start with 'a', make sure we have a widget for it
> and fill the popup). But that's not quite in the scope of this bug :-)
> 
> 

Will file another bug for that 

> > Rather than adding all the '/'
> > commands on each update, we can add during init() and when updating,
> > recreate the list except adding again the '/' commands.
> 
> Mmh, it's a bit ugly that the completion starts caring about what it
> completes, but I guess that's something we'll have to do anyway for the
> insertion part ...

Yes for '/' have to care whether it is at starting or not, and not to insert ": " with it.

Created attachment 321146 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Review of attachment 321146 [details] [review]:

::: src/tabCompletion.js
@@ +7,3 @@
+const IRCCommands = ["/HELP", "/INVITE", "/JOIN", "/KICK", "/ME", "/MSG",
+                     "/NAMES", "/NICK", "/PART", "/QUERY", "/QUIT", "/SAY",
+                     "/TOPIC"];

Maintaining a separate list of implemented commands is a bit ugly:

@@ +43,3 @@
         this._widgetMap = {};
+
+        for (let i = 0; i < IRCCommands.length; i++) {

You could do:

let commands = Object.keys(IrcParser.knownCommands);
for (let i = 0; i < commands.length; i++) {
    row._text = '/' + commands[i];
    ...

@@ +164,2 @@
     _getRowCompletion: function(row) {
+        if ((this._startPos == 0 || this._previousCompletion) && !this._isIRCCommand)

Commands are always followed by a space, so it makes sense to insert that. We'd have to figure out another way to disable chaining in that case though.

@@ +180,3 @@
+            return row._casefoldedText.startsWith(this._key) && row._casefoldedText.startsWith('/');
+        }
+        return row._casefoldedText.startsWith(this._key) && !row._casefoldedText.startsWith('/');

I don't understand this code - if we are completing a command, this._key is something like '/j', no? So "row._casefoldedText.startsWith(this._key);" should do the right thing in either case.

@@ +205,3 @@
         this._startPos = text.lastIndexOf(' ') + 1;
         this._key = text.toLowerCase().substr(this._startPos);
+        this._isIRCCommand = false;

Or simply:
  this._isIRCCommand = (this._startPos == 0 && this._key.startsWith('/');

Though I'd reconsider restricting command completions to the start of line - they don't actually work elsewhere, but they can still be mentioned:
 'you can get a list of supported command by typing /h<tab>'

(In reply to Florian Müllner from comment #8)
> Review of attachment 321146 [details] [review] [review]:
> 
> ::: src/tabCompletion.js
> @@ +7,3 @@
> +const IRCCommands = ["/HELP", "/INVITE", "/JOIN", "/KICK", "/ME", "/MSG",
> +                     "/NAMES", "/NICK", "/PART", "/QUERY", "/QUIT", "/SAY",
> +                     "/TOPIC"];
> 
> Maintaining a separate list of implemented commands is a bit ugly:
> 
> @@ +43,3 @@
>          this._widgetMap = {};
> +
> +        for (let i = 0; i < IRCCommands.length; i++) {
> 
> You could do:
> 
> let commands = Object.keys(IrcParser.knownCommands);
> for (let i = 0; i < commands.length; i++) {
>     row._text = '/' + commands[i];
>     ...


Thanks I was trying to do this only. But didn't know about Object.keys function :-(

> 
> @@ +164,2 @@
>      _getRowCompletion: function(row) {
> +        if ((this._startPos == 0 || this._previousCompletion) &&
> !this._isIRCCommand)
> 
> Commands are always followed by a space, so it makes sense to insert that.
> We'd have to figure out another way to disable chaining in that case though.

Yup, I deliberately didn't insert space to prevent chaining. Well there is a tradeoff between the space and code complexity to disable chaining in that case. Would again try to simplify code.

> 
> @@ +180,3 @@
> +            return row._casefoldedText.startsWith(this._key) &&
> row._casefoldedText.startsWith('/');
> +        }
> +        return row._casefoldedText.startsWith(this._key) &&
> !row._casefoldedText.startsWith('/');
> 
> I don't understand this code - if we are completing a command, this._key is
> something like '/j', no? So "row._casefoldedText.startsWith(this._key);"
> should do the right thing in either case.

Yup, but I did it for code readability. 

> 
> @@ +205,3 @@
>          this._startPos = text.lastIndexOf(' ') + 1;
>          this._key = text.toLowerCase().substr(this._startPos);
> +        this._isIRCCommand = false;
> 
> Or simply:
>   this._isIRCCommand = (this._startPos == 0 && this._key.startsWith('/');
> 
> Though I'd reconsider restricting command completions to the start of line -
> they don't actually work elsewhere, but they can still be mentioned:
>  'you can get a list of supported command by typing /h<tab>'

umm, that's a rare case about mentioning the commands in between chats. We can either support the usability case where the commands comes in starting, or for general case, where we provide auto-completion in between too. We would have to handle the case separately in that case too, as not to have ':' after completion, no chaining.

Created attachment 321354 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Review of attachment 321354 [details] [review]:

::: src/tabCompletion.js
@@ +40,2 @@
         this._widgetMap = {};
+        this._isLastIRCCompletion = false;

Bad name: "is last IRC completion" == "this is the last IRC completion, there will be no following completions".
_lastWasIRCCommandCompletion would be better, though I'd use "previous" instead of "last" for consistency with _previousCompletion and leave out superfluous bits from the name to make it more readable - my suggestion would be _previousWasCommand

@@ +41,3 @@
+        this._isLastIRCCompletion = false;
+
+        let IRCCommands = Object.keys(IrcParser.knownCommands);

Normal variables are camelCase, CamelCase is used for modules. Admittedly ircCommands looks a bit odd, but you could just use 'commands' - it's quite clear from the context what kind of commands are meant (not only because they come from a module called 'IrcParser', but also because polari is an IRC client ;-) )

@@ +166,3 @@
+            return row._text + ' ';
+        }
+        this._isLastIRCCompletion = false;

More concise as:

this._isLastIRCCompletion = this._isIRCCommand;
if (this._isIRCCommand)
    return row._text + ' ';
else if (...)

@@ +183,3 @@
+        if (this._isIRCCommand)
+            return row._casefoldedText.startsWith(this._key);
+        return row._casefoldedText.startsWith(this._key) && !row._casefoldedText.startsWith('/');

OK, so the reason you have two cases here is that you artificially restrict /command completion to the beginning of the line. You are right that it's not super common (except in combination with /SAY, where it's a primary use case), which can be a reason to not add complexity to support it - but here you *are* adding complexity to *not* support it, which doesn't make sense - remove the limitation on isIRCCommand, and you don't have to touch this code at all.

Created attachment 321391 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Created attachment 321405 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Created attachment 321406 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Created attachment 321408 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly IRC commands are inserted in the beginning, and
there is no chaining of commands.

Created attachment 321412 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Provide tab based auto-completion for IRC commands,
similar to what we do for nicks with little modifications,
mainly there is no chaining of commands and no ':' is inserted
after a command.

Comment on attachment 321412 [details] [review]
tabCompletion: Provide auto-completion for implemented IRC commands

Attachment 321412 [details] pushed as 68758ea - tabCompletion: Provide auto-completion for implemented IRC commands

Created attachment 321423 [details] [review]
tabCompletion: Don't chain for auto-completion of command

In commit 68758ea we added support for auto-completion
of IRC commands. We should not chain completion if the current
or the previous completion was of command.

Review of attachment 321423 [details] [review]:

Looks good, though I got a suggestion for a somewhat related cleanup, see below.

In the commit message:
 - "We should not chain completion" should be "We should not chain completions"
 - "was of command" => "was a command"

::: src/tabCompletion.js
@@ +212,3 @@
+        // In case current or the last completion was for IRC Command, don't chain.
+        let shouldChain = !this._isIRCCommand && !this._previousWasCommand;
+        this._previousCompletion = (this._endPos == this._startPos) && shouldChain;

I haven't tested it, but the code looks like it should work. There's something not quite related to the fix though: _previousCompletion has really become a misnomer now, as it may be false even when there's a preceding completion. This is probably a good opportunity to fix this by changing the above to:

  // Chain completions if the current completion directly follows a previous one,
  // except when one of them was for an IRC command
  let previousCompletion = (this._endPos == this._startPos);
  this._isChained = previousCompletion && !this._isIRCCommand && !this._previousWasCommand;

then change _previousCompletion to _isChained in the rest of the code.

Created attachment 321438 [details] [review]
tabCompletion: Don't chain for auto-completion of command

In commit 68758ea we added support for auto-completion
of IRC commands. We should not chain completions if the current
or the previous completion was a command. Also
_previousCompletion is a misnomer, use _isChained instead.

Created attachment 321439 [details] [review]
tabCompletion: Rename variable isIRCCommand to isCommand

It's quite clear from context, that by command, we mean
IRC command. Other variables in tabComplelion use Command
instead of IRCCommand, so rename the variable isIRCCommand
to isCommand.

Created attachment 321441 [details] [review]
tabCompletion: Rename variable isIRCCommand to isCommand

It's quite clear from context, that by command, we mean
IRC command. Other variables in tabCompletion use Command
instead of IRCCommand, so rename the variable isIRCCommand
to isCommand.

Review of attachment 321441 [details] [review]:

Sure

Review of attachment 321438 [details] [review]:

LGTM

Attachment 321438 [details] pushed as 4ee97c5 - tabCompletion: Don't chain for auto-completion of command
Attachment 321441 [details] pushed as cc93369 - tabCompletion: Rename variable isIRCCommand to isCommand