Rewrite #1

AydinHassan · 2018-04-18T16:41:55Z

@mikeymike I will annotate this PR in a few hours so maybe we can go through it together at some point after that.

AydinHassan · 2018-04-18T18:49:14Z

src/InputCharacter.php

+/**
+ * @author Aydin Hassan <[email protected]>
+ */
+class InputCharacter


So this represents a single character or control character read from the input source. It contains a bunch of helper methods for dealing with control sequences and validation of them.

AydinHassan · 2018-04-18T18:56:37Z

src/Terminal.php

 */
-class Terminal implements TerminalInterface
+interface Terminal


So I've rename TerminalInterface to Terminal and Terminal to UnixTerminal. I know we only have 1 terminal but we might add a windows one in the future if the terminal gets a bit better. What do you think?

Seems logical, Windows afaik are working more towards unix style / vt100 compatable terminal with the Linux subsystem stuff anyway but I'm not sure if it replaces the standard cmd shit.

AydinHassan · 2018-04-18T18:57:41Z

src/Terminal.php

+     *
+     * @return bool
+     */
+    public function isInteractive() : bool;


This replaces isATTY - I think it's a bit nicer and more explanatory

AydinHassan · 2018-04-18T18:59:04Z

src/Terminal.php


-    public function getDetails(): string


I've dropped this completely. Basically this got the name of the terminal. But it was only used when the terminal was not a TTY to throw an exception. But if it's not a TTY - then it doesn't really have a name because it's either a pipe or a redirect. So I don't think we need this.

yeah makes sense

AydinHassan · 2018-04-18T19:00:06Z

src/Terminal.php


-    public function setCanonicalMode(bool $useCanonicalMode = true)


I've swapped this for disableCanonicalMode & enableCanonicalMode

AydinHassan · 2018-04-18T19:03:57Z

src/TerminalReader.php

+/**
+ * @author Aydin Hassan <[email protected]>
+ */
+class TerminalReader


So this new class takes a Terminal object and allows to read input and maps to InputCharacters - it also allows to map other characters to control code. So cli-menu code will look like this:

$reader = new TerminalReader($this->terminal); $reader->addControlMappings([ '^P' => InputCharacter::UP, 'k' => InputCharacter::UP, '^K' => InputCharacter::DOWN, 'j' => InputCharacter::DOWN, "\r" => InputCharacter::ENTER, ' ' => InputCharacter::ENTER, ]); while ($this->isOpen() && $char = $reader->readCharacter()) { if ($char->isNotControl()) { continue; } switch ($char->getControl()) { case InputCharacter::UP: case InputCharacter::DOWN: $this->moveSelection($char->getControl()); $this->draw(); break; case InputCharacter::ENTER: $this->executeCurrentItem(); break; } }

What do you think?

AydinHassan · 2018-04-18T19:05:03Z

src/UnixTerminal.php

+ * @author Michael Woodward <[email protected]>
+ * @author Aydin Hassan <[email protected]>
+ */
+class UnixTerminal implements Terminal


This should hopefully be self explanatory

…te in non-canonical mode will read only half a control sequence.

…al mode and empty the buffer altogether. It should either be a control sequence or a character.

codecov-io · 2018-04-18T20:14:59Z

Codecov Report

❗ No coverage uploaded for pull request base (master@12a0a3d). Click here to learn what that means.
The diff coverage is 80.92%.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   80.92%           
  Complexity        ?       80           
=========================================
  Files             ?        7           
  Lines             ?      152           
  Branches          ?        0           
=========================================
  Hits              ?      123           
  Misses            ?       29           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
src/Exception/NotInteractiveTerminal.php	`100% <100%> (ø)`	`2 <2> (?)`
src/InputCharacter.php	`100% <100%> (ø)`	`11 <11> (?)`
src/UnixTerminal.php	`69.86% <69.86%> (ø)`	`39 <39> (?)`
src/IO/BufferedOutput.php	`81.81% <81.81%> (ø)`	`5 <5> (?)`
src/IO/ResourceOutputStream.php	`81.81% <81.81%> (ø)`	`7 <7> (?)`
src/IO/ResourceInputStream.php	`83.33% <83.33%> (ø)`	`7 <7> (?)`
src/NonCanonicalReader.php	`94.73% <94.73%> (ø)`	`9 <9> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12a0a3d...90e8de2. Read the comment docs.

AydinHassan · 2018-04-18T20:15:56Z

src/Terminal.php


-    public function setCanonicalMode(bool $useCanonicalMode = true)


I've swapped this for disableCanonicalMode & enableCanonicalMode

AydinHassan · 2018-04-18T20:17:52Z

src/NonCanonicalReader.php

+ *
+ * @author Aydin Hassan <[email protected]>
+ */
+class NonCanonicalReader


So this new class takes a Terminal object and allows to read input and maps to InputCharacters - it also allows to map other characters to control code. So cli-menu code will look like this:

$reader = new NonCanonicalReader($this->terminal); $reader->addControlMappings([ '^P' => InputCharacter::UP, 'k' => InputCharacter::UP, '^K' => InputCharacter::DOWN, 'j' => InputCharacter::DOWN, "\r" => InputCharacter::ENTER, ' ' => InputCharacter::ENTER, ]); while ($this->isOpen() && $char = $reader->readCharacter()) { if ($char->isNotControl()) { continue; } switch ($char->getControl()) { case InputCharacter::UP: case InputCharacter::DOWN: $this->moveSelection($char->getControl()); $this->draw(); break; case InputCharacter::ENTER: $this->executeCurrentItem(); break; } }

It enables and disables canonical mode on the fly and empties the buffer on each read (well reads 4 bytes but there should never be more in there)

What do you think?

mikeymike · 2018-04-21T20:52:07Z

src/InputCharacter.php

+
+namespace PhpSchool\Terminal;
+
+use function in_array;


this feels wierd, is it needed? 🤔

Not really - the EA inspections plugin for PHP storm recommends it for optimisation - I can remove if you want?

I don't mind it being there, just didn't understand why it was

mikeymike · 2018-04-21T21:05:58Z

src/Terminal.php

 */
-class Terminal implements TerminalInterface
+interface Terminal


Seems logical, Windows afaik are working more towards unix style / vt100 compatable terminal with the Linux subsystem stuff anyway but I'm not sure if it replaces the standard cmd shit.

mikeymike · 2018-04-21T21:06:49Z

src/Terminal.php


-    public function getDetails(): string


yeah makes sense

Rewrite

3fe1292

AydinHassan requested a review from mikeymike April 18, 2018 16:41

AydinHassan commented Apr 18, 2018

View reviewed changes

Docs for InputCharacter

ab40182

AydinHassan commented Apr 18, 2018

View reviewed changes

AydinHassan added 2 commits April 18, 2018 21:05

Remove debug code

6796414

Add travis

d72f6ad

php-school deleted a comment from codecov-io Apr 18, 2018

AydinHassan added 3 commits April 18, 2018 22:11

Don't exec if we don't need to

d78d6c6

Remove readCharacter because it doesn't do what we want. Reading 1 by…

7e47fbd

…te in non-canonical mode will read only half a control sequence.

Rename TerminalReader to NonCanonicalReader as we now disable canonic…

141afd2

…al mode and empty the buffer altogether. It should either be a control sequence or a character.

AydinHassan commented Apr 18, 2018

View reviewed changes

AydinHassan added 3 commits April 19, 2018 10:26

Parse original canonical mode

0db57e0

Remove unused property

8db49d2

Add restoreOriginalConfiguration to interface

0c4de0b

AydinHassan mentioned this pull request Apr 19, 2018

Refactor IO php-school/cli-menu#80

Closed

Remove debug

90e8de2

mikeymike reviewed Apr 21, 2018

View reviewed changes

mikeymike merged commit c3a87b9 into master Apr 22, 2018

mikeymike deleted the rewrite branch April 22, 2018 14:16


		public function setCanonicalMode(bool $useCanonicalMode = true)

Rewrite #1

Rewrite #1

Uh oh!

Conversation

AydinHassan commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AydinHassan commented Apr 18, 2018 •

edited

Loading

codecov-io commented Apr 18, 2018 •

edited

Loading