From eacf930cc9bdd52589e2a0d27af6abd892bc473f Mon Sep 17 00:00:00 2001 From: Aydin Hassan Date: Tue, 15 May 2018 12:34:16 +0200 Subject: [PATCH 1/2] Simplify builder code --- src/Builder/BuilderUtils.php | 2 +- src/Builder/CliMenuBuilder.php | 135 +++++++--------------------- src/Builder/SplitItemBuilder.php | 7 +- src/MenuStyle.php | 36 +++++--- test/Builder/CliMenuBuilderTest.php | 74 +++++++-------- test/MenuStyleTest.php | 19 +--- 6 files changed, 100 insertions(+), 173 deletions(-) diff --git a/src/Builder/BuilderUtils.php b/src/Builder/BuilderUtils.php index c807ad7c..929d3772 100644 --- a/src/Builder/BuilderUtils.php +++ b/src/Builder/BuilderUtils.php @@ -71,7 +71,7 @@ public function addSubMenu(string $id, string $text) : CliMenuBuilder 'id' => $id ]; - $this->subMenuBuilders[$id] = new CliMenuBuilder($this); + $this->subMenuBuilders[$id] = CliMenuBuilder::newFromParent($this); return $this->subMenuBuilders[$id]; } diff --git a/src/Builder/CliMenuBuilder.php b/src/Builder/CliMenuBuilder.php index ca5e9392..246bd4ed 100644 --- a/src/Builder/CliMenuBuilder.php +++ b/src/Builder/CliMenuBuilder.php @@ -11,7 +11,6 @@ use PhpSchool\CliMenu\CliMenu; use PhpSchool\CliMenu\MenuStyle; use PhpSchool\CliMenu\Terminal\TerminalFactory; -use PhpSchool\CliMenu\Util\ColourUtil; use PhpSchool\Terminal\Terminal; use RuntimeException; @@ -54,7 +53,7 @@ class CliMenuBuilder implements Builder private $exitButtonText = 'Exit'; /** - * @var array + * @var MenuStyle */ private $style; @@ -78,13 +77,16 @@ class CliMenuBuilder implements Builder */ private $disabled = false; - public function __construct(Builder $parent = null) + public function __construct(Terminal $terminal = null, Builder $parent = null) { + $this->terminal = $terminal ?? TerminalFactory::fromSystem(); $this->parent = $parent; - $this->terminal = $this->parent !== null - ? $this->parent->getTerminal() - : TerminalFactory::fromSystem(); - $this->style = MenuStyle::getDefaultStyleValues(); + $this->style = new MenuStyle($this->terminal); + } + + public static function newFromParent(Builder $parent) : self + { + return new self($parent->getTerminal(), $parent); } public function setTitle(string $title) : self @@ -170,174 +172,127 @@ public function setExitButtonText(string $exitButtonText) : self public function setBackgroundColour(string $colour, string $fallback = null) : self { - $this->style['bg'] = ColourUtil::validateColour( - $this->terminal, - $colour, - $fallback - ); + $this->style->setBg($colour, $fallback); return $this; } public function setForegroundColour(string $colour, string $fallback = null) : self { - $this->style['fg'] = ColourUtil::validateColour( - $this->terminal, - $colour, - $fallback - ); + $this->style->setFg($colour, $fallback); return $this; } public function setWidth(int $width) : self { - $this->style['width'] = $width; + $this->style->setWidth($width); return $this; } public function setPadding(int $topBottom, int $leftRight = null) : self { - if ($leftRight === null) { - $leftRight = $topBottom; - } - - $this->setPaddingTopBottom($topBottom); - $this->setPaddingLeftRight($leftRight); + $this->style->setPadding($topBottom, $leftRight); return $this; } public function setPaddingTopBottom(int $topBottom) : self { - $this->style['paddingTopBottom'] = $topBottom; + $this->style->setPaddingTopBottom($topBottom); return $this; } public function setPaddingLeftRight(int $leftRight) : self { - $this->style['paddingLeftRight'] = $leftRight; + $this->style->setPaddingLeftRight($leftRight); return $this; } public function setMarginAuto() : self { - $this->style['marginAuto'] = true; + $this->style->setMarginAuto(); return $this; } public function setMargin(int $margin) : self { - $this->style['marginAuto'] = false; - $this->style['margin'] = $margin; + $this->style->setMargin($margin); return $this; } public function setUnselectedMarker(string $marker) : self { - $this->style['unselectedMarker'] = $marker; + $this->style->setUnselectedMarker($marker); return $this; } public function setSelectedMarker(string $marker) : self { - $this->style['selectedMarker'] = $marker; + $this->style->setSelectedMarker($marker); return $this; } public function setItemExtra(string $extra) : self { - $this->style['itemExtra'] = $extra; + $this->style->setItemExtra($extra); return $this; } public function setTitleSeparator(string $separator) : self { - $this->style['titleSeparator'] = $separator; + $this->style->setTitleSeparator($separator); return $this; } - public function setBorder( - int $topWidth, - $rightWidth = null, - $bottomWidth = null, - $leftWidth = null, - string $colour = null - ) : self { - if (!is_int($rightWidth)) { - $colour = $rightWidth; - $rightWidth = $bottomWidth = $leftWidth = $topWidth; - } elseif (!is_int($bottomWidth)) { - $colour = $bottomWidth; - $bottomWidth = $topWidth; - $leftWidth = $rightWidth; - } elseif (!is_int($leftWidth)) { - $colour = $leftWidth; - $leftWidth = $rightWidth; - } - - $this->style['borderTopWidth'] = $topWidth; - $this->style['borderRightWidth'] = $rightWidth; - $this->style['borderBottomWidth'] = $bottomWidth; - $this->style['borderLeftWidth'] = $leftWidth; - - if (is_string($colour)) { - $this->style['borderColour'] = $colour; - } elseif ($colour !== null) { - throw new \InvalidArgumentException('Invalid colour'); - } + public function setBorder(int $top, $right = null, $bottom = null, $left = null, string $colour = null) : self + { + $this->style->setBorder($top, $right, $bottom, $left, $colour); return $this; } public function setBorderTopWidth(int $width) : self { - $this->style['borderTopWidth'] = $width; + $this->style->setBorderTopWidth($width); return $this; } public function setBorderRightWidth(int $width) : self { - $this->style['borderRightWidth'] = $width; + $this->style->setBorderRightWidth($width); return $this; } public function setBorderBottomWidth(int $width) : self { - $this->style['borderBottomWidth'] = $width; + $this->style->setBorderBottomWidth($width); return $this; } public function setBorderLeftWidth(int $width) : self { - $this->style['borderLeftWidth'] = $width; + $this->style->setBorderLeftWidth($width); return $this; } public function setBorderColour(string $colour, $fallback = null) : self { - $this->style['borderColour'] = $colour; - $this->style['borderColourFallback'] = $fallback; - - return $this; - } + $this->style->setBorderColour($colour, $fallback); - public function setTerminal(Terminal $terminal) : self - { - $this->terminal = $terminal; return $this; } @@ -378,40 +333,16 @@ private function itemsHaveExtra(array $items) : bool public function getMenuStyle() : MenuStyle { if (null === $this->parent) { - return $this->buildStyle(); + return $this->style; } - if ($this->style !== MenuStyle::getDefaultStyleValues()) { - return $this->buildStyle(); + if ($this->style->hasChangedFromDefaults()) { + return $this->style; } return $this->parent->getMenuStyle(); } - private function buildStyle() : MenuStyle - { - $style = (new MenuStyle($this->terminal)) - ->setFg($this->style['fg']) - ->setBg($this->style['bg']) - ->setWidth($this->style['width']) - ->setPaddingTopBottom($this->style['paddingTopBottom']) - ->setPaddingLeftRight($this->style['paddingLeftRight']) - ->setSelectedMarker($this->style['selectedMarker']) - ->setUnselectedMarker($this->style['unselectedMarker']) - ->setItemExtra($this->style['itemExtra']) - ->setDisplaysExtra($this->style['displaysExtra']) - ->setTitleSeparator($this->style['titleSeparator']) - ->setBorderTopWidth($this->style['borderTopWidth']) - ->setBorderRightWidth($this->style['borderRightWidth']) - ->setBorderBottomWidth($this->style['borderBottomWidth']) - ->setBorderLeftWidth($this->style['borderLeftWidth']) - ->setBorderColour($this->style['borderColour'], $this->style['borderColourFallback']); - - $this->style['marginAuto'] ? $style->setMarginAuto() : $style->setMargin($this->style['margin']); - - return $style; - } - /** * @throws RuntimeException */ @@ -450,7 +381,7 @@ public function build() : CliMenu $menuItems = $this->buildSplitItems($mergedItems); $menuItems = $this->buildSubMenus($menuItems); - $this->style['displaysExtra'] = $this->itemsHaveExtra($menuItems); + $this->style->setDisplaysExtra($this->itemsHaveExtra($menuItems)); $menu = new CliMenu( $this->menuTitle, diff --git a/src/Builder/SplitItemBuilder.php b/src/Builder/SplitItemBuilder.php index d5e58b9e..01007875 100644 --- a/src/Builder/SplitItemBuilder.php +++ b/src/Builder/SplitItemBuilder.php @@ -19,7 +19,12 @@ class SplitItemBuilder implements Builder */ private $gutter = 2; - public function __construct(Builder $parent) + /** + * @var CliMenuBuilder + */ + private $parent; + + public function __construct(CliMenuBuilder $parent) { $this->parent = $parent; } diff --git a/src/MenuStyle.php b/src/MenuStyle.php index 9f001eda..bd0a6dac 100644 --- a/src/MenuStyle.php +++ b/src/MenuStyle.php @@ -167,15 +167,9 @@ class MenuStyle 'borderBottomWidth' => 0, 'borderLeftWidth' => 0, 'borderColour' => 'white', - 'borderColourFallback' => null, 'marginAuto' => false, ]; - public static function getDefaultStyleValues() : array - { - return static::$defaultStyleValues; - } - /** * @var array */ @@ -243,10 +237,32 @@ public function __construct(Terminal $terminal = null) $this->setBorderRightWidth(static::$defaultStyleValues['borderRightWidth']); $this->setBorderBottomWidth(static::$defaultStyleValues['borderBottomWidth']); $this->setBorderLeftWidth(static::$defaultStyleValues['borderLeftWidth']); - $this->setBorderColour( - static::$defaultStyleValues['borderColour'], - static::$defaultStyleValues['borderColourFallback'] - ); + $this->setBorderColour(static::$defaultStyleValues['borderColour']); + } + + public function hasChangedFromDefaults() : bool + { + $currentValues = [ + $this->fg, + $this->bg, + $this->width, + $this->paddingTopBottom, + $this->paddingLeftRight, + $this->margin, + $this->selectedMarker, + $this->unselectedMarker, + $this->itemExtra, + $this->displaysExtra, + $this->titleSeparator, + $this->borderTopWidth, + $this->borderRightWidth, + $this->borderBottomWidth, + $this->borderLeftWidth, + $this->borderColour, + $this->marginAuto, + ]; + + return $currentValues !== array_values(static::$defaultStyleValues); } public function getDisabledItemText(string $text) : string diff --git a/test/Builder/CliMenuBuilderTest.php b/test/Builder/CliMenuBuilderTest.php index 3f315919..67f86fb6 100644 --- a/test/Builder/CliMenuBuilderTest.php +++ b/test/Builder/CliMenuBuilderTest.php @@ -51,7 +51,13 @@ public function testModifyExitButtonText() : void public function testModifyStyles() : void { - $builder = new CliMenuBuilder; + $terminal = static::createMock(Terminal::class); + $terminal + ->expects($this->any()) + ->method('getWidth') + ->will($this->returnValue(200)); + + $builder = new CliMenuBuilder($terminal); $builder->setBackgroundColour('red'); $builder->setForegroundColour('red'); $builder->setWidth(40); @@ -62,14 +68,6 @@ public function testModifyStyles() : void $builder->setItemExtra('*'); $builder->setTitleSeparator('-'); - $terminal = static::createMock(Terminal::class); - $terminal - ->expects($this->any()) - ->method('getWidth') - ->will($this->returnValue(200)); - - $builder->setTerminal($terminal); - $menu = $builder->build(); $this->checkStyleVariable($menu, 'bg', 'red'); @@ -92,80 +90,80 @@ public function testSetBorderShorthandFunction() : void ->method('getWidth') ->will($this->returnValue(200)); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2) ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 2); $this->checkStyleVariable($menu, 'borderBottomWidth', 2); $this->checkStyleVariable($menu, 'borderLeftWidth', 2); $this->checkStyleVariable($menu, 'borderColour', 'white'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4) ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 2); $this->checkStyleVariable($menu, 'borderLeftWidth', 4); $this->checkStyleVariable($menu, 'borderColour', 'white'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4, 6) ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 6); $this->checkStyleVariable($menu, 'borderLeftWidth', 4); $this->checkStyleVariable($menu, 'borderColour', 'white'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4, 6, 8) ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 6); $this->checkStyleVariable($menu, 'borderLeftWidth', 8); $this->checkStyleVariable($menu, 'borderColour', 'white'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4, 6, 8, 'green') ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 6); $this->checkStyleVariable($menu, 'borderLeftWidth', 8); $this->checkStyleVariable($menu, 'borderColour', 'green'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4, 6, 'green') ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 6); $this->checkStyleVariable($menu, 'borderLeftWidth', 4); $this->checkStyleVariable($menu, 'borderColour', 'green'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 4, 'green') ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 4); $this->checkStyleVariable($menu, 'borderBottomWidth', 2); $this->checkStyleVariable($menu, 'borderLeftWidth', 4); $this->checkStyleVariable($menu, 'borderColour', 'green'); - $menu = (new CliMenuBuilder) - ->setTerminal($terminal) + $menu = (new CliMenuBuilder($terminal)) ->setBorder(2, 'green') ->build(); + $this->checkStyleVariable($menu, 'borderTopWidth', 2); $this->checkStyleVariable($menu, 'borderRightWidth', 2); $this->checkStyleVariable($menu, 'borderBottomWidth', 2); @@ -231,8 +229,7 @@ public function test256ColoursCodes() : void ->method('getColourSupport') ->will($this->returnValue(256)); - $builder = new CliMenuBuilder; - $builder->setTerminal($terminal); + $builder = new CliMenuBuilder($terminal); $builder->setBackgroundColour(16, 'white'); $builder->setForegroundColour(206, 'red'); $menu = $builder->build(); @@ -246,8 +243,7 @@ public function test256ColoursCodes() : void ->method('getColourSupport') ->will($this->returnValue(8)); - $builder = new CliMenuBuilder; - $builder->setTerminal($terminal); + $builder = new CliMenuBuilder($terminal); $builder->setBackgroundColour(16, 'white'); $builder->setForegroundColour(206, 'red'); $menu = $builder->build(); @@ -267,8 +263,7 @@ public function testSetFgThrowsExceptionWhenColourCodeIsNotInRange() : void ->method('getColourSupport') ->will($this->returnValue(256)); - $builder = new CliMenuBuilder; - $builder->setTerminal($terminal); + $builder = new CliMenuBuilder($terminal); $builder->setForegroundColour(512, 'white'); } @@ -283,8 +278,7 @@ public function testSetBgThrowsExceptionWhenColourCodeIsNotInRange() : void ->method('getColourSupport') ->will($this->returnValue(256)); - $builder = new CliMenuBuilder; - $builder->setTerminal($terminal); + $builder = new CliMenuBuilder($terminal); $builder->setBackgroundColour(257, 'white'); } @@ -523,8 +517,8 @@ public function testAddSubMenuWithBuilderThrowsExceptionOnNonUniqueId() : void $subMenuBuilder = new CliMenuBuilder; $builder = new CliMenuBuilder; - $builder->addSubMenu('sub-menu', 'My SubMenu', $subMenuBuilder); - $builder->addSubMenu('sub-menu', 'My Other SubMenu', $subMenuBuilder); + $builder->addSubMenuFromExistingBuilder('sub-menu', 'My SubMenu', $subMenuBuilder); + $builder->addSubMenuFromExistingBuilder('sub-menu', 'My Other SubMenu', $subMenuBuilder); } public function testAddSubMenuUsesTextParameterAsMenuItemText() : void @@ -551,6 +545,7 @@ public function testSubMenuInheritsParentsStyle() : void ->build(); $this->assertSame('green', $builder->getSubMenu('sub-menu')->getStyle()->getBg()); + self::assertSame($menu->getStyle(), $builder->getSubMenu('sub-menu')->getStyle()); } public function testSubMenuDoesNotInheritsParentsStyleWhenSubMenuStyleHasAlterations() : void @@ -707,9 +702,8 @@ public function testSetMarginAutoAutomaticallyCalculatesMarginToCenter() : void ->method('getWidth') ->will($this->returnValue(200)); - $builder = new CliMenuBuilder; + $builder = new CliMenuBuilder($terminal); $menu = $builder - ->setTerminal($terminal) ->setMarginAuto() ->setWidth(100) ->build(); @@ -725,9 +719,8 @@ public function testSetMarginAutoOverwritesSetMargin() : void ->method('getWidth') ->will($this->returnValue(200)); - $builder = new CliMenuBuilder; + $builder = new CliMenuBuilder($terminal); $menu = $builder - ->setTerminal($terminal) ->setMargin(10) ->setMarginAuto() ->setWidth(100) @@ -744,9 +737,8 @@ public function testSetMarginManuallyOverwritesSetMarginAuto() : void ->method('getWidth') ->will($this->returnValue(200)); - $builder = new CliMenuBuilder; + $builder = new CliMenuBuilder($terminal); $menu = $builder - ->setTerminal($terminal) ->setMarginAuto() ->setMargin(10) ->setWidth(100) diff --git a/test/MenuStyleTest.php b/test/MenuStyleTest.php index 4f4d393c..dfdd9a88 100644 --- a/test/MenuStyleTest.php +++ b/test/MenuStyleTest.php @@ -16,24 +16,7 @@ class MenuStyleTest extends TestCase { private function getMenuStyle(int $colours = 8) : MenuStyle { - // Use the CliMenuBuilder & reflection to get the style Obj - $builder = new CliMenuBuilder(); - $menu = $builder->build(); - - $reflectionMenu = new \ReflectionObject($menu); - $styleProperty = $reflectionMenu->getProperty('style'); - $styleProperty->setAccessible(true); - $style = $styleProperty->getValue($menu); - - $reflectionStyle = new \ReflectionObject($style); - $terminalProperty = $reflectionStyle->getProperty('terminal'); - $terminalProperty->setAccessible(true); - $terminalProperty->setValue($style, $this->getMockTerminal($colours)); - - // Force recalculate terminal widths now terminal is set - $style->setWidth(100); - - return $style; + return new MenuStyle($this->getMockTerminal($colours)); } private function getMockTerminal(int $colours = 8) : MockObject From 00e4a9b5819154fee332091b207c3b1a5b491ebd Mon Sep 17 00:00:00 2001 From: Aydin Hassan Date: Tue, 15 May 2018 12:49:01 +0200 Subject: [PATCH 2/2] Mock terminal --- src/Builder/SplitItemBuilder.php | 2 +- test/Builder/CliMenuBuilderTest.php | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Builder/SplitItemBuilder.php b/src/Builder/SplitItemBuilder.php index 01007875..16ce34e1 100644 --- a/src/Builder/SplitItemBuilder.php +++ b/src/Builder/SplitItemBuilder.php @@ -20,7 +20,7 @@ class SplitItemBuilder implements Builder private $gutter = 2; /** - * @var CliMenuBuilder + * @var CliMenuBuilder */ private $parent; diff --git a/test/Builder/CliMenuBuilderTest.php b/test/Builder/CliMenuBuilderTest.php index 67f86fb6..8f1ee8c8 100644 --- a/test/Builder/CliMenuBuilderTest.php +++ b/test/Builder/CliMenuBuilderTest.php @@ -536,7 +536,13 @@ public function testAddSubMenuUsesTextParameterAsMenuItemText() : void public function testSubMenuInheritsParentsStyle() : void { - $builder = new CliMenuBuilder; + $terminal = self::createMock(Terminal::class); + $terminal + ->expects($this->any()) + ->method('getWidth') + ->will($this->returnValue(200)); + + $builder = new CliMenuBuilder($terminal); $menu = $builder->setBackgroundColour('green') ->addSubMenu('sub-menu', 'My SubMenu') ->addItem('Some Item', function () {