@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

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

+143 -95
+114 -62
src/applications/differential/constants/DifferentialRevisionStatus.php
··· 1 1 <?php 2 2 3 - /** 4 - * NOTE: you probably want {@class:ArcanistDifferentialRevisionStatus}. 5 - * This class just contains a mapping for color within the Differential 6 - * application. 7 - */ 8 - 9 3 final class DifferentialRevisionStatus extends Phobject { 10 4 11 - const COLOR_STATUS_DEFAULT = 'bluegrey'; 12 - const COLOR_STATUS_DARK = 'indigo'; 13 - const COLOR_STATUS_BLUE = 'blue'; 14 - const COLOR_STATUS_GREEN = 'green'; 15 - const COLOR_STATUS_RED = 'red'; 5 + const NEEDS_REVIEW = 'needs-review'; 6 + const NEEDS_REVISION = 'needs-revision'; 7 + const CHANGES_PLANNED = 'changes-planned'; 8 + const ACCEPTED = 'accepted'; 9 + const PUBLISHED = 'published'; 10 + const ABANDONED = 'abandoned'; 16 11 17 - public static function getRevisionStatusColor($status) { 18 - $default = self::COLOR_STATUS_DEFAULT; 12 + private $key; 13 + private $spec = array(); 19 14 20 - $map = array( 21 - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW => 22 - self::COLOR_STATUS_DEFAULT, 23 - ArcanistDifferentialRevisionStatus::NEEDS_REVISION => 24 - self::COLOR_STATUS_RED, 25 - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => 26 - self::COLOR_STATUS_RED, 27 - ArcanistDifferentialRevisionStatus::ACCEPTED => 28 - self::COLOR_STATUS_GREEN, 29 - ArcanistDifferentialRevisionStatus::CLOSED => 30 - self::COLOR_STATUS_DARK, 31 - ArcanistDifferentialRevisionStatus::ABANDONED => 32 - self::COLOR_STATUS_DARK, 33 - ArcanistDifferentialRevisionStatus::IN_PREPARATION => 34 - self::COLOR_STATUS_BLUE, 35 - ); 36 - return idx($map, $status, $default); 15 + public function getIcon() { 16 + return idx($this->spec, 'icon'); 37 17 } 38 18 39 - public static function getRevisionStatusIcon($status) { 40 - $default = 'fa-square-o bluegrey'; 19 + public function getIconColor() { 20 + return idx($this->spec, 'color.icon', 'bluegrey'); 21 + } 41 22 42 - $map = array( 43 - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW => 44 - 'fa-square-o bluegrey', 45 - ArcanistDifferentialRevisionStatus::NEEDS_REVISION => 46 - 'fa-refresh', 47 - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => 48 - 'fa-headphones', 49 - ArcanistDifferentialRevisionStatus::ACCEPTED => 50 - 'fa-check', 51 - ArcanistDifferentialRevisionStatus::CLOSED => 52 - 'fa-check-square-o', 53 - ArcanistDifferentialRevisionStatus::ABANDONED => 54 - 'fa-plane', 55 - ArcanistDifferentialRevisionStatus::IN_PREPARATION => 56 - 'fa-question-circle', 57 - ); 58 - return idx($map, $status, $default); 23 + public function getTagColor() { 24 + return idx($this->spec, 'color.tag', 'bluegrey'); 59 25 } 60 26 61 - public static function renderFullDescription($status) { 62 - $status_name = 63 - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus($status); 27 + public function getDisplayName() { 28 + return idx($this->spec, 'name'); 29 + } 64 30 65 - $tag = id(new PHUITagView()) 66 - ->setName($status_name) 67 - ->setIcon(self::getRevisionStatusIcon($status)) 68 - ->setColor(self::getRevisionStatusColor($status)) 69 - ->setType(PHUITagView::TYPE_SHADE); 31 + public function isClosedStatus() { 32 + return idx($this->spec, 'closed'); 33 + } 70 34 71 - return $tag; 35 + public function isAbandoned() { 36 + return ($this->key === self::ABANDONED); 37 + } 38 + 39 + public function isAccepted() { 40 + return ($this->key === self::ACCEPTED); 41 + } 42 + 43 + public function isNeedsReview() { 44 + return ($this->key === self::NEEDS_REVIEW); 45 + } 46 + 47 + public static function newForLegacyStatus($legacy_status) { 48 + $result = new self(); 49 + 50 + $map = self::getMap(); 51 + foreach ($map as $key => $spec) { 52 + if (!isset($spec['legacy'])) { 53 + continue; 54 + } 55 + 56 + if ($spec['legacy'] != $legacy_status) { 57 + continue; 58 + } 59 + 60 + $result->key = $key; 61 + $result->spec = $spec; 62 + break; 63 + } 64 + 65 + return $result; 66 + } 67 + 68 + private static function getMap() { 69 + $close_on_accept = PhabricatorEnv::getEnvConfig( 70 + 'differential.close-on-accept'); 71 + 72 + return array( 73 + self::NEEDS_REVIEW => array( 74 + 'name' => pht('Needs Review'), 75 + 'legacy' => 0, 76 + 'icon' => 'fa-code', 77 + 'closed' => false, 78 + 'color.icon' => 'grey', 79 + 'color.tag' => 'bluegrey', 80 + 'color.ansi' => 'magenta', 81 + ), 82 + self::NEEDS_REVISION => array( 83 + 'name' => pht('Needs Revision'), 84 + 'legacy' => 1, 85 + 'icon' => 'fa-refresh', 86 + 'closed' => false, 87 + 'color.icon' => 'red', 88 + 'color.tag' => 'red', 89 + 'color.ansi' => 'red', 90 + ), 91 + self::CHANGES_PLANNED => array( 92 + 'name' => pht('Changes Planned'), 93 + 'legacy' => 5, 94 + 'icon' => 'fa-headphones', 95 + 'closed' => false, 96 + 'color.icon' => 'red', 97 + 'color.tag' => 'red', 98 + 'color.ansi' => 'red', 99 + ), 100 + self::ACCEPTED => array( 101 + 'name' => pht('Accepted'), 102 + 'legacy' => 2, 103 + 'icon' => 'fa-check', 104 + 'closed' => $close_on_accept, 105 + 'color.icon' => 'green', 106 + 'color.tag' => 'green', 107 + 'color.ansi' => 'green', 108 + ), 109 + self::PUBLISHED => array( 110 + 'name' => pht('Closed'), 111 + 'legacy' => 3, 112 + 'icon' => 'fa-check-square-o', 113 + 'closed' => true, 114 + 'color.icon' => 'black', 115 + 'color.tag' => 'indigo', 116 + 'color.ansi' => 'cyan', 117 + ), 118 + self::ABANDONED => array( 119 + 'name' => pht('Abandoned'), 120 + 'legacy' => 4, 121 + 'icon' => 'fa-plane', 122 + 'closed' => true, 123 + 'color.icon' => 'black', 124 + 'color.tag' => 'indigo', 125 + 'color.ansi' => null, 126 + ), 127 + ); 72 128 } 73 129 74 130 public static function getClosedStatuses() { ··· 98 154 ArcanistDifferentialRevisionStatus::ABANDONED, 99 155 ArcanistDifferentialRevisionStatus::IN_PREPARATION, 100 156 ); 101 - } 102 - 103 - public static function isClosedStatus($status) { 104 - return in_array($status, self::getClosedStatuses()); 105 157 } 106 158 107 159 }
+6 -4
src/applications/differential/controller/DifferentialRevisionViewController.php
··· 508 508 ->setPolicyObject($revision) 509 509 ->setHeaderIcon('fa-cog'); 510 510 511 - $status = $revision->getStatus(); 512 - $status_name = 513 - DifferentialRevisionStatus::renderFullDescription($status); 511 + $status_tag = id(new PHUITagView()) 512 + ->setName($revision->getStatusDisplayName()) 513 + ->setIcon($revision->getStatusIcon()) 514 + ->setColor($revision->getStatusIconColor()) 515 + ->setType(PHUITagView::TYPE_SHADE); 514 516 515 - $view->addProperty(PHUIHeaderView::PROPERTY_STATUS, $status_name); 517 + $view->addProperty(PHUIHeaderView::PROPERTY_STATUS, $status_tag); 516 518 517 519 return $view; 518 520 }
+2 -2
src/applications/differential/phid/DifferentialRevisionPHIDType.php
··· 48 48 49 49 $status = $revision->getStatus(); 50 50 51 - $icon = DifferentialRevisionStatus::getRevisionStatusIcon($status); 52 - $color = DifferentialRevisionStatus::getRevisionStatusColor($status); 51 + $icon = $revision->getStatusIcon($status); 52 + $color = $revision->getStatusIconColor($status); 53 53 $name = $revision->getStatusDisplayName(); 54 54 55 55 $handle
+14 -25
src/applications/differential/storage/DifferentialRevision.php
··· 613 613 } 614 614 615 615 public function isClosed() { 616 - return DifferentialRevisionStatus::isClosedStatus($this->getStatus()); 616 + return $this->getStatusObject()->isClosedStatus(); 617 617 } 618 618 619 619 public function isAbandoned() { 620 - $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; 621 - return ($this->getStatus() == $status_abandoned); 620 + return $this->getStatusObject()->isAbandoned(); 622 621 } 623 622 624 623 public function isAccepted() { 625 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 626 - return ($this->getStatus() == $status_accepted); 624 + return $this->getStatusObject()->isAccepted(); 627 625 } 628 626 629 627 public function isNeedsReview() { 630 - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 631 - return ($this->getStatus() == $status_review); 628 + return $this->getStatusObject()->isNeedsReview(); 632 629 } 633 630 634 631 public function getStatusIcon() { 635 - $map = array( 636 - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW 637 - => 'fa-code grey', 638 - ArcanistDifferentialRevisionStatus::NEEDS_REVISION 639 - => 'fa-refresh red', 640 - ArcanistDifferentialRevisionStatus::CHANGES_PLANNED 641 - => 'fa-headphones red', 642 - ArcanistDifferentialRevisionStatus::ACCEPTED 643 - => 'fa-check green', 644 - ArcanistDifferentialRevisionStatus::CLOSED 645 - => 'fa-check-square-o black', 646 - ArcanistDifferentialRevisionStatus::ABANDONED 647 - => 'fa-plane black', 648 - ); 632 + return $this->getStatusObject()->getIcon(); 633 + } 634 + 635 + public function getStatusDisplayName() { 636 + return $this->getStatusObject()->getDisplayName(); 637 + } 649 638 650 - return idx($map, $this->getStatus()); 639 + public function getStatusIconColor() { 640 + return $this->getStatusObject()->getIconColor(); 651 641 } 652 642 653 - public function getStatusDisplayName() { 643 + public function getStatusObject() { 654 644 $status = $this->getStatus(); 655 - return ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( 656 - $status); 645 + return DifferentialRevisionStatus::newForLegacyStatus($status); 657 646 } 658 647 659 648 public function getFlag(PhabricatorUser $viewer) {
+4 -1
src/applications/differential/view/DifferentialRevisionListView.php
··· 145 145 $item->setDisabled(true); 146 146 } 147 147 148 + $icon = $revision->getStatusIcon(); 149 + $color = $revision->getStatusIconColor(); 150 + 148 151 $item->setStatusIcon( 149 - $revision->getStatusIcon(), 152 + "{$icon} {$color}", 150 153 $revision->getStatusDisplayName()); 151 154 152 155 $list->addItem($item);
+3 -1
src/infrastructure/graph/DifferentialRevisionGraph.php
··· 27 27 28 28 if ($object) { 29 29 $status_icon = $object->getStatusIcon(); 30 + $status_color = $object->getStatusIconColor(); 30 31 $status_name = $object->getStatusDisplayName(); 31 32 32 33 $status = array( 33 - id(new PHUIIconView())->setIcon($status_icon), 34 + id(new PHUIIconView()) 35 + ->setIcon($status_icon, $status_color), 34 36 ' ', 35 37 $status_name, 36 38 );