[Console] Add autocomplete as you type #6391

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
@lmcd
Contributor

lmcd commented Dec 17, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
License of the code: MIT

Finally got around to reviving the console autocomplete code.
Is now up to date with Symfony master. Also changed backspace behaviour to remove one character instead of two.

stty stuff is a mystery to a lot of people, so I've commented verbosely.

See also: #2364

*
* @return string The user answer
*
* @throws \RuntimeException If there is no data to read in the input stream
*/
- public function ask(OutputInterface $output, $question, $default = null)
+ public function ask(OutputInterface $output, $question, $default = null, $autocomplete = null)

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

If it can only be an array or null, please add the array typehint

@stof

stof Dec 17, 2012

Member

If it can only be an array or null, please add the array typehint

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Dec 17, 2012

Owner

I don't think that using array as a type hint is required everywhere. It forbids the possibility to pass an ArrayAccess object for instance (which sometimes might works).

@fabpot

fabpot Dec 17, 2012

Owner

I don't think that using array as a type hint is required everywhere. It forbids the possibility to pass an ArrayAccess object for instance (which sometimes might works).

This comment has been minimized.

Show comment Hide comment
@schmittjoh

schmittjoh Dec 17, 2012

Contributor

How about changing the PHP doc, so that people can rely on the fact that ArrayAccess is also supported?

@schmittjoh

schmittjoh Dec 17, 2012

Contributor

How about changing the PHP doc, so that people can rely on the fact that ArrayAccess is also supported?

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

So shall I typehint?

@lmcd

lmcd Dec 17, 2012

Contributor

So shall I typehint?

This comment has been minimized.

Show comment Hide comment
@Tobion

Tobion Dec 17, 2012

Member

I'm in favor of adding the typehint and not support ArrayAccess everywhere where it could potentially be used. Let's go with the more restrictive behavior first and soften it when required. Same reason as using private visibility over protected.

@Tobion

Tobion Dec 17, 2012

Member

I'm in favor of adding the typehint and not support ArrayAccess everywhere where it could potentially be used. Let's go with the more restrictive behavior first and soften it when required. Same reason as using private visibility over protected.

+ if (3 === strlen($c) && "\033" === $c[0]) {
+ // Escape sequences for arrow keys
+ if ('A' === $c[2] || 'B' === $c[2] || 'C' === $c[2] || 'D' === $c[2]) {
+ continue;

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

This looks useless to me as you are continuing anyway here

@stof

stof Dec 17, 2012

Member

This looks useless to me as you are continuing anyway here

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

I did have plans for the arrow keys, which I have left out for now to keep it simple (cycle through options with up/down arrows).

@lmcd

lmcd Dec 17, 2012

Contributor

I did have plans for the arrow keys, which I have left out for now to keep it simple (cycle through options with up/down arrows).

+ continue;
+ }
+
+ $c = $c[0];

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

wouldn't you loose the next chars here ?

@stof

stof Dec 17, 2012

Member

wouldn't you loose the next chars here ?

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

Once we're done with escape sequences, we only deal with single characters from this point forward.

@lmcd

lmcd Dec 17, 2012

Contributor

Once we're done with escape sequences, we only deal with single characters from this point forward.

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

but will the other 2 chars be read again by fread on the next loop ?

@stof

stof Dec 17, 2012

Member

but will the other 2 chars be read again by fread on the next loop ?

*
* @return mixed
*
* @throws \Exception When any of the validators return an error
*/
- public function askAndValidate(OutputInterface $output, $question, $validator, $attempts = false, $default = null)
+ public function askAndValidate(OutputInterface $output, $question, $validator, $attempts = false, $default = null, $autocomplete = null)

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

same here for the typehint

@stof

stof Dec 17, 2012

Member

same here for the typehint

+ $this->assertEquals('AsseticBundleTest', $dialog->ask($this->getOutputStream(), 'Please select a bundle', 'FrameworkBundle', $bundles));
+ $this->assertEquals('FrameworkBundle', $dialog->ask($this->getOutputStream(), 'Please select a bundle', 'FrameworkBundle', $bundles));
+ }
+ else {

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

This should be on the previous line

@stof

stof Dec 17, 2012

Member

This should be on the previous line

+ continue;
+ }
+
+ if (ord($c[0]) < 32) {

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 17, 2012

Member

$c[0] looks useless here as there is already a single char

@stof

stof Dec 17, 2012

Member

$c[0] looks useless here as there is already a single char

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

@stof - updated with a better solution

Contributor

lmcd commented Dec 17, 2012

@stof - updated with a better solution

@Seldaek

This comment has been minimized.

Show comment Hide comment
@Seldaek

Seldaek Dec 17, 2012

Member

Seems pretty cool, but could you replace all /usr/bin/env stty calls by simply stty? That way it would also work on windows - if you have mingw or cygwin installed and stty is in the path at least. I don't see the benefit of doing the /usr/bin/env trick here. That's good for shebang lines because you need an absolute path, but in an exec/shell_exec call, you can rely on the PATH and just type the command name.

Member

Seldaek commented Dec 17, 2012

Seems pretty cool, but could you replace all /usr/bin/env stty calls by simply stty? That way it would also work on windows - if you have mingw or cygwin installed and stty is in the path at least. I don't see the benefit of doing the /usr/bin/env trick here. That's good for shebang lines because you need an absolute path, but in an exec/shell_exec call, you can rely on the PATH and just type the command name.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

@Seldaek makes sense. Changed.

Contributor

lmcd commented Dec 17, 2012

@Seldaek makes sense. Changed.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 17, 2012

Contributor

Tested on Mac OS X 10.8 and Ubuntu 12.04. Would be great to hear from people on Windows, cygwin and those with exotic terminal setups.

I'll update my fork of SensioGeneratorBundle a little later with support for this.

@fabpot - is there still time for this to land in 2.2?

Contributor

lmcd commented Dec 17, 2012

Tested on Mac OS X 10.8 and Ubuntu 12.04. Would be great to hear from people on Windows, cygwin and those with exotic terminal setups.

I'll update my fork of SensioGeneratorBundle a little later with support for this.

@fabpot - is there still time for this to land in 2.2?

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Dec 17, 2012

Owner

If we have good feedback from Windows users, yes it can land in 2.2. ping @pborreli

Owner

fabpot commented Dec 17, 2012

If we have good feedback from Windows users, yes it can land in 2.2. ping @pborreli

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 17, 2012

Contributor

A am about to try on windows 7 with cmd, powershell and cygwin. Any other way to test without writing a new command using the helper ?

Contributor

michelsalib commented Dec 17, 2012

A am about to try on windows 7 with cmd, powershell and cygwin. Any other way to test without writing a new command using the helper ?

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

I tried on Windows 7 with cmd, powershell and cygwin and got this error: Le chemin d'accès spécifié est introuvable.. You can translate it to The specified path could not be found.

Contributor

michelsalib commented Dec 18, 2012

I tried on Windows 7 with cmd, powershell and cygwin and got this error: Le chemin d'accès spécifié est introuvable.. You can translate it to The specified path could not be found.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor

I've updated SensioGeneratorBundle to support autocompletion on the generate:doctrine:entity command. It autocompletes bundle names, configuration formats and field types. See here: lmcd/SensioGeneratorBundle@c627c67

@michelsalib ping

Contributor

lmcd commented Dec 18, 2012

I've updated SensioGeneratorBundle to support autocompletion on the generate:doctrine:entity command. It autocompletes bundle names, configuration formats and field types. See here: lmcd/SensioGeneratorBundle@c627c67

@michelsalib ping

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor

@michelsalib - hmm. I imagine it's either a problem locating stty or some configuration issue on your end. Do you have file/line number?

Contributor

lmcd commented Dec 18, 2012

@michelsalib - hmm. I imagine it's either a problem locating stty or some configuration issue on your end. Do you have file/line number?

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

Verbose mode did not help. Let me try with some dirty line by line check to see if I can give you a line.

Contributor

michelsalib commented Dec 18, 2012

Verbose mode did not help. Let me try with some dirty line by line check to see if I can give you a line.

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

My bad, I should have guessed that line 144 exec('/usr/bin/env stty', $output, $exitcode); cannot work on regular Windows environment. This should at least fails silently for users using cmd or powershell. Apparently cygwin users can activate stty. Let me investigate.

Contributor

michelsalib commented Dec 18, 2012

My bad, I should have guessed that line 144 exec('/usr/bin/env stty', $output, $exitcode); cannot work on regular Windows environment. This should at least fails silently for users using cmd or powershell. Apparently cygwin users can activate stty. Let me investigate.

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

Ok, cygwin comes pre-bundled with stty. I applied the fix recommended by @Seldaek and it fixed the cygwin command.
The only remaining problem is that your redirect the output of the exec call to the console, in this case cmd and powershell output the error telling that stty is not defined in the system: 'stty' n'est pas reconnu en tant que commande interne ou externe, un programme exécutable ou un fichier de commandes..

Contributor

michelsalib commented Dec 18, 2012

Ok, cygwin comes pre-bundled with stty. I applied the fix recommended by @Seldaek and it fixed the cygwin command.
The only remaining problem is that your redirect the output of the exec call to the console, in this case cmd and powershell output the error telling that stty is not defined in the system: 'stty' n'est pas reconnu en tant que commande interne ou externe, un programme exécutable ou un fichier de commandes..

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor

Ah, I see you're running the unit tests. The hasSttyAvailable method was lifted from DialogHelper where it is also used in askHiddenResponse. Question: is defined('PHP_WINDOWS_VERSION_BUILD') true for cygwin?

Contributor

lmcd commented Dec 18, 2012

Ah, I see you're running the unit tests. The hasSttyAvailable method was lifted from DialogHelper where it is also used in askHiddenResponse. Question: is defined('PHP_WINDOWS_VERSION_BUILD') true for cygwin?

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

I am not running test, I am actually running a homemade command:

$dialog = $this->getHelper('dialog');

$ask = $dialog->ask($output, 'Autocomplete example', null, array(
    'French', 'English', 'Chineese',
));

$output->writeln($ask);

hasSttyAvailable is called in the ask function at line 80. The incriminated function is here : https://github.com/lmcd/symfony/blob/9ebcd4bac97551d1b6e68d194c7109f5dcdf364a/src/Symfony/Component/Console/Helper/DialogHelper.php#L411.

Also defined('PHP_WINDOWS_VERSION_BUILD') is true in the three of my consoles.

Contributor

michelsalib commented Dec 18, 2012

I am not running test, I am actually running a homemade command:

$dialog = $this->getHelper('dialog');

$ask = $dialog->ask($output, 'Autocomplete example', null, array(
    'French', 'English', 'Chineese',
));

$output->writeln($ask);

hasSttyAvailable is called in the ask function at line 80. The incriminated function is here : https://github.com/lmcd/symfony/blob/9ebcd4bac97551d1b6e68d194c7109f5dcdf364a/src/Symfony/Component/Console/Helper/DialogHelper.php#L411.

Also defined('PHP_WINDOWS_VERSION_BUILD') is true in the three of my consoles.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor
@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

Why keeping /usr/bin/env in your calls ? Cygwin cannot interpret it as long a stty is not in this folder.

Contributor

michelsalib commented Dec 18, 2012

Why keeping /usr/bin/env in your calls ? Cygwin cannot interpret it as long a stty is not in this folder.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor

@michelsalib /usr/bin/env was put there by someone else for the askHiddenResponse method. I can remove it, so long as it doesn't break something else.

Contributor

lmcd commented Dec 18, 2012

@michelsalib /usr/bin/env was put there by someone else for the askHiddenResponse method. I can remove it, so long as it doesn't break something else.

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 18, 2012

Contributor

IMO users who want's to use stty should have it configured in the PATH, as @Seldaek said. Prefixing the folder where the stty "should" be is a nonsense to me. Moreover it might work well on *nix systems, but will never be compatible with cygwin.
I tested it locally (without the /usr/bin/env prefix) and now I have a nice autocomplete on Cygwin, and nothing on cmd or powershell. Which is just what I expected.

@lmcd very nice work :)

Contributor

michelsalib commented Dec 18, 2012

IMO users who want's to use stty should have it configured in the PATH, as @Seldaek said. Prefixing the folder where the stty "should" be is a nonsense to me. Moreover it might work well on *nix systems, but will never be compatible with cygwin.
I tested it locally (without the /usr/bin/env prefix) and now I have a nice autocomplete on Cygwin, and nothing on cmd or powershell. Which is just what I expected.

@lmcd very nice work :)

@lmcd lmcd referenced this pull request in sensiolabs/SensioGeneratorBundle Dec 18, 2012

Merged

Add autocompletion to generate:doctrine:entity #167

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 18, 2012

Contributor

For anyone interested, you can scroll through available autocomplete options that match typed characters by using up and down arrow keys. This has been implemented in a seperate branch here: https://github.com/lmcd/symfony/tree/autocomplete-arrows

Contributor

lmcd commented Dec 18, 2012

For anyone interested, you can scroll through available autocomplete options that match typed characters by using up and down arrow keys. This has been implemented in a seperate branch here: https://github.com/lmcd/symfony/tree/autocomplete-arrows

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Dec 18, 2012

@lmcd - The console PRs never cease to amaze me. Really well done!

ghost commented Dec 18, 2012

@lmcd - The console PRs never cease to amaze me. Really well done!

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Dec 19, 2012

Owner

@lmcd Is it mergeable now?

Owner

fabpot commented Dec 19, 2012

@lmcd Is it mergeable now?

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 19, 2012

Contributor

@fabpot Yes.

Contributor

lmcd commented Dec 19, 2012

@fabpot Yes.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 19, 2012

Contributor

Edit: commits squashed

Contributor

lmcd commented Dec 19, 2012

Edit: commits squashed

@stloyd

View changes

src/Symfony/Component/Console/Tests/Helper/DialogHelperTest.php
+ } else {
+ $this->markTestSkipped();
+ $this->markTestSkipped();
+ $this->markTestSkipped();

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 19, 2012

Contributor

Two of those should be removed.

@stloyd

stloyd Dec 19, 2012

Contributor

Two of those should be removed.

@stloyd

View changes

src/Symfony/Component/Console/Helper/DialogHelper.php
+
+ foreach ($autocomplete as $j => $value) {
+ // Get a substring of the current autocomplete item based on number of chars typed (e.g. AcmeDemoBundle = Acme)
+ $matchTest = substr($value, 0, strlen($ret));

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 19, 2012

Contributor

Move strlen($ret) out of loop.

@stloyd

stloyd Dec 19, 2012

Contributor

Move strlen($ret) out of loop.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 19, 2012

Contributor

@stloyd I have addressed the two things mentioned. I'm now using $i in place of strlen($ret) as it held the same value.

Contributor

lmcd commented Dec 19, 2012

@stloyd I have addressed the two things mentioned. I'm now using $i in place of strlen($ret) as it held the same value.

Add autocomplete as you type to the console
Use OutputFormatterStyle for highlighted text

Use shell_exec instead of system

Use octal notation for backspace character (consistency)

Fix detection of arrow keys. May have produced error if strlen($c) was 2 instead of 3

Fix ignored backspace after tabbing

Prevent some ctrl sequences from printing invisible characters and screwing things up

Check for ctrl sequences and other bad characters AFTER testing for tabs/newlines

Change for loop to foreach

Yoda conditions for consistency

Add tests

Update tests

CS

Read one character per keystroke instead of three (except when escape sequence is detected)

Don't use env to launch stty

Array type hinting and comment amends

Supress errors when stty is not found

Fix stty detection for cygwin

Fix stty detection for cygwin (DialogHelperTest)

Use $i in place of strlen($ret)

Only call markTestSkipped once
@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Dec 20, 2012

Owner

@lmcd: Thanks a lot for finishing this in time for 2.2. Before I can merge, there are two remaining tasks:

  • add a note about the new feature in the CHANGELOG file of the Console component;
  • create a PR on symfony/symfony-docs to explain the new feature.

Can you take care of that?

Owner

fabpot commented Dec 20, 2012

@lmcd: Thanks a lot for finishing this in time for 2.2. Before I can merge, there are two remaining tasks:

  • add a note about the new feature in the CHANGELOG file of the Console component;
  • create a PR on symfony/symfony-docs to explain the new feature.

Can you take care of that?

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 20, 2012

Contributor

@fabpot sure

Contributor

lmcd commented Dec 20, 2012

@fabpot sure

@lmcd lmcd referenced this pull request in symfony/symfony-docs Dec 24, 2012

Merged

[Waiting Code Merge] Add note about console autocompletion #2055

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 29, 2012

Contributor

@lmcd You should squash your "merge" commit.

Contributor

stloyd commented Dec 29, 2012

@lmcd You should squash your "merge" commit.

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Dec 29, 2012

Contributor

Well that screwed up. File Changed: 216 :S
Edit: hard reset to an earlier hash and reapplied the CHANGELOG commit. Should be good now ping @stloyd

Contributor

lmcd commented Dec 29, 2012

Well that screwed up. File Changed: 216 :S
Edit: hard reset to an earlier hash and reapplied the CHANGELOG commit. Should be good now ping @stloyd

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 29, 2012

Contributor

@lmcd ask() method is quite long now, but in overall looks ok =)

Contributor

stloyd commented Dec 29, 2012

@lmcd ask() method is quite long now, but in overall looks ok =)

@lmcd

This comment has been minimized.

Show comment Hide comment
@lmcd

lmcd Jan 2, 2013

Contributor

Anything preventing this being merged? Ping @fabpot

Contributor

lmcd commented Jan 2, 2013

Anything preventing this being merged? Ping @fabpot

@fabpot fabpot closed this in 7863f88 Jan 3, 2013

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Jan 3, 2013

Owner

@lmcd: merged, thanks a lot!

Owner

fabpot commented Jan 3, 2013

@lmcd: merged, thanks a lot!

fabpot added a commit that referenced this pull request Jan 6, 2013

merged branch lmcd/autocomplete-arrows-new (PR #6564)
This PR was merged into the master branch.

Commits
-------

2b73975 Add new tests and fix problem with the second option being chosen on down arrow
8a0bcfb Use strpos in place of substr
9864d95 Not much use in blanking the array each time
a4d711c Enable arrow keys to browse matched autocomplete options

Discussion
----------

[Console] Enable arrow keys to browse matched autocomplete options

See notes in original autocomplete pull request: #6391
See also @bamarni's pull request implementing more or less the same stuff: #6561

---------------------------------------------------------------------------

by fabpot at 2013-01-05T10:12:47Z

Looks good to me. Can you add some unit tests?

---------------------------------------------------------------------------

by bamarni at 2013-01-05T12:51:51Z

1 line addition... you definitely got me on the diff!

That's what I had mind too, excepted for the default highlight, but as you said it's usually displayed in the question so it 's not necessary. 👍

---------------------------------------------------------------------------

by lmcd at 2013-01-06T04:07:02Z

@fabpot Added tests

weaverryan pushed a commit to weaverryan/SensioGeneratorBundle that referenced this pull request Oct 2, 2014

merged branch lmcd/autocomplete (PR #167)
This PR was merged into the master branch.

Commits
-------

c627c67 Add autocompletion to generate:doctrine:entity

Discussion
----------

Add autocompletion to generate:doctrine:entity

See also: symfony/symfony#6391

---------------------------------------------------------------------------

by lmcd at 2013-01-06T02:42:25Z

Autocompletion has landed now in symfony, so this PR is now relevant. Ping @fabpot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment