From 70088f7eec8fe8e33c7f6deae9dc289395ace42c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Aug 2017 08:08:18 -0700 Subject: [PATCH] Continue reducing callsites to ArcanistDifferentialRevisionStatus Summary: Ref T2543. Further consolidates status management into DifferentialRevisionStatus. One change I'm making here is internally renaming "CLOSED" to "PUBLISHED". The UI will continue to say "Closed", at least for now, but this should make the code more clear because we care about "is closed, exactly" vs "is any closed status (closed, abandoned, sometimes accepted)". This distinction is more obvious as `isClosed()` vs `isPublished()` than, e.g., `isClosedWithExactlyTheClosedStatus()` or something. I think "Published" is generally more clear, too, and more consistent with modern language (e.g., "pre-publish review" replacing "pre-commit review" to make it more clear what we mean in Git/Mercurial). I've removed the IN_PREPARATION status since this was just earlier groundwork for "Draft" and not actually used, and under the newer plan I'm trying to just abandon `ArcanistDifferentialRevisionStatus` entirely (or, at least, substantially). Test Plan: - Viewed revisions. - Viewed revision list. - Viewed revisions linked to a task in Maniphest. - Viewed revision graph of dependencies in Differential. - Grepped for `COLOR_STATUS_...` constants. - Grepped for removed method `getRevisionStatusIcon()` (no callsites). - Grepped for removed method `renderFullDescription()` (one callsite, replaced with just building a `TagView` inline). - Grepped for removed method `isClosedStatus()` (no callsites after other changes). Reviewers: chad Reviewed By: chad Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18340 --- .../constants/DifferentialRevisionStatus.php | 178 +++++++++++------- .../DifferentialRevisionViewController.php | 10 +- .../phid/DifferentialRevisionPHIDType.php | 4 +- .../storage/DifferentialRevision.php | 39 ++-- .../view/DifferentialRevisionListView.php | 5 +- .../graph/DifferentialRevisionGraph.php | 4 +- 6 files changed, 144 insertions(+), 96 deletions(-) diff --git a/src/applications/differential/constants/DifferentialRevisionStatus.php b/src/applications/differential/constants/DifferentialRevisionStatus.php index 38e6a2dd49..0b2abe51fe 100644 --- a/src/applications/differential/constants/DifferentialRevisionStatus.php +++ b/src/applications/differential/constants/DifferentialRevisionStatus.php @@ -1,74 +1,130 @@ - self::COLOR_STATUS_DEFAULT, - ArcanistDifferentialRevisionStatus::NEEDS_REVISION => - self::COLOR_STATUS_RED, - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => - self::COLOR_STATUS_RED, - ArcanistDifferentialRevisionStatus::ACCEPTED => - self::COLOR_STATUS_GREEN, - ArcanistDifferentialRevisionStatus::CLOSED => - self::COLOR_STATUS_DARK, - ArcanistDifferentialRevisionStatus::ABANDONED => - self::COLOR_STATUS_DARK, - ArcanistDifferentialRevisionStatus::IN_PREPARATION => - self::COLOR_STATUS_BLUE, - ); - return idx($map, $status, $default); + public function getIcon() { + return idx($this->spec, 'icon'); } - public static function getRevisionStatusIcon($status) { - $default = 'fa-square-o bluegrey'; - - $map = array( - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW => - 'fa-square-o bluegrey', - ArcanistDifferentialRevisionStatus::NEEDS_REVISION => - 'fa-refresh', - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => - 'fa-headphones', - ArcanistDifferentialRevisionStatus::ACCEPTED => - 'fa-check', - ArcanistDifferentialRevisionStatus::CLOSED => - 'fa-check-square-o', - ArcanistDifferentialRevisionStatus::ABANDONED => - 'fa-plane', - ArcanistDifferentialRevisionStatus::IN_PREPARATION => - 'fa-question-circle', - ); - return idx($map, $status, $default); + public function getIconColor() { + return idx($this->spec, 'color.icon', 'bluegrey'); } - public static function renderFullDescription($status) { - $status_name = - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus($status); + public function getTagColor() { + return idx($this->spec, 'color.tag', 'bluegrey'); + } - $tag = id(new PHUITagView()) - ->setName($status_name) - ->setIcon(self::getRevisionStatusIcon($status)) - ->setColor(self::getRevisionStatusColor($status)) - ->setType(PHUITagView::TYPE_SHADE); + public function getDisplayName() { + return idx($this->spec, 'name'); + } - return $tag; + public function isClosedStatus() { + return idx($this->spec, 'closed'); + } + + public function isAbandoned() { + return ($this->key === self::ABANDONED); + } + + public function isAccepted() { + return ($this->key === self::ACCEPTED); + } + + public function isNeedsReview() { + return ($this->key === self::NEEDS_REVIEW); + } + + public static function newForLegacyStatus($legacy_status) { + $result = new self(); + + $map = self::getMap(); + foreach ($map as $key => $spec) { + if (!isset($spec['legacy'])) { + continue; + } + + if ($spec['legacy'] != $legacy_status) { + continue; + } + + $result->key = $key; + $result->spec = $spec; + break; + } + + return $result; + } + + private static function getMap() { + $close_on_accept = PhabricatorEnv::getEnvConfig( + 'differential.close-on-accept'); + + return array( + self::NEEDS_REVIEW => array( + 'name' => pht('Needs Review'), + 'legacy' => 0, + 'icon' => 'fa-code', + 'closed' => false, + 'color.icon' => 'grey', + 'color.tag' => 'bluegrey', + 'color.ansi' => 'magenta', + ), + self::NEEDS_REVISION => array( + 'name' => pht('Needs Revision'), + 'legacy' => 1, + 'icon' => 'fa-refresh', + 'closed' => false, + 'color.icon' => 'red', + 'color.tag' => 'red', + 'color.ansi' => 'red', + ), + self::CHANGES_PLANNED => array( + 'name' => pht('Changes Planned'), + 'legacy' => 5, + 'icon' => 'fa-headphones', + 'closed' => false, + 'color.icon' => 'red', + 'color.tag' => 'red', + 'color.ansi' => 'red', + ), + self::ACCEPTED => array( + 'name' => pht('Accepted'), + 'legacy' => 2, + 'icon' => 'fa-check', + 'closed' => $close_on_accept, + 'color.icon' => 'green', + 'color.tag' => 'green', + 'color.ansi' => 'green', + ), + self::PUBLISHED => array( + 'name' => pht('Closed'), + 'legacy' => 3, + 'icon' => 'fa-check-square-o', + 'closed' => true, + 'color.icon' => 'black', + 'color.tag' => 'indigo', + 'color.ansi' => 'cyan', + ), + self::ABANDONED => array( + 'name' => pht('Abandoned'), + 'legacy' => 4, + 'icon' => 'fa-plane', + 'closed' => true, + 'color.icon' => 'black', + 'color.tag' => 'indigo', + 'color.ansi' => null, + ), + ); } public static function getClosedStatuses() { @@ -100,8 +156,4 @@ final class DifferentialRevisionStatus extends Phobject { ); } - public static function isClosedStatus($status) { - return in_array($status, self::getClosedStatuses()); - } - } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index ab03ee6d81..5c7f5104f5 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -508,11 +508,13 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setPolicyObject($revision) ->setHeaderIcon('fa-cog'); - $status = $revision->getStatus(); - $status_name = - DifferentialRevisionStatus::renderFullDescription($status); + $status_tag = id(new PHUITagView()) + ->setName($revision->getStatusDisplayName()) + ->setIcon($revision->getStatusIcon()) + ->setColor($revision->getStatusIconColor()) + ->setType(PHUITagView::TYPE_SHADE); - $view->addProperty(PHUIHeaderView::PROPERTY_STATUS, $status_name); + $view->addProperty(PHUIHeaderView::PROPERTY_STATUS, $status_tag); return $view; } diff --git a/src/applications/differential/phid/DifferentialRevisionPHIDType.php b/src/applications/differential/phid/DifferentialRevisionPHIDType.php index 8547ec9e83..d652a8c056 100644 --- a/src/applications/differential/phid/DifferentialRevisionPHIDType.php +++ b/src/applications/differential/phid/DifferentialRevisionPHIDType.php @@ -48,8 +48,8 @@ final class DifferentialRevisionPHIDType extends PhabricatorPHIDType { $status = $revision->getStatus(); - $icon = DifferentialRevisionStatus::getRevisionStatusIcon($status); - $color = DifferentialRevisionStatus::getRevisionStatusColor($status); + $icon = $revision->getStatusIcon($status); + $color = $revision->getStatusIconColor($status); $name = $revision->getStatusDisplayName(); $handle diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index cd6df4b6e5..c2856f5d20 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -613,47 +613,36 @@ final class DifferentialRevision extends DifferentialDAO } public function isClosed() { - return DifferentialRevisionStatus::isClosedStatus($this->getStatus()); + return $this->getStatusObject()->isClosedStatus(); } public function isAbandoned() { - $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; - return ($this->getStatus() == $status_abandoned); + return $this->getStatusObject()->isAbandoned(); } public function isAccepted() { - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - return ($this->getStatus() == $status_accepted); + return $this->getStatusObject()->isAccepted(); } public function isNeedsReview() { - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - return ($this->getStatus() == $status_review); + return $this->getStatusObject()->isNeedsReview(); } public function getStatusIcon() { - $map = array( - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW - => 'fa-code grey', - ArcanistDifferentialRevisionStatus::NEEDS_REVISION - => 'fa-refresh red', - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED - => 'fa-headphones red', - ArcanistDifferentialRevisionStatus::ACCEPTED - => 'fa-check green', - ArcanistDifferentialRevisionStatus::CLOSED - => 'fa-check-square-o black', - ArcanistDifferentialRevisionStatus::ABANDONED - => 'fa-plane black', - ); - - return idx($map, $this->getStatus()); + return $this->getStatusObject()->getIcon(); } public function getStatusDisplayName() { + return $this->getStatusObject()->getDisplayName(); + } + + public function getStatusIconColor() { + return $this->getStatusObject()->getIconColor(); + } + + public function getStatusObject() { $status = $this->getStatus(); - return ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( - $status); + return DifferentialRevisionStatus::newForLegacyStatus($status); } public function getFlag(PhabricatorUser $viewer) { diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php index 5373192b31..ed97435746 100644 --- a/src/applications/differential/view/DifferentialRevisionListView.php +++ b/src/applications/differential/view/DifferentialRevisionListView.php @@ -145,8 +145,11 @@ final class DifferentialRevisionListView extends AphrontView { $item->setDisabled(true); } + $icon = $revision->getStatusIcon(); + $color = $revision->getStatusIconColor(); + $item->setStatusIcon( - $revision->getStatusIcon(), + "{$icon} {$color}", $revision->getStatusDisplayName()); $list->addItem($item); diff --git a/src/infrastructure/graph/DifferentialRevisionGraph.php b/src/infrastructure/graph/DifferentialRevisionGraph.php index 892540f610..715820e9e3 100644 --- a/src/infrastructure/graph/DifferentialRevisionGraph.php +++ b/src/infrastructure/graph/DifferentialRevisionGraph.php @@ -27,10 +27,12 @@ final class DifferentialRevisionGraph if ($object) { $status_icon = $object->getStatusIcon(); + $status_color = $object->getStatusIconColor(); $status_name = $object->getStatusDisplayName(); $status = array( - id(new PHUIIconView())->setIcon($status_icon), + id(new PHUIIconView()) + ->setIcon($status_icon, $status_color), ' ', $status_name, );