diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -367,7 +367,6 @@ 'DifferentialDiffQuery' => 'applications/differential/query/DifferentialDiffQuery.php', 'DifferentialDiffRepositoryHeraldField' => 'applications/differential/herald/DifferentialDiffRepositoryHeraldField.php', 'DifferentialDiffRepositoryProjectsHeraldField' => 'applications/differential/herald/DifferentialDiffRepositoryProjectsHeraldField.php', - 'DifferentialDiffTableOfContentsView' => 'applications/differential/view/DifferentialDiffTableOfContentsView.php', 'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php', 'DifferentialDiffTransaction' => 'applications/differential/storage/DifferentialDiffTransaction.php', 'DifferentialDiffTransactionQuery' => 'applications/differential/query/DifferentialDiffTransactionQuery.php', @@ -519,7 +518,6 @@ 'DiffusionCommitAutocloseHeraldField' => 'applications/diffusion/herald/DiffusionCommitAutocloseHeraldField.php', 'DiffusionCommitBranchesController' => 'applications/diffusion/controller/DiffusionCommitBranchesController.php', 'DiffusionCommitBranchesHeraldField' => 'applications/diffusion/herald/DiffusionCommitBranchesHeraldField.php', - 'DiffusionCommitChangeTableView' => 'applications/diffusion/view/DiffusionCommitChangeTableView.php', 'DiffusionCommitCommitterHeraldField' => 'applications/diffusion/herald/DiffusionCommitCommitterHeraldField.php', 'DiffusionCommitController' => 'applications/diffusion/controller/DiffusionCommitController.php', 'DiffusionCommitDiffContentAddedHeraldField' => 'applications/diffusion/herald/DiffusionCommitDiffContentAddedHeraldField.php', @@ -4006,7 +4004,6 @@ 'DifferentialDiffQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialDiffRepositoryHeraldField' => 'DifferentialDiffHeraldField', 'DifferentialDiffRepositoryProjectsHeraldField' => 'DifferentialDiffHeraldField', - 'DifferentialDiffTableOfContentsView' => 'AphrontView', 'DifferentialDiffTestCase' => 'PhutilTestCase', 'DifferentialDiffTransaction' => 'PhabricatorApplicationTransaction', 'DifferentialDiffTransactionQuery' => 'PhabricatorApplicationTransactionQuery', @@ -4178,7 +4175,6 @@ 'DiffusionCommitAutocloseHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitBranchesController' => 'DiffusionController', 'DiffusionCommitBranchesHeraldField' => 'DiffusionCommitHeraldField', - 'DiffusionCommitChangeTableView' => 'DiffusionView', 'DiffusionCommitCommitterHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitController' => 'DiffusionController', 'DiffusionCommitDiffContentAddedHeraldField' => 'DiffusionCommitHeraldField', diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -21,4 +21,33 @@ return $this->buildSideNavView(true)->getMenu(); } + protected function buildTableOfContents( + array $changesets, + array $visible_changesets, + array $coverage) { + $viewer = $this->getViewer(); + + $toc_view = id(new PHUIDiffTableOfContentsListView()) + ->setUser($viewer); + + foreach ($changesets as $changeset_id => $changeset) { + $is_visible = isset($visible_changesets[$changeset_id]); + $anchor = $changeset->getAnchorName(); + + $filename = $changeset->getFilename(); + $coverage_id = 'differential-mcoverage-'.md5($filename); + + $item = id(new PHUIDiffTableOfContentsItemView()) + ->setChangeset($changeset) + ->setIsVisible($is_visible) + ->setAnchor($anchor) + ->setCoverage(idx($coverage, $filename)) + ->setCoverageID($coverage_id); + + $toc_view->addItem($item); + } + + return $toc_view; + } + } diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -116,10 +116,10 @@ $changesets = $diff->loadChangesets(); $changesets = msort($changesets, 'getSortKey'); - $table_of_contents = id(new DifferentialDiffTableOfContentsView()) - ->setChangesets($changesets) - ->setVisibleChangesets($changesets) - ->setCoverageMap($diff->loadCoverageMap($viewer)); + $table_of_contents = $this->buildTableOfContents( + $changesets, + $changesets, + $diff->loadCoverageMap($viewer)); $refs = array(); foreach ($changesets as $changeset) { diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -1036,34 +1036,4 @@ return $view; } - private function buildTableOfContents( - array $changesets, - array $visible_changesets, - array $coverage) { - $viewer = $this->getViewer(); - - $toc_view = id(new PHUIDiffTableOfContentsListView()) - ->setUser($viewer); - - foreach ($changesets as $changeset_id => $changeset) { - $is_visible = isset($visible_changesets[$changeset_id]); - $anchor = $changeset->getAnchorName(); - - $filename = $changeset->getFilename(); - $coverage_id = 'differential-mcoverage-'.md5($filename); - - $item = id(new PHUIDiffTableOfContentsItemView()) - ->setChangeset($changeset) - ->setIsVisible($is_visible) - ->setAnchor($anchor) - ->setCoverage(idx($coverage, $filename)) - ->setCoverageID($coverage_id); - - $toc_view->addItem($item); - } - - return $toc_view; - } - - } diff --git a/src/applications/differential/view/DifferentialDiffTableOfContentsView.php b/src/applications/differential/view/DifferentialDiffTableOfContentsView.php deleted file mode 100644 --- a/src/applications/differential/view/DifferentialDiffTableOfContentsView.php +++ /dev/null @@ -1,300 +0,0 @@ -changesets = $changesets; - return $this; - } - - public function setVisibleChangesets($visible_changesets) { - $this->visibleChangesets = $visible_changesets; - return $this; - } - - public function setRenderingReferences(array $references) { - $this->references = $references; - return $this; - } - - public function setRepository(PhabricatorRepository $repository) { - $this->repository = $repository; - return $this; - } - - public function setDiff(DifferentialDiff $diff) { - $this->diff = $diff; - return $this; - } - - public function setCoverageMap(array $coverage_map) { - $this->coverageMap = $coverage_map; - return $this; - } - - public function render() { - - $this->requireResource('differential-core-view-css'); - $this->requireResource('differential-table-of-contents-css'); - $this->requireResource('phui-text-css'); - - $rows = array(); - - $changesets = $this->changesets; - $paths = array(); - foreach ($changesets as $id => $changeset) { - $type = $changeset->getChangeType(); - $ftype = $changeset->getFileType(); - $ref = idx($this->references, $id); - $display_file = $changeset->getDisplayFilename(); - - $meta = null; - if (DifferentialChangeType::isOldLocationChangeType($type)) { - $away = $changeset->getAwayPaths(); - if (count($away) > 1) { - $meta = array(); - if ($type == DifferentialChangeType::TYPE_MULTICOPY) { - $meta[] = pht('Deleted after being copied to multiple locations:'); - } else { - $meta[] = pht('Copied to multiple locations:'); - } - foreach ($away as $path) { - $meta[] = $path; - } - $meta = phutil_implode_html(phutil_tag('br'), $meta); - } else { - if ($type == DifferentialChangeType::TYPE_MOVE_AWAY) { - $display_file = $this->renderRename( - $display_file, - reset($away), - "\xE2\x86\x92"); - } else { - $meta = pht('Copied to %s', reset($away)); - } - } - } else if ($type == DifferentialChangeType::TYPE_MOVE_HERE) { - $old_file = $changeset->getOldFile(); - $display_file = $this->renderRename( - $display_file, - $old_file, - "\xE2\x86\x90"); - } else if ($type == DifferentialChangeType::TYPE_COPY_HERE) { - $meta = pht('Copied from %s', $changeset->getOldFile()); - } - - $link = $this->renderChangesetLink($changeset, $ref, $display_file); - - $line_count = $changeset->getAffectedLineCount(); - if ($line_count == 0) { - $lines = ''; - } else { - $lines = ' '.pht('(%d line(s))', $line_count); - } - - $char = DifferentialChangeType::getSummaryCharacterForChangeType($type); - $chartitle = DifferentialChangeType::getFullNameForChangeType($type); - $desc = DifferentialChangeType::getShortNameForFileType($ftype); - $color = DifferentialChangeType::getSummaryColorForChangeType($type); - if ($desc) { - $desc = '('.$desc.')'; - } - $pchar = - ($changeset->getOldProperties() === $changeset->getNewProperties()) - ? '' - : phutil_tag( - 'span', - array('title' => pht('Properties Changed')), - 'M'); - - $fname = $changeset->getFilename(); - $cov = $this->renderCoverage($this->coverageMap, $fname); - if ($cov === null) { - $mcov = $cov = phutil_tag('em', array(), '-'); - } else { - $mcov = phutil_tag( - 'div', - array( - 'id' => 'differential-mcoverage-'.md5($fname), - 'class' => 'differential-mcoverage-loading', - ), - (isset($this->visibleChangesets[$id]) ? - pht('Loading...') : pht('?'))); - } - - if ($meta) { - $meta = phutil_tag( - 'div', - array( - 'class' => 'differential-toc-meta', - ), - $meta); - } - - if ($this->diff && $this->repository) { - $paths[] = - $changeset->getAbsoluteRepositoryPath($this->repository, $this->diff); - } - - $char = phutil_tag('span', array('class' => 'phui-text-'.$color), $char); - - $rows[] = array( - $char, - $pchar, - $desc, - array($link, $lines, $meta), - $cov, - $mcov, - ); - } - - $editor_link = null; - if ($paths && $this->user) { - $editor_link = $this->user->loadEditorLink( - $paths, - 1, // line number - $this->repository->getCallsign()); - if ($editor_link) { - $editor_link = - phutil_tag( - 'a', - array( - 'href' => $editor_link, - 'class' => 'button differential-toc-edit-all', - ), - pht('Open All in Editor')); - } - } - - $reveal_link = javelin_tag( - 'a', - array( - 'sigil' => 'differential-reveal-all', - 'mustcapture' => true, - 'class' => 'button differential-toc-reveal-all', - ), - pht('Show All Context')); - - $buttons = phutil_tag( - 'div', - array( - 'class' => 'differential-toc-buttons grouped', - ), - array( - $editor_link, - $reveal_link, - )); - - $table = id(new AphrontTableView($rows)); - $table->setHeaders( - array( - '', - '', - '', - pht('Path'), - pht('Coverage (All)'), - pht('Coverage (Touched)'), - )); - $table->setColumnClasses( - array( - 'differential-toc-char center', - 'differential-toc-prop center', - 'differential-toc-ftype center', - 'differential-toc-file wide', - 'differential-toc-cov', - 'differential-toc-cov', - )); - $table->setDeviceVisibility( - array( - true, - true, - true, - true, - false, - false, - )); - $anchor = id(new PhabricatorAnchorView()) - ->setAnchorName('toc') - ->setNavigationMarker(true); - - return id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Table of Contents')) - ->setTable($table) - ->appendChild($anchor) - ->appendChild($buttons); - } - - private function renderRename($display_file, $other_file, $arrow) { - $old = explode('/', $display_file); - $new = explode('/', $other_file); - - $start = count($old); - foreach ($old as $index => $part) { - if (!isset($new[$index]) || $part != $new[$index]) { - $start = $index; - break; - } - } - - $end = count($old); - foreach (array_reverse($old) as $from_end => $part) { - $index = count($new) - $from_end - 1; - if (!isset($new[$index]) || $part != $new[$index]) { - $end = $from_end; - break; - } - } - - $rename = - '{'. - implode('/', array_slice($old, $start, count($old) - $end - $start)). - ' '.$arrow.' '. - implode('/', array_slice($new, $start, count($new) - $end - $start)). - '}'; - - array_splice($new, $start, count($new) - $end - $start, $rename); - return implode('/', $new); - } - - private function renderCoverage(array $coverage, $file) { - $info = idx($coverage, $file); - if (!$info) { - return null; - } - - $not_covered = substr_count($info, 'U'); - $covered = substr_count($info, 'C'); - - if (!$not_covered && !$covered) { - return null; - } - - return sprintf('%d%%', 100 * ($covered / ($covered + $not_covered))); - } - - - private function renderChangesetLink( - DifferentialChangeset $changeset, - $ref, - $display_file) { - - return javelin_tag( - 'a', - array( - 'href' => '#'.$changeset->getAnchorName(), - 'sigil' => 'differential-load', - 'meta' => array( - 'id' => 'diff-'.$changeset->getAnchorName(), - ), - ), - $display_file); - } - -} diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -75,7 +75,6 @@ $commit_data = $commit->getCommitData(); $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); - $changesets = null; if ($is_foreign) { $subpath = $commit_data->getCommitDetail('svn-subpath'); @@ -181,24 +180,6 @@ $user, $this->auditAuthorityPHIDs); - $owners_paths = array(); - if ($highlighted_audits) { - $packages = id(new PhabricatorOwnersPackage())->loadAllWhere( - 'phid IN (%Ls)', - mpull($highlighted_audits, 'getAuditorPHID')); - if ($packages) { - $owners_paths = id(new PhabricatorOwnersPath())->loadAllWhere( - 'repositoryPHID = %s AND packageID IN (%Ld)', - $repository->getPHID(), - mpull($packages, 'getID')); - } - } - - $change_table = new DiffusionCommitChangeTableView(); - $change_table->setDiffusionRequest($drequest); - $change_table->setPathChanges($changes); - $change_table->setOwnersPaths($owners_paths); - $count = count($changes); $bad_commit = null; @@ -210,6 +191,7 @@ 'r'.$callsign.$commit->getCommitIdentifier()); } + $show_changesets = false; if ($bad_commit) { $content[] = $this->renderStatusMessage( pht('Bad Commit'), @@ -235,6 +217,8 @@ 'Changes are not shown.', $hard_limit)); } else { + $show_changesets = true; + // The user has clicked "Show All Changes", and we should show all the // changes inline even if there are more than the soft limit. $show_all_details = $request->getBool('show_all'); @@ -264,15 +248,20 @@ $header->addActionLink($button); } + $changesets = DiffusionPathChange::convertToDifferentialChangesets( + $user, + $changes); + + // TODO: This table and panel shouldn't really be separate, but we need + // to clean up the "Load All Files" interaction first. + $change_table = $this->buildTableOfContents( + $changesets); + $change_panel->setTable($change_table); $change_panel->setHeader($header); $content[] = $change_panel; - $changesets = DiffusionPathChange::convertToDifferentialChangesets( - $user, - $changes); - $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: @@ -353,12 +342,6 @@ $change_list->setInlineCommentControllerURI( '/diffusion/inline/edit/'.phutil_escape_uri($commit->getPHID()).'/'); - $change_references = array(); - foreach ($changesets as $key => $changeset) { - $change_references[$changeset->getID()] = $references[$key]; - } - $change_table->setRenderingReferences($change_references); - $content[] = $change_list->render(); } @@ -375,7 +358,7 @@ $show_filetree = $prefs->getPreference($pref_filetree); $collapsed = $prefs->getPreference($pref_collapse); - if ($changesets && $show_filetree) { + if ($show_changesets && $show_filetree) { $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle($short_name) ->setBaseURI(new PhutilURI('/'.$commit_id)) @@ -1082,4 +1065,40 @@ return $parser->processCorpus($corpus); } + private function buildTableOfContents(array $changesets) { + $viewer = $this->getViewer(); + + $toc_view = id(new PHUIDiffTableOfContentsListView()) + ->setUser($viewer); + + // TODO: This is hacky, we just want access to the linkX() methods on + // DiffusionView. + $diffusion_view = id(new DiffusionEmptyResultView()) + ->setDiffusionRequest($this->getDiffusionRequest()); + + // TODO: Restore package stuff here. + + foreach ($changesets as $changeset_id => $changeset) { + $path = $changeset->getFilename(); + $anchor = substr(md5($path), 0, 8); + + $history_link = $diffusion_view->linkHistory($path); + $browse_link = $diffusion_view->linkBrowse($path); + + $item = id(new PHUIDiffTableOfContentsItemView()) + ->setChangeset($changeset) + ->setAnchor($anchor) + ->setContext( + array( + $history_link, + ' ', + $browse_link, + )); + + $toc_view->addItem($item); + } + + return $toc_view; + } + } diff --git a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php b/src/applications/diffusion/view/DiffusionCommitChangeTableView.php deleted file mode 100644 --- a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php +++ /dev/null @@ -1,103 +0,0 @@ -pathChanges = $path_changes; - return $this; - } - - public function setOwnersPaths(array $owners_paths) { - assert_instances_of($owners_paths, 'PhabricatorOwnersPath'); - $this->ownersPaths = $owners_paths; - return $this; - } - - public function setRenderingReferences(array $value) { - $this->renderingReferences = $value; - return $this; - } - - public function render() { - $rows = array(); - $rowc = array(); - - // TODO: Experiment with path stack rendering. - - // TODO: Copy Away and Move Away are rendered junkily still. - - foreach ($this->pathChanges as $id => $change) { - $path = $change->getPath(); - $hash = substr(md5($path), 0, 8); - if ($change->getFileType() == DifferentialChangeType::FILE_DIRECTORY) { - $path .= '/'; - } - - if (isset($this->renderingReferences[$id])) { - $path_column = javelin_tag( - 'a', - array( - 'href' => '#'.$hash, - 'meta' => array( - 'id' => 'diff-'.$hash, - 'ref' => $this->renderingReferences[$id], - ), - 'sigil' => 'differential-load', - ), - $path); - } else { - $path_column = $path; - } - - $rows[] = array( - $this->linkHistory($change->getPath()), - $this->linkBrowse($change->getPath()), - $this->linkChange( - $change->getChangeType(), - $change->getFileType(), - $change->getPath()), - $path_column, - ); - - $row_class = null; - foreach ($this->ownersPaths as $owners_path) { - $excluded = $owners_path->getExcluded(); - $owners_path = $owners_path->getPath(); - if (strncmp('/'.$path, $owners_path, strlen($owners_path)) == 0) { - if ($excluded) { - $row_class = null; - break; - } - $row_class = 'highlighted'; - } - } - $rowc[] = $row_class; - } - - $view = new AphrontTableView($rows); - $view->setHeaders( - array( - pht('History'), - pht('Browse'), - pht('Change'), - pht('Path'), - )); - $view->setColumnClasses( - array( - '', - '', - '', - 'wide', - )); - $view->setRowClasses($rowc); - $view->setNoDataString(pht('This change has not been fully parsed yet.')); - - return $view->render(); - } - -} diff --git a/src/infrastructure/diff/view/PHUIDiffTableOfContentsItemView.php b/src/infrastructure/diff/view/PHUIDiffTableOfContentsItemView.php --- a/src/infrastructure/diff/view/PHUIDiffTableOfContentsItemView.php +++ b/src/infrastructure/diff/view/PHUIDiffTableOfContentsItemView.php @@ -3,10 +3,12 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView { private $changeset; - private $isVisible; + private $isVisible = true; private $anchor; private $coverage; private $coverageID; + private $context; + private $package; public function setChangeset(DifferentialChangeset $changeset) { $this->changeset = $changeset; @@ -53,11 +55,31 @@ return $this->coverageID; } + public function setContext($context) { + $this->context = $context; + return $this; + } + + public function getContext() { + return $this->context; + } + + public function setPackage(PhabricatorOwnersPackage $package) { + $this->package = $package; + return $this; + } + + public function getPackage() { + return $this->package; + } + public function render() { $changeset = $this->getChangeset(); $cells = array(); + $cells[] = $this->getContext(); + $cells[] = $this->renderPathChangeCharacter(); $cells[] = $this->renderPropertyChangeCharacter(); $cells[] = $this->renderPropertyChangeDescription(); @@ -75,6 +97,8 @@ $cells[] = $this->renderCoverage(); $cells[] = $this->renderModifiedCoverage(); + $cells[] = $this->renderPackage(); + return $cells; } @@ -89,7 +113,7 @@ return javelin_tag( 'span', array( - 'sigil' => 'has-tip', + 'sigil' => 'has-tooltip', 'meta' => array( 'tip' => $title, 'align' => 'E', @@ -112,10 +136,11 @@ return javelin_tag( 'span', array( - 'sigil' => 'has-tip', + 'sigil' => 'has-tooltip', 'meta' => array( 'tip' => pht('Properties Modified'), 'align' => 'E', + 'size' => 200, ), ), 'M'); @@ -259,6 +284,16 @@ $meta); } + private function renderPackage() { + $package = $this->getPackage(); + + if (!$package) { + return null; + } + + return $this->getUser()->renderHandle($package->getPHID()); + } + private function renderRename($self, $other, $arrow) { $old = explode('/', $self); $new = explode('/', $other); diff --git a/src/infrastructure/diff/view/PHUIDiffTableOfContentsListView.php b/src/infrastructure/diff/view/PHUIDiffTableOfContentsListView.php --- a/src/infrastructure/diff/view/PHUIDiffTableOfContentsListView.php +++ b/src/infrastructure/diff/view/PHUIDiffTableOfContentsListView.php @@ -14,13 +14,35 @@ $this->requireResource('differential-table-of-contents-css'); $this->requireResource('phui-text-css'); + Javelin::initBehavior('phabricator-tooltips'); + $items = $this->items; $rows = array(); foreach ($items as $item) { + $item->setUser($this->getUser()); $rows[] = $item->render(); } + // Check if any item has content in these columns. If no item does, we'll + // just hide them. + $any_coverage = false; + $any_context = false; + $any_package = false; + foreach ($items as $item) { + if ($item->getContext() !== null) { + $any_context = true; + } + + if (strlen($item->getCoverage())) { + $any_coverage = true; + } + + if ($item->getPackage() !== null) { + $any_package = true; + } + } + $reveal_link = javelin_tag( 'a', array( @@ -40,21 +62,36 @@ $table = id(new AphrontTableView($rows)) ->setHeaders( array( - '', - '', - '', + null, + null, + null, + null, pht('Path'), pht('Coverage (All)'), pht('Coverage (Touched)'), + null, )) ->setColumnClasses( array( + 'center', 'differential-toc-char center', 'differential-toc-prop center', 'differential-toc-ftype center', 'differential-toc-file wide', 'differential-toc-cov', 'differential-toc-cov', + 'center', + )) + ->setColumnVisibility( + array( + $any_context, + true, + true, + true, + true, + $any_coverage, + $any_coverage, + $any_package, )) ->setDeviceVisibility( array( @@ -62,8 +99,10 @@ true, true, true, + true, false, false, + true, )); $anchor = id(new PhabricatorAnchorView())