Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Added better interaction for choice questions #11326

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@florianv
Copy link
Contributor

commented Jul 5, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This PR adds a more user friendly way to ask choice questions when stty is available.

Single choice question

single

You can move up and down with the arrow keys.
To validate your choice press enter.

Multi choice question

multi

You can move up and down with the arrow keys.
To select / deselect a choice press the spacebar.
To validate your choices press enter.

Todos:

  • Finish tests
  • Documentation
* @throws \InvalidArgumentException
* @throws \RuntimeException
*
* @api

This comment has been minimized.

Copy link
@cordoval

cordoval Jul 7, 2014

Contributor

i think putting @api on the class is enough. I mean I would assume it would have methods unshakable

This comment has been minimized.

Copy link
@stof

stof Jul 8, 2014

Member

Actually, @api shoulmd never be added in a PR submitting the code. you cannot make it part of the stable API if it is not even part of the codebase.
We will mark it as part of the stable API only a bit later

$result = null;
// Hide the cursor
$output->write("\033[?25l");

This comment has been minimized.

Copy link
@cordoval

cordoval Jul 7, 2014

Contributor

i wonder if we put these strings in a class on itself and instead of adding these comments we basically call the constant with meaningful names

This comment has been minimized.

Copy link
@florianv

florianv Jul 7, 2014

Author Contributor

Yes this is a good idea. That's what I did with the Keyboard class so it's also available to the developers.

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2014

I was thinking that this helper could replace the current one for choice questions only when stty is available. This could be done by renaming QuestionHelper into GenericQuestionHelper for example, and then creating a new QuestionHelper that just proxies the other two:

public function ask(InputInterface $input, OutputInterface $output, Question $question)
{
   if ($this->hasSttyAvailable() && $question instanceof ChoiceQuestion) {
       return $this->helperSet->get('choice_question')
           ->ask($input, $output, $question);
   }

   return $this->helperSet->get('generic_question')
       ->ask($input, $output, $question);
}
$output->write($text);
// Move the cursor left
$output->write("\033[10000D");

This comment has been minimized.

Copy link
@florianv

florianv Jul 7, 2014

Author Contributor

Note that here I pull the cursor left of 10 000 columns to avoid using mb_strlen (maybe less should be enough too).

@stof

This comment has been minimized.

Copy link
Member

commented Jul 8, 2014

IMO, it does not mak sense to have a separate helper for choice questions. It makes things harder to use for developers, by requiring to choose the right helper (or they will always use the question helper they already know, and users will not benefit from the better experience).
The enhanced experience should be implemented in the QuestionHelper directly (using the existing logic when you don't have stty)

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2014

I didn't put it in the QuestionHelper because I feared it becomes a mess with the different handlings (for example they write the questions and choices differently). On the other side the choice question handling is mostly done by the SingleChoiceQuestionAsker and the MultiChoiceQuestionAsker.

What I could do is to put the default handling from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Helper/QuestionHelper.php#L110 in a separate command like GenericQuestionAsker to keep the QuestionHelper clean. It will choose the appropriate Asker depending on the type of question and if stty is available and use that directly.

What do you think @stof ?

@florianv florianv changed the title [Console] Added a new helper for choice questions [Console] Added better interaction for choice questions Jul 11, 2014

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2014

I have updated the PR, so it keeps only the QuestionHelper and uses the new way when stty is available and it's a choice question. I have also completed the tests.

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2014

Btw, it would be nice if someone without stty could run the tests.

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2014

What do you think about this PR ? @fabpot @stof

@@ -24,8 +26,8 @@
*/
class QuestionHelper extends Helper
{
private $options = array();

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

I don't like these options. They are making the helper stateful, which is causing issues (it means asking a question in one place will have side effects on next questions in other places).

The configuration should be part of the Question object instead

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

Yes but I think when you want to configure the templates, you want to do it globally for all questions.

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

but here, your options are mutable. This means that some code using the helper can decide to change the options, and this will impact unrelated code (which was expecting to use the defaults).

If the goal is to configure them globally, they should not be mutable by code using the helper

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

Yes it's better to attach them to the Question as you propose. Thanks

$this->positionValueMap[$i] = $value;
$this->valuePositionMap[$value] = $i;
$i++;
}

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member
$this->positionValueMap = array_keys($choices);
$this->valuePositionMap = array_flip($this->positionValueMap);
*/
public function getChoiceTextAt($position)
{
$value = isset($this->positionValueMap[$position]) ? $this->positionValueMap[$position] : null;

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

you could reuse getChoiceValueAt here instead of duplicating it

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

Yes, nice catch

@eko

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2014

This is a great addition, a much better way to give answers, user-friendly 👍

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2014

👍

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2014

Thanks for reviewing @stof, I have addressed your comments

*
* @author Florian Voutzinos <florian@voutzinos.com>
*/
interface EscapeSequence

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

I'm not sure these utility classes/Interface should actually be in the root of the component (I don't have a good idea for the naming of a subnamespace holding them though).

also, Symfony uses a final class with a private constructor for enums, not an interface (it would not make sense for someone to implement this interface)

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

Maybe IOUtil or something like that ?

protected $question;
protected $choicesMap;
public function __construct(ChoiceQuestion $question)

This comment has been minimized.

Copy link
@ProPheT777

ProPheT777 Jul 17, 2014

Contributor

Missing phpdocs

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

IMO it's not needed

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 17, 2014

Member

Correct, not needed as there is a type hint on the argument.

$output->write(sprintf(EscapeSequences::CURSOR_MOVE_BACKWARD_N, 10000));
}
private function doAsk(OutputInterface $output, ChoicesCursor $cursor, $inputStream)

This comment has been minimized.

Copy link
@ProPheT777

ProPheT777 Jul 17, 2014

Contributor

Same here

This comment has been minimized.

Copy link
@florianv

florianv Jul 17, 2014

Author Contributor

Afaik private methods are not documented unless there is ambiguity (but here the argument names are quite descriptive already)

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 17, 2014

Member

correct, not needed

@ProPheT777

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2014

👍

@florianv

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2014

Any new about this PR @fabpot @stof ?

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017

@chalasr chalasr self-requested a review Feb 5, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

@ro0NL Probably one that you could be interested in taking over :)

@vudaltsov

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I guess, this PR could be closed after #26698.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.