797016 - GCLI does not properly handle suggestions containing spaces

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Grant Gayed]

- observed in the Orion-1.0 branch

- in gcli's lib/gcli/ui/display.js file replace string 'gcli-css-display' with 'gcli-css- display' (just inserted a space)
- view the index.html in gcli's root directory
- type "edit c"
 -> the suggested completion should now be "css#gcli-css- display"
 -> press Tab, it's inserted, press Enter, seems OK

- press Up-arrow to repeat this last command
 -> it won't let you Enter it

- refresh the page
- type "edit c"
 -> the suggested completion should now be "css#gcli-css- display"
 -> press Tab, it's inserted
 -> press any key and a "Too many arguments" error is shown

Comment 1

[Grant Gayed]

I've taken the change in the Orion-1.0 branch for a spin and it seems to work reasonably well, thanks!  A couple of follow-up comments:

1. When passing a string that contains a space to a custom type's parse() implementation, would it be more correct to pass the string with the start/end quotes removed?  To me this seems more correct, because the presence of the quotes is an artifact of GCLI's argument grouping on the command line, but suggestion values provided by the type do not contain these quotes.  Our custom type has had to add lines to its #parse implementation to remove leading/trailing quotes before processing it, which presumably all custom types would need to do.

2. When auto-completing a value that has a space, the insertion caret ends up beyond the trailing quote mark, which is not ideal if the value within the quotes was not complete yet (need to backspace a couple of times in order to continue).  Perhaps if the prediction being inserted has "incomplete: true" then the caret should be placed within the trailing quote mark?

Comment 2

[Joe Walker [:jwalker]]

(In reply to Grant Gayed from comment #1)
> I've taken the change in the Orion-1.0 branch for a spin and it seems to
> work reasonably well, thanks!  A couple of follow-up comments:
> 
> 1. When passing a string that contains a space to a custom type's parse()
> implementation, would it be more correct to pass the string with the
> start/end quotes removed?  To me this seems more correct, because the
> presence of the quotes is an artifact of GCLI's argument grouping on the
> command line, but suggestion values provided by the type do not contain
> these quotes.  Our custom type has had to add lines to its #parse
> implementation to remove leading/trailing quotes before processing it, which
> presumably all custom types would need to do.

I don't think you should need to do any messing with quotes at all. The unit tests don't need to:
https://github.com/joewalker/gcli/blob/padscroll-791085/lib/gclitest/mockCommands.js#L448

> 2. When auto-completing a value that has a space, the insertion caret ends
> up beyond the trailing quote mark, which is not ideal if the value within
> the quotes was not complete yet (need to backspace a couple of times in
> order to continue).  Perhaps if the prediction being inserted has
> "incomplete: true" then the caret should be placed within the trailing quote
> mark?

Hmmm.
There isn't an option to selection type to allow non-suggested values (but there should be) Until there is this option, I'm not sure that it makes sense to be able to edit the completed values does it?

Comment 3

[Grant Gayed]

(In reply to Joe Walker [:jwalker] from comment #2)
> There isn't an option to selection type to allow non-suggested values (but
> there should be) Until there is this option, I'm not sure that it makes
> sense to be able to edit the completed values does it?

The case I have is a 'cd' command whose list of completions can grow in response to a completion being selected.  For example, given file structure:

currentDir
    |---bbb
         |---ccc

The initial set of completions is ["bbb"].  A user that chooses this completion can then either select it by pressing Enter, or can enter a subsequent path segment by adding '/' ("bbb/"), at which point the new set of completions is offered (in this case: ["bbb/ccc"]).  In the latter case the caret needs to be backspaced twice before typing the '/' that enables entering the next path segment.

Comment 4

[Joe Walker [:jwalker]]

(In reply to Grant Gayed from comment #3)
> (In reply to Joe Walker [:jwalker] from comment #2)
> > There isn't an option to selection type to allow non-suggested values (but
> > there should be) Until there is this option, I'm not sure that it makes
> > sense to be able to edit the completed values does it?
> 
> The case I have is a 'cd' command whose list of completions can grow in
> response to a completion being selected.  For example, given file structure:
> 
> currentDir
>     |---bbb
>          |---ccc
> 
> The initial set of completions is ["bbb"].  A user that chooses this
> completion can then either select it by pressing Enter, or can enter a
> subsequent path segment by adding '/' ("bbb/"), at which point the new set
> of completions is offered (in this case: ["bbb/ccc"]).  In the latter case
> the caret needs to be backspaced twice before typing the '/' that enables
> entering the next path segment.

See bug 803526.

Comment 5

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=6337e18d0024