From 2bfa0e087e0029ea427b0286942e16e068d36511 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Oct 2015 07:38:15 -0700 Subject: [PATCH] Improve consistency and Harbormaster integration of Diffusion Summary: Ref T9123. Two major Harbormaster-related UI changes in Diffusion: - Tags table now shows tag build status. - Branches table now shows branch build status. Then some minor consistency / qualtiy of life changes: - Picked a nicer looking "history" icon? - Branches table now uses the same "history" icon as other tables. - Tags table now has a "history" link. - Browse table now has a "history" link. - Dates now use more consistent formatting. - Column order is now more consistent. - Use of style is now more consistent. Test Plan: {F865056} {F865057} {F865058} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9123 Differential Revision: https://secure.phabricator.com/D14242 --- .../DiffusionLastModifiedController.php | 5 +- .../view/DiffusionBranchTableView.php | 45 ++++++----- .../view/DiffusionBrowseTableView.php | 19 ++--- .../view/DiffusionHistoryTableView.php | 80 +++---------------- .../diffusion/view/DiffusionTagListView.php | 60 ++++++++++---- .../diffusion/view/DiffusionView.php | 80 ++++++++++++++++++- 6 files changed, 174 insertions(+), 115 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionLastModifiedController.php b/src/applications/diffusion/controller/DiffusionLastModifiedController.php index 3487733c8b..b23abf1903 100644 --- a/src/applications/diffusion/controller/DiffusionLastModifiedController.php +++ b/src/applications/diffusion/controller/DiffusionLastModifiedController.php @@ -98,12 +98,10 @@ final class DiffusionLastModifiedController extends DiffusionController { $modified = DiffusionView::linkCommit( $drequest->getRepository(), $commit->getCommitIdentifier()); - $date = phabricator_date($epoch, $viewer); - $time = phabricator_time($epoch, $viewer); + $date = phabricator_datetime($epoch, $viewer); } else { $modified = ''; $date = ''; - $time = ''; } $data = $commit->getCommitData(); @@ -137,7 +135,6 @@ final class DiffusionLastModifiedController extends DiffusionController { $return = array( 'commit' => $modified, 'date' => $date, - 'time' => $time, 'author' => $author, 'details' => $details, ); diff --git a/src/applications/diffusion/view/DiffusionBranchTableView.php b/src/applications/diffusion/view/DiffusionBranchTableView.php index d053d5bb33..0f4594e576 100644 --- a/src/applications/diffusion/view/DiffusionBranchTableView.php +++ b/src/applications/diffusion/view/DiffusionBranchTableView.php @@ -21,6 +21,11 @@ final class DiffusionBranchTableView extends DiffusionView { $drequest = $this->getDiffusionRequest(); $current_branch = $drequest->getBranch(); $repository = $drequest->getRepository(); + $commits = $this->commits; + $viewer = $this->getUser(); + + $buildables = $this->loadBuildables($commits); + $have_builds = false; $can_close_branches = ($repository->isHg()); @@ -31,13 +36,21 @@ final class DiffusionBranchTableView extends DiffusionView { $rows = array(); $rowc = array(); foreach ($this->branches as $branch) { - $commit = idx($this->commits, $branch->getCommitIdentifier()); + $commit = idx($commits, $branch->getCommitIdentifier()); if ($commit) { $details = $commit->getSummary(); - $datetime = phabricator_datetime($commit->getEpoch(), $this->user); + $datetime = phabricator_datetime($commit->getEpoch(), $viewer); + $buildable = idx($buildables, $commit->getPHID()); + if ($buildable) { + $build_status = $this->renderBuildable($buildable); + $have_builds = true; + } else { + $build_status = null; + } } else { $datetime = null; $details = null; + $build_status = null; } switch ($repository->shouldSkipAutocloseBranch($branch->getShortName())) { @@ -86,16 +99,7 @@ final class DiffusionBranchTableView extends DiffusionView { } $rows[] = array( - phutil_tag( - 'a', - array( - 'href' => $drequest->generateURI( - array( - 'action' => 'history', - 'branch' => $branch->getShortName(), - )), - ), - pht('History')), + $this->linkBranchHistory($branch->getShortName()), phutil_tag( 'a', array( @@ -109,10 +113,11 @@ final class DiffusionBranchTableView extends DiffusionView { self::linkCommit( $drequest->getRepository(), $branch->getCommitIdentifier()), + $build_status, $status, + AphrontTableView::renderSingleDisplayLine($details), $status_icon, $datetime, - AphrontTableView::renderSingleDisplayLine($details), ); if ($branch->getShortName() == $current_branch) { $rowc[] = 'highlighted'; @@ -124,33 +129,37 @@ final class DiffusionBranchTableView extends DiffusionView { $view = new AphrontTableView($rows); $view->setHeaders( array( - pht('History'), + null, pht('Branch'), pht('Head'), + null, pht('State'), - pht(''), - pht('Modified'), pht('Details'), + null, + pht('Committed'), )); $view->setColumnClasses( array( '', 'pri', '', - '', - '', + 'icon', '', 'wide', + '', + '', )); $view->setColumnVisibility( array( true, true, true, + $have_builds, $can_close_branches, )); $view->setRowClasses($rowc); return $view->render(); } + } diff --git a/src/applications/diffusion/view/DiffusionBrowseTableView.php b/src/applications/diffusion/view/DiffusionBrowseTableView.php index a8eb745fe4..71cc659897 100644 --- a/src/applications/diffusion/view/DiffusionBrowseTableView.php +++ b/src/applications/diffusion/view/DiffusionBrowseTableView.php @@ -31,6 +31,8 @@ final class DiffusionBrowseTableView extends DiffusionView { $show_edit = false; foreach ($this->paths as $path) { + $history_link = $this->linkHistory($path->getPath()); + $dir_slash = null; $file_type = $path->getFileType(); if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { @@ -67,7 +69,6 @@ final class DiffusionBrowseTableView extends DiffusionView { 'lint' => celerity_generate_unique_node_id(), 'commit' => celerity_generate_unique_node_id(), 'date' => celerity_generate_unique_node_id(), - 'time' => celerity_generate_unique_node_id(), 'author' => celerity_generate_unique_node_id(), 'details' => celerity_generate_unique_node_id(), ); @@ -78,13 +79,13 @@ final class DiffusionBrowseTableView extends DiffusionView { } $rows[] = array( + $history_link, $browse_link, idx($dict, 'lint'), $dict['commit'], $dict['author'], $dict['details'], $dict['date'], - $dict['time'], ); } @@ -108,29 +109,29 @@ final class DiffusionBrowseTableView extends DiffusionView { $view = new AphrontTableView($rows); $view->setHeaders( array( + null, pht('Path'), ($lint ? $lint : pht('Lint')), pht('Modified'), pht('Author/Committer'), pht('Details'), - pht('Date'), - pht('Time'), + pht('Committed'), )); $view->setColumnClasses( array( + 'nudgeright', + '', + '', '', - 'n', - 'n', '', 'wide', '', - 'right', )); $view->setColumnVisibility( array( true, - $show_lint, true, + $show_lint, true, true, true, @@ -140,11 +141,11 @@ final class DiffusionBrowseTableView extends DiffusionView { $view->setDeviceVisibility( array( true, - false, true, false, true, false, + true, false, )); diff --git a/src/applications/diffusion/view/DiffusionHistoryTableView.php b/src/applications/diffusion/view/DiffusionHistoryTableView.php index 80989efdf2..314bfb435c 100644 --- a/src/applications/diffusion/view/DiffusionHistoryTableView.php +++ b/src/applications/diffusion/view/DiffusionHistoryTableView.php @@ -7,12 +7,10 @@ final class DiffusionHistoryTableView extends DiffusionView { private $handles = array(); private $isHead; private $parents; - private $buildCache; public function setHistory(array $history) { assert_instances_of($history, 'DiffusionPathChange'); $this->history = $history; - $this->buildCache = null; return $this; } @@ -62,33 +60,14 @@ final class DiffusionHistoryTableView extends DiffusionView { return $this; } - public function loadBuildablesOnDemand() { - if ($this->buildCache !== null) { - return $this->buildCache; - } - - $commits_to_builds = array(); - - $commits = mpull($this->history, 'getCommit'); - - $commit_phids = mpull($commits, 'getPHID'); - - $buildables = id(new HarbormasterBuildableQuery()) - ->setViewer($this->getUser()) - ->withBuildablePHIDs($commit_phids) - ->withManualBuildables(false) - ->execute(); - - $this->buildCache = mpull($buildables, null, 'getBuildablePHID'); - - return $this->buildCache; - } - public function render() { $drequest = $this->getDiffusionRequest(); $viewer = $this->getUser(); + $buildables = $this->loadBuildables(mpull($this->history, 'getCommit')); + $has_any_build = false; + $show_revisions = PhabricatorApplication::isClassInstalledForViewer( 'PhabricatorDifferentialApplication', $viewer); @@ -110,11 +89,9 @@ final class DiffusionHistoryTableView extends DiffusionView { $epoch = $history->getEpoch(); if ($epoch) { - $date = phabricator_date($epoch, $this->user); - $time = phabricator_time($epoch, $this->user); + $committed = phabricator_datetime($epoch, $viewer); } else { - $date = null; - $time = null; + $committed = null; } $data = $history->getCommitData(); @@ -160,36 +137,9 @@ final class DiffusionHistoryTableView extends DiffusionView { $build = null; if ($show_builds) { - $buildable_lookup = $this->loadBuildablesOnDemand(); - $buildable = idx($buildable_lookup, $commit->getPHID()); + $buildable = idx($buildables, $commit->getPHID()); if ($buildable !== null) { - $icon = HarbormasterBuildable::getBuildableStatusIcon( - $buildable->getBuildableStatus()); - $color = HarbormasterBuildable::getBuildableStatusColor( - $buildable->getBuildableStatus()); - $name = HarbormasterBuildable::getBuildableStatusName( - $buildable->getBuildableStatus()); - - $icon_view = id(new PHUIIconView()) - ->setIconFont($icon.' '.$color); - - $tooltip_view = javelin_tag( - 'span', - array( - 'sigil' => 'has-tooltip', - 'meta' => array('tip' => $name), - ), - $icon_view); - - Javelin::initBehavior('phabricator-tooltips'); - - $href_view = phutil_tag( - 'a', - array('href' => '/'.$buildable->getMonogram()), - $tooltip_view); - - $build = $href_view; - + $build = $this->renderBuildable($buildable); $has_any_build = true; } } @@ -214,8 +164,7 @@ final class DiffusionHistoryTableView extends DiffusionView { null), $author, $summary, - $date, - $time, + $committed, ); } @@ -226,30 +175,28 @@ final class DiffusionHistoryTableView extends DiffusionView { null, pht('Commit'), null, - pht('Revision'), + null, pht('Author/Committer'), pht('Details'), - pht('Date'), - pht('Time'), + pht('Committed'), )); $view->setColumnClasses( array( 'threads', 'nudgeright', - 'n', + '', 'icon', - 'n', + '', '', 'wide', '', - 'right', )); $view->setColumnVisibility( array( $graph ? true : false, true, true, - true, + $has_any_build, $show_revisions, )); $view->setDeviceVisibility( @@ -262,7 +209,6 @@ final class DiffusionHistoryTableView extends DiffusionView { false, true, false, - false, )); return $view->render(); } diff --git a/src/applications/diffusion/view/DiffusionTagListView.php b/src/applications/diffusion/view/DiffusionTagListView.php index 668453f88e..3b27284f5e 100644 --- a/src/applications/diffusion/view/DiffusionTagListView.php +++ b/src/applications/diffusion/view/DiffusionTagListView.php @@ -29,6 +29,8 @@ final class DiffusionTagListView extends DiffusionView { $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); + $buildables = $this->loadBuildables($this->commits); + $has_builds = false; $rows = array(); foreach ($this->tags as $tag) { @@ -80,30 +82,56 @@ final class DiffusionTagListView extends DiffusionView { } } + $build = null; + if ($commit) { + $buildable = idx($buildables, $commit->getPHID()); + if ($buildable) { + $build = $this->renderBuildable($buildable); + $has_builds = true; + } + } + + $history = $this->linkTagHistory($tag->getName()); + $rows[] = array( + $history, $tag_link, $commit_link, - $description, + $build, $author, + $description, phabricator_datetime($tag->getEpoch(), $this->user), ); } - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - pht('Tag'), - pht('Commit'), - pht('Description'), - pht('Author'), - pht('Created'), - )); - $table->setColumnClasses( - array( - 'pri', - '', - 'wide', - )); + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + null, + pht('Tag'), + pht('Commit'), + null, + pht('Author'), + pht('Description'), + pht('Created'), + )) + ->setColumnClasses( + array( + 'nudgeright', + 'pri', + '', + '', + '', + 'wide', + )) + ->setColumnVisibility( + array( + true, + true, + true, + $has_builds, + )); + return $table->render(); } diff --git a/src/applications/diffusion/view/DiffusionView.php b/src/applications/diffusion/view/DiffusionView.php index b7b3599a2d..83fdc1f7e5 100644 --- a/src/applications/diffusion/view/DiffusionView.php +++ b/src/applications/diffusion/view/DiffusionView.php @@ -20,6 +20,30 @@ abstract class DiffusionView extends AphrontView { 'path' => $path, )); + return $this->renderHistoryLink($href); + } + + final public function linkBranchHistory($branch) { + $href = $this->getDiffusionRequest()->generateURI( + array( + 'action' => 'history', + 'branch' => $branch, + )); + + return $this->renderHistoryLink($href); + } + + final public function linkTagHistory($tag) { + $href = $this->getDiffusionRequest()->generateURI( + array( + 'action' => 'history', + 'commit' => $tag, + )); + + return $this->renderHistoryLink($href); + } + + private function renderHistoryLink($href) { return javelin_tag( 'a', array( @@ -31,7 +55,7 @@ abstract class DiffusionView extends AphrontView { 'align' => 'E', ), ), - id(new PHUIIconView())->setIconFont('fa-list-ul blue')); + id(new PHUIIconView())->setIconFont('fa-history bluegrey')); } final public function linkBrowse($path, array $details = array()) { @@ -170,4 +194,58 @@ abstract class DiffusionView extends AphrontView { return hsprintf('%s', $name); } + final protected function renderBuildable(HarbormasterBuildable $buildable) { + $status = $buildable->getBuildableStatus(); + + $icon = HarbormasterBuildable::getBuildableStatusIcon($status); + $color = HarbormasterBuildable::getBuildableStatusColor($status); + $name = HarbormasterBuildable::getBuildableStatusName($status); + + $icon_view = id(new PHUIIconView()) + ->setIconFont($icon.' '.$color); + + $tooltip_view = javelin_tag( + 'span', + array( + 'sigil' => 'has-tooltip', + 'meta' => array('tip' => $name), + ), + $icon_view); + + Javelin::initBehavior('phabricator-tooltips'); + + return phutil_tag( + 'a', + array('href' => '/'.$buildable->getMonogram()), + $tooltip_view); + } + + final protected function loadBuildables(array $commits) { + assert_instances_of($commits, 'PhabricatorRepositoryCommit'); + + if (!$commits) { + return array(); + } + + $viewer = $this->getUser(); + + $harbormaster_app = 'PhabricatorHarbormasterApplication'; + $have_harbormaster = PhabricatorApplication::isClassInstalledForViewer( + $harbormaster_app, + $viewer); + + if ($have_harbormaster) { + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(mpull($commits, 'getPHID')) + ->withManualBuildables(false) + ->execute(); + $buildables = mpull($buildables, null, 'getBuildablePHID'); + } else { + $buildables = array(); + } + + return $buildables; + } + }