From 9e8387175e2e44d37bb99a95373640cebe01900e Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 12 Dec 2012 17:50:42 -0800 Subject: [PATCH] upgrade diffusion to use modern header UI and fix a few quirks Summary: upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs. Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no, Test Plan: played around in diffusion and differential Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin, chad Maniphest Tasks: T2048, T2178 Differential Revision: https://secure.phabricator.com/D4169 --- src/__phutil_library_map__.php | 4 +- .../DifferentialRevisionViewController.php | 109 +----------- ...rentialChangesetFileTreeSideNavBuilder.php | 134 +++++++++++++++ .../view/DifferentialRevisionDetailView.php | 2 +- .../controller/DiffusionBrowseController.php | 18 +- .../DiffusionBrowseFileController.php | 15 +- .../controller/DiffusionChangeController.php | 16 +- .../controller/DiffusionCommitController.php | 162 ++++++++++-------- .../controller/DiffusionController.php | 104 +++++------ .../controller/DiffusionHistoryController.php | 16 +- .../controller/DiffusionLintController.php | 23 +-- .../DiffusionLintDetailsController.php | 30 ++-- .../diffusion/view/DiffusionView.php | 14 +- src/view/layout/PhabricatorCrumbView.php | 17 +- .../rsrc/css/aphront/phabricator-nav-view.css | 2 +- 15 files changed, 381 insertions(+), 285 deletions(-) create mode 100644 src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 07fe9c24c4..a48e51ad0b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -224,6 +224,7 @@ phutil_register_library_map(array( 'DifferentialChangeType' => 'applications/differential/constants/DifferentialChangeType.php', 'DifferentialChangeset' => 'applications/differential/storage/DifferentialChangeset.php', 'DifferentialChangesetDetailView' => 'applications/differential/view/DifferentialChangesetDetailView.php', + 'DifferentialChangesetFileTreeSideNavBuilder' => 'applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php', 'DifferentialChangesetListView' => 'applications/differential/view/DifferentialChangesetListView.php', 'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php', 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', @@ -2570,12 +2571,13 @@ phutil_register_library_map(array( 1 => 'PhabricatorMarkupInterface', 2 => 'PonderVotableInterface', 3 => 'PhabricatorSubscribableInterface', + 4 => 'PhabricatorPolicyInterface', ), 'PonderQuestionAskController' => 'PonderController', 'PonderQuestionDetailView' => 'AphrontView', 'PonderQuestionEditor' => 'PhabricatorEditor', 'PonderQuestionPreviewController' => 'PonderController', - 'PonderQuestionQuery' => 'PhabricatorCursorPagedPolicyQuery', + 'PonderQuestionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PonderQuestionSummaryView' => 'AphrontView', 'PonderQuestionViewController' => 'PonderController', 'PonderReplyHandler' => 'PhabricatorMailReplyHandler', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 72a6e9f2ee..54498cb9c0 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -398,12 +398,17 @@ final class DifferentialRevisionViewController extends DifferentialController { PhabricatorFeedStoryNotification::updateObjectNotificationViews( $user, $revision->getPHID()); + $object_id = 'D'.$revision->getID(); + $top_anchor = id(new PhabricatorAnchorView()) ->setAnchorName('top') ->setNavigationMarker(true); - $nav = $this->buildSideNavView($revision, $changesets); - $nav->selectFilter(''); + $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) + ->setAnchorName('top') + ->setTitle('D'.$revision->getID()) + ->setBaseURI(new PhutilURI('/D'.$revision->getID())) + ->build($changesets); $nav->appendChild( array( $reviewer_warning, @@ -412,7 +417,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $page_pane, )); - $object_id = 'D'.$revision->getID(); $crumbs = $this->buildApplicationCrumbs(); $crumbs->addCrumb( @@ -960,103 +964,4 @@ final class DifferentialRevisionViewController extends DifferentialController { return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } - - private function buildSideNavView( - DifferentialRevision $revision, - array $changesets) { - - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/D'.$revision->getID())); - $nav->setFlexible(true); - - $nav->addFilter('top', 'D'.$revision->getID(), '#top'); - - $tree = new PhutilFileTree(); - foreach ($changesets as $changeset) { - try { - $tree->addPath($changeset->getFilename(), $changeset); - } catch (Exception $ex) { - // TODO: See T1702. When viewing the versus diff of diffs, we may - // have files with the same filename. For example, if you have a setup - // like this in SVN: - // - // a/ - // README - // b/ - // README - // - // ...and you run "arc diff" once from a/, and again from b/, you'll - // get two diffs with path README. However, in the versus diff view we - // will compute their absolute repository paths and detect that they - // aren't really the same file. This is correct, but causes us to - // throw when inserting them. - // - // We should probably compute the smallest unique path for each file - // and show these as "a/README" and "b/README" when diffed against - // one another. However, we get this wrong in a lot of places (the - // other TOC shows two "README" files, and we generate the same anchor - // hash for both) so I'm just stopping the bleeding until we can get - // a proper fix in place. - } - } - - require_celerity_resource('phabricator-filetree-view-css'); - - $filetree = array(); - - $path = $tree; - while (($path = $path->getNextNode())) { - $data = $path->getData(); - - $name = $path->getName(); - $style = 'padding-left: '.(2 + (3 * $path->getDepth())).'px'; - - $href = null; - if ($data) { - $href = '#'.$data->getAnchorName(); - $title = $name; - $icon = 'phabricator-filetree-icon-file'; - } else { - $name .= '/'; - $title = $path->getFullPath().'/'; - $icon = 'phabricator-filetree-icon-dir'; - } - - $icon = phutil_render_tag( - 'span', - array( - 'class' => 'phabricator-filetree-icon '.$icon, - ), - ''); - - $name_element = phutil_render_tag( - 'span', - array( - 'class' => 'phabricator-filetree-name', - ), - phutil_escape_html($name)); - - $filetree[] = javelin_render_tag( - $href ? 'a' : 'span', - array( - 'href' => $href, - 'style' => $style, - 'title' => $title, - 'class' => 'phabricator-filetree-item', - ), - $icon.$name_element); - } - $tree->destroy(); - - $filetree = - '
'. - implode("\n", $filetree). - '
'; - $nav->addFilter('toc', 'Table of Contents', '#toc'); - $nav->addCustomBlock($filetree); - $nav->addFilter('comment', 'Add Comment', '#comment'); - $nav->setActive(true); - - return $nav; - } } diff --git a/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php new file mode 100644 index 0000000000..f9d4fa1662 --- /dev/null +++ b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php @@ -0,0 +1,134 @@ +anchorName = $anchor_name; + return $this; + } + public function getAnchorName() { + return $this->anchorName; + } + + public function setBaseURI(PhutilURI $base_uri) { + $this->baseURI = $base_uri; + return $this; + } + public function getBaseURI() { + return $this->baseURI; + } + + public function setTitle($title) { + $this->title = $title; + return $this; + } + public function getTitle() { + return $this->title; + } + + public function build(array $changesets) { + assert_instances_of($changesets, 'DifferentialChangeset'); + + $nav = new AphrontSideNavFilterView(); + $nav->setBaseURI($this->getBaseURI()); + $nav->setFlexible(true); + + $anchor = $this->getAnchorName(); + $nav->addFilter($anchor, $this->getTitle(), '#'.$anchor); + + $tree = new PhutilFileTree(); + foreach ($changesets as $changeset) { + try { + $tree->addPath($changeset->getFilename(), $changeset); + } catch (Exception $ex) { + // TODO: See T1702. When viewing the versus diff of diffs, we may + // have files with the same filename. For example, if you have a setup + // like this in SVN: + // + // a/ + // README + // b/ + // README + // + // ...and you run "arc diff" once from a/, and again from b/, you'll + // get two diffs with path README. However, in the versus diff view we + // will compute their absolute repository paths and detect that they + // aren't really the same file. This is correct, but causes us to + // throw when inserting them. + // + // We should probably compute the smallest unique path for each file + // and show these as "a/README" and "b/README" when diffed against + // one another. However, we get this wrong in a lot of places (the + // other TOC shows two "README" files, and we generate the same anchor + // hash for both) so I'm just stopping the bleeding until we can get + // a proper fix in place. + } + } + + require_celerity_resource('phabricator-filetree-view-css'); + + $filetree = array(); + + $path = $tree; + while (($path = $path->getNextNode())) { + $data = $path->getData(); + + $name = $path->getName(); + $style = 'padding-left: '.(2 + (3 * $path->getDepth())).'px'; + + $href = null; + if ($data) { + $href = '#'.$data->getAnchorName(); + $title = $name; + $icon = 'phabricator-filetree-icon-file'; + } else { + $name .= '/'; + $title = $path->getFullPath().'/'; + $icon = 'phabricator-filetree-icon-dir'; + } + + $icon = phutil_render_tag( + 'span', + array( + 'class' => 'phabricator-filetree-icon '.$icon, + ), + ''); + + $name_element = phutil_render_tag( + 'span', + array( + 'class' => 'phabricator-filetree-name', + ), + phutil_escape_html($name)); + + $filetree[] = javelin_render_tag( + $href ? 'a' : 'span', + array( + 'href' => $href, + 'style' => $style, + 'title' => $title, + 'class' => 'phabricator-filetree-item', + ), + $icon.$name_element); + } + $tree->destroy(); + + $filetree = + '
'. + implode("\n", $filetree). + '
'; + $nav->addFilter('toc', 'Table of Contents', '#toc'); + $nav->addCustomBlock($filetree); + $nav->addFilter('comment', 'Add Comment', '#comment'); + $nav->setActive(true); + $nav->selectFilter(''); + + return $nav; + } + +} + diff --git a/src/applications/differential/view/DifferentialRevisionDetailView.php b/src/applications/differential/view/DifferentialRevisionDetailView.php index 5e9b966ebe..ee3e6f7c3b 100644 --- a/src/applications/differential/view/DifferentialRevisionDetailView.php +++ b/src/applications/differential/view/DifferentialRevisionDetailView.php @@ -83,7 +83,7 @@ final class DifferentialRevisionDetailView extends AphrontView { } } if ($next_step) { - $properties->addProperty('Next Step:', $next_step); + $properties->addProperty('Next Step', $next_step); } foreach ($this->auxiliaryFields as $field) { diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index a1d1e24b78..9ed36ea8c0 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -10,13 +10,6 @@ final class DiffusionBrowseController extends DiffusionController { $content = array(); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'browse', - )); - if ($drequest->getTagContent()) { $title = 'Tag: '.$drequest->getSymbolicCommit(); @@ -34,6 +27,7 @@ final class DiffusionBrowseController extends DiffusionController { DiffusionBrowseQuery::REASON_IS_FILE) { $controller = new DiffusionBrowseFileController($this->getRequest()); $controller->setDiffusionRequest($drequest); + $controller->setCurrentApplication($this->getCurrentApplication()); return $this->delegateToController($controller); } @@ -85,7 +79,15 @@ final class DiffusionBrowseController extends DiffusionController { $nav = $this->buildSideNav('browse', false); $nav->appendChild($content); - return $this->buildStandardPageResponse( + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'browse', + )); + $nav->setCrumbs($crumbs); + + return $this->buildApplicationPage( $nav, array( 'title' => array( diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index a62b62d7b0..3729ce9c60 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -69,12 +69,6 @@ final class DiffusionBrowseFileController extends DiffusionController { // Render the page. $content = array(); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'browse', - )); $follow = $request->getStr('follow'); if ($follow) { @@ -114,10 +108,17 @@ final class DiffusionBrowseFileController extends DiffusionController { $nav = $this->buildSideNav('browse', true); $nav->appendChild($content); + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'browse', + )); + $nav->setCrumbs($crumbs); $basename = basename($this->getDiffusionRequest()->getPath()); - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $nav, array( 'title' => $basename, diff --git a/src/applications/diffusion/controller/DiffusionChangeController.php b/src/applications/diffusion/controller/DiffusionChangeController.php index f09786769d..cabe90feb0 100644 --- a/src/applications/diffusion/controller/DiffusionChangeController.php +++ b/src/applications/diffusion/controller/DiffusionChangeController.php @@ -45,13 +45,6 @@ final class DiffusionChangeController extends DiffusionController { DifferentialChangesetParser::WHITESPACE_SHOW_ALL); $changeset_view->setUser($this->getRequest()->getUser()); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'change', - )); - // TODO: This is pretty awkward, unify the CSS between Diffusion and // Differential better. require_celerity_resource('differential-core-view-css'); @@ -62,8 +55,15 @@ final class DiffusionChangeController extends DiffusionController { $nav = $this->buildSideNav('change', true); $nav->appendChild($content); + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'change', + )); + $nav->setCrumbs($crumbs); - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $nav, array( 'title' => 'Change', diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 0d654a29e7..1d91f059d9 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -26,10 +26,6 @@ final class DiffusionCommitController extends DiffusionController { $callsign = $drequest->getRepository()->getCallsign(); $content = array(); - $content[] = $this->buildCrumbs(array( - 'commit' => true, - )); - $repository = $drequest->getRepository(); $commit = $drequest->loadCommit(); @@ -51,6 +47,10 @@ final class DiffusionCommitController extends DiffusionController { $commit_data = $drequest->loadCommitData(); $commit->attachCommitData($commit_data); + $top_anchor = id(new PhabricatorAnchorView()) + ->setAnchorName('top') + ->setNavigationMarker(true); + $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); if ($is_foreign) { $subpath = $commit_data->getCommitDetail('svn-subpath'); @@ -64,6 +64,7 @@ final class DiffusionCommitController extends DiffusionController { "didn't affect the tracked subdirectory ('". phutil_escape_html($subpath)."'), so no information is available."); $content[] = $error_panel; + $content[] = $top_anchor; } else { $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(); @@ -73,22 +74,32 @@ final class DiffusionCommitController extends DiffusionController { $parent_query = DiffusionCommitParentsQuery::newFromDiffusionRequest( $drequest); - $headsup_panel = new AphrontHeadsupView(); - $headsup_panel->setHeader('Commit Detail'); - $headsup_panel->setActionList( - $this->renderHeadsupActionList($commit, $repository)); - $headsup_panel->setProperties( - $this->getCommitProperties( - $commit, - $commit_data, - $parent_query->loadParents())); + $headsup_view = id(new PhabricatorHeaderView()) + ->setHeader('Commit Detail'); - $headsup_panel->appendChild( + $headsup_actions = $this->renderHeadsupActionList($commit, $repository); + + $commit_properties = $this->loadCommitProperties( + $commit, + $commit_data, + $parent_query->loadParents() + ); + $property_list = id(new PhabricatorPropertyListView()) + ->setHasKeyboardShortcuts(true); + foreach ($commit_properties as $key => $value) { + $property_list->addProperty($key, $value); + } + + $property_list->addTextContent( '
'. - $engine->markupText($commit_data->getCommitMessage()). - '
'); + $engine->markupText($commit_data->getCommitMessage()). + '' + ); - $content[] = $headsup_panel; + $content[] = $top_anchor; + $content[] = $headsup_view; + $content[] = $headsup_actions; + $content[] = $property_list; } $query = new PhabricatorAuditQuery(); @@ -289,19 +300,43 @@ final class DiffusionCommitController extends DiffusionController { 'id' => $pane_id ), $change_list->render(). + id(new PhabricatorAnchorView()) + ->setAnchorName('comment') + ->setNavigationMarker(true) + ->render(). $add_comment_view); $content[] = $main_pane; } - return $this->buildStandardPageResponse( - $content, + $commit_id = 'r'.$callsign.$commit->getCommitIdentifier(); + $short_name = DiffusionView::nameCommit( + $repository, + $commit->getCommitIdentifier() + ); + $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) + ->setAnchorName('top') + ->setTitle($short_name) + ->setBaseURI(new PhutilURI('/'.$commit_id)) + ->build($changesets); + foreach ($content as $child) { + $nav->appendChild($child); + } + + $crumbs = $this->buildCrumbs(array( + 'commit' => true, + )); + $nav->setCrumbs($crumbs); + + return $this->buildApplicationPage( + $nav, array( - 'title' => 'r'.$callsign.$commit->getCommitIdentifier(), - )); + 'title' => $commit_id + ) + ); } - private function getCommitProperties( + private function loadCommitProperties( PhabricatorRepositoryCommit $commit, PhabricatorRepositoryCommitData $data, array $parents) { @@ -766,75 +801,54 @@ final class DiffusionCommitController extends DiffusionController { $request = $this->getRequest(); $user = $request->getUser(); - $actions = array(); + $actions = id(new PhabricatorActionListView()) + ->setUser($user) + ->setObject($commit); // TODO -- integrate permissions into whether or not this action is shown $uri = '/diffusion/'.$repository->getCallSign().'/commit/'. $commit->getCommitIdentifier().'/edit/'; - $action = new AphrontHeadsupActionView(); - $action->setClass('action-edit'); - $action->setURI($uri); - $action->setName('Edit Commit'); - $action->setWorkflow(false); - $actions[] = $action; - require_celerity_resource('phabricator-flag-css'); - $flag = PhabricatorFlagQuery::loadUserFlag($user, $commit->getPHID()); - if ($flag) { - $class = PhabricatorFlagColor::getCSSClass($flag->getColor()); - $color = PhabricatorFlagColor::getColorName($flag->getColor()); - - $action = new AphrontHeadsupActionView(); - $action->setClass('flag-clear '.$class); - $action->setURI('/flag/delete/'.$flag->getID().'/'); - $action->setName('Remove '.$color.' Flag'); - $action->setWorkflow(true); - $actions[] = $action; - } else { - $action = new AphrontHeadsupActionView(); - $action->setClass('phabricator-flag-ghost'); - $action->setURI('/flag/edit/'.$commit->getPHID().'/'); - $action->setName('Flag Commit'); - $action->setWorkflow(true); - $actions[] = $action; - } + $action = id(new PhabricatorActionView()) + ->setName('Edit Commit') + ->setHref($uri) + ->setIcon('edit'); + $actions->addAction($action); require_celerity_resource('phabricator-object-selector-css'); require_celerity_resource('javelin-behavior-phabricator-object-selector'); if (PhabricatorEnv::getEnvConfig('maniphest.enabled')) { - $action = new AphrontHeadsupActionView(); - $action->setName('Edit Maniphest Tasks'); - $action->setURI('/search/attach/'.$commit->getPHID().'/TASK/edge/'); - $action->setWorkflow(true); - $action->setClass('attach-maniphest'); - $actions[] = $action; + $action = id(new PhabricatorActionView()) + ->setName('Edit Maniphest Tasks') + ->setIcon('attach') + ->setHref('/search/attach/'.$commit->getPHID().'/TASK/edge/') + ->setWorkflow(true); + $actions->addAction($action); } if ($user->getIsAdmin()) { - $action = new AphrontHeadsupActionView(); - $action->setName('MetaMTA Transcripts'); - $action->setURI('/mail/?phid='.$commit->getPHID()); - $action->setClass('transcripts-metamta'); - $actions[] = $action; + $action = id(new PhabricatorActionView()) + ->setName('MetaMTA Transcripts') + ->setIcon('file') + ->setHref('/mail/?phid='.$commit->getPHID()); + $actions->addAction($action); } - $action = new AphrontHeadsupActionView(); - $action->setName('Herald Transcripts'); - $action->setURI('/herald/transcript/?phid='.$commit->getPHID()); - $action->setClass('transcripts-herald'); - $actions[] = $action; + $action = id(new PhabricatorActionView()) + ->setName('Herald Transcripts') + ->setIcon('file') + ->setHref('/herald/transcript/?phid='.$commit->getPHID()) + ->setWorkflow(true); + $actions->addAction($action); - $action = new AphrontHeadsupActionView(); - $action->setName('Download Raw Diff'); - $action->setURI($request->getRequestURI()->alter('diff', true)); - $action->setClass('action-download'); - $actions[] = $action; + $action = id(new PhabricatorActionView()) + ->setName('Download Raw Diff') + ->setHref($request->getRequestURI()->alter('diff', true)) + ->setIcon('download'); + $actions->addAction($action); - $action_list = new AphrontHeadsupActionListView(); - $action_list->setActions($actions); - - return $action_list; + return $actions; } private function buildRefs(DiffusionRequest $request) { diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 2c16ab2246..08fffae287 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -72,7 +72,7 @@ abstract class DiffusionController extends PhabricatorController { $selected_href = $href; } - $nav->addFilter($href, $name); + $nav->addFilter($href, $name, $href); } $nav->selectFilter($selected_href, null); @@ -88,9 +88,11 @@ abstract class DiffusionController extends PhabricatorController { } public function buildCrumbs(array $spec = array()) { - $crumbs = new AphrontCrumbsView(); + $crumbs = $this->buildApplicationCrumbs(); $crumb_list = $this->buildCrumbList($spec); - $crumbs->setCrumbs($crumb_list); + foreach ($crumb_list as $crumb) { + $crumbs->addCrumb($crumb); + } return $crumbs; } @@ -155,15 +157,11 @@ abstract class DiffusionController extends PhabricatorController { $repository = null; } - if ($repository) { - $crumb_list[] = phutil_render_tag( - 'a', - array( - 'href' => '/diffusion/', - ), - 'Diffusion'); - } else { - $crumb_list[] = 'Diffusion'; + $crumb = id(new PhabricatorCrumbView()) + ->setName('Diffusion') + ->setHref('/diffusion/'); + $crumb_list[] = $crumb; + if (!$repository) { return $crumb_list; } @@ -177,49 +175,53 @@ abstract class DiffusionController extends PhabricatorController { } } - if (!$spec['view'] && !$spec['commit'] - && !$spec['tags'] && !$spec['branches']) { - $crumb_list[] = $repository_name; - return $crumb_list; + $crumb = id(new PhabricatorCrumbView()) + ->setName($repository_name); + if (!$spec['view'] && !$spec['commit'] && + !$spec['tags'] && !$spec['branches']) { + $crumb_list[] = $crumb; + return $crumb_list; } - - $crumb_list[] = phutil_render_tag( - 'a', - array( - 'href' => "/diffusion/{$callsign}/", - ), - $repository_name); + $crumb->setHref("/diffusion/{$callsign}/"); + $crumb_list[] = $crumb; $raw_commit = $drequest->getRawCommit(); if ($spec['tags']) { + $crumb = new PhabricatorCrumbView(); if ($spec['commit']) { - $crumb_list[] = "Tags for ".phutil_render_tag( - 'a', + $crumb->setName( + "Tags for r{$callsign}{$raw_commit}" + ); + $crumb->setHref($drequest->generateURI( array( - 'href' => $drequest->generateURI( - array( - 'action' => 'commit', - 'commit' => $raw_commit, - )), - ), - phutil_escape_html("r{$callsign}{$raw_commit}")); + 'action' => 'commit', + 'commit' => $raw_commit, + )) + ); } else { - $crumb_list[] = 'Tags'; + $crumb->setName('Tags'); } + $crumb_list[] = $crumb; return $crumb_list; } if ($spec['branches']) { - $crumb_list[] = 'Branches'; + $crumb = id(new PhabricatorCrumbView()) + ->setName('Branches'); + $crumb_list[] = $crumb; return $crumb_list; } if ($spec['commit']) { - $crumb_list[] = "r{$callsign}{$raw_commit}"; + $crumb = id(new PhabricatorCrumbView()) + ->setName("r{$callsign}{$raw_commit}") + ->setHref("r{$callsign}{$raw_commit}"); + $crumb_list[] = $crumb; return $crumb_list; } + $crumb = new PhabricatorCrumbView(); $view = $spec['view']; $path = null; @@ -247,7 +249,9 @@ abstract class DiffusionController extends PhabricatorController { break; case 'change': $view_name = 'Change'; - $crumb_list[] = phutil_escape_html($path).' ('.$commit_link.')'; + $crumb_list[] = $crumb->setRawName( + phutil_escape_html($path).' ('.$commit_link.')' + ); return $crumb_list; } @@ -255,19 +259,18 @@ abstract class DiffusionController extends PhabricatorController { 'action' => $view, ); + $crumb = id(new PhabricatorCrumbView()) + ->setName($view_name); if (!strlen($path)) { - $crumb_list[] = $view_name; + $crumb_list[] = $crumb; } else { - $crumb_list[] = phutil_render_tag( - 'a', + $crumb->setHref($drequest->generateURI( array( - 'href' => $drequest->generateURI( - array( - 'path' => '', - ) + $uri_params), - ), - $view_name); + 'path' => '', + ) + $uri_params) + ); + $crumb_list[] = $crumb; $path_parts = explode('/', $path); do { @@ -292,7 +295,8 @@ abstract class DiffusionController extends PhabricatorController { $path_sections[] = phutil_escape_html($last); $path_sections = '/'.implode('/', $path_sections); - $crumb_list[] = $path_sections; + $crumb_list[] = id(new PhabricatorCrumbView()) + ->setRawName($path_sections); } $last_crumb = array_pop($crumb_list); @@ -307,9 +311,13 @@ abstract class DiffusionController extends PhabricatorController { ) + $uri_params), ), 'Jump to HEAD'); - $last_crumb .= " @ {$commit_link} ({$jump_link})"; + $last_crumb->setRawName( + $last_crumb->getNameForRender() . " @ {$commit_link} ({$jump_link})" + ); } else if ($spec['view'] != 'lint') { - $last_crumb .= " @ HEAD"; + $last_crumb->setRawName( + $last_crumb->getNameForRender() . " @ HEAD" + ); } $crumb_list[] = $last_crumb; diff --git a/src/applications/diffusion/controller/DiffusionHistoryController.php b/src/applications/diffusion/controller/DiffusionHistoryController.php index ced077881a..3890d99d0c 100644 --- a/src/applications/diffusion/controller/DiffusionHistoryController.php +++ b/src/applications/diffusion/controller/DiffusionHistoryController.php @@ -39,13 +39,6 @@ final class DiffusionHistoryController extends DiffusionController { $content = array(); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'history', - )); - if ($request->getBool('copies')) { $button_title = 'Hide Copies/Branches'; $copies_new = null; @@ -89,8 +82,15 @@ final class DiffusionHistoryController extends DiffusionController { $nav = $this->buildSideNav('history', false); $nav->appendChild($content); + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'history', + )); + $nav->setCrumbs($crumbs); - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $nav, array( 'title' => 'history', diff --git a/src/applications/diffusion/controller/DiffusionLintController.php b/src/applications/diffusion/controller/DiffusionLintController.php index 467e1d6283..103fadb646 100644 --- a/src/applications/diffusion/controller/DiffusionLintController.php +++ b/src/applications/diffusion/controller/DiffusionLintController.php @@ -10,6 +10,7 @@ final class DiffusionLintController extends DiffusionController { if ($request->getStr('lint') !== null) { $controller = new DiffusionLintDetailsController($request); $controller->setDiffusionRequest($drequest); + $controller->setCurrentApplication($this->getCurrentApplication()); return $this->delegateToController($controller); } @@ -93,14 +94,6 @@ final class DiffusionLintController extends DiffusionController { $content = array(); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'lint', - )); - - $link = null; if ($this->diffusionRequest) { $link = hsprintf( @@ -134,12 +127,22 @@ final class DiffusionLintController extends DiffusionController { ->appendChild($table); $title = array('Lint'); + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'lint', + )); if ($this->diffusionRequest) { $title[] = $drequest->getCallsign(); - $content = $this->buildSideNav('lint', false)->appendChild($content); + $content = $this->buildSideNav('lint', false) + ->setCrumbs($crumbs) + ->appendChild($content); + } else { + array_unshift($content, $crumbs); } - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $content, array('title' => $title)); } diff --git a/src/applications/diffusion/controller/DiffusionLintDetailsController.php b/src/applications/diffusion/controller/DiffusionLintDetailsController.php index 3559584598..5eec9c507c 100644 --- a/src/applications/diffusion/controller/DiffusionLintDetailsController.php +++ b/src/applications/diffusion/controller/DiffusionLintDetailsController.php @@ -54,13 +54,6 @@ final class DiffusionLintDetailsController extends DiffusionController { $content = array(); - $content[] = $this->buildCrumbs( - array( - 'branch' => true, - 'path' => true, - 'view' => 'lint', - )); - $pager = id(new AphrontPagerView()) ->setPageSize($limit) ->setOffset($offset) @@ -86,8 +79,15 @@ final class DiffusionLintDetailsController extends DiffusionController { $nav = $this->buildSideNav('lint', false); $nav->appendChild($content); + $crumbs = $this->buildCrumbs( + array( + 'branch' => true, + 'path' => true, + 'view' => 'lint', + )); + $nav->setCrumbs($crumbs); - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $nav, array('title' => array( 'Lint', @@ -107,7 +107,12 @@ final class DiffusionLintDetailsController extends DiffusionController { $conn = $branch->establishConnection('r'); - $where = array(); + $where = array( + qsprintf( + $conn, + 'branchID = %d', + $branch->getID()) + ); if ($drequest->getPath() != '') { $is_dir = (substr($drequest->getPath(), -1) == '/'); $where[] = qsprintf( @@ -127,12 +132,9 @@ final class DiffusionLintDetailsController extends DiffusionController { $conn, 'SELECT * FROM %T - WHERE branchID = %d - AND %Q - ORDER BY path, code, line - LIMIT %d OFFSET %d', + WHERE %Q + ORDER BY path, code, line LIMIT %d OFFSET %d', PhabricatorRepository::TABLE_LINTMESSAGE, - $branch->getID(), implode(' AND ', $where), $limit, $offset); diff --git a/src/applications/diffusion/view/DiffusionView.php b/src/applications/diffusion/view/DiffusionView.php index 1ae1befba7..afa52d9d36 100644 --- a/src/applications/diffusion/view/DiffusionView.php +++ b/src/applications/diffusion/view/DiffusionView.php @@ -95,7 +95,9 @@ abstract class DiffusionView extends AphrontView { $text); } - final public static function linkCommit($repository, $commit) { + final public static function nameCommit( + PhabricatorRepository $repository, + $commit) { switch ($repository->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -108,7 +110,15 @@ abstract class DiffusionView extends AphrontView { } $callsign = $repository->getCallsign(); - $commit_name = "r{$callsign}{$commit_name}"; + return "r{$callsign}{$commit_name}"; + } + + final public static function linkCommit( + PhabricatorRepository $repository, + $commit) { + + $commit_name = self::nameCommit($repository, $commit); + $callsign = $repository->getCallsign(); return phutil_render_tag( 'a', diff --git a/src/view/layout/PhabricatorCrumbView.php b/src/view/layout/PhabricatorCrumbView.php index 84d2230754..8d756281b7 100644 --- a/src/view/layout/PhabricatorCrumbView.php +++ b/src/view/layout/PhabricatorCrumbView.php @@ -6,12 +6,27 @@ final class PhabricatorCrumbView extends AphrontView { private $href; private $icon; private $isLastCrumb; + private $rawName; + + /** + * Allows for custom HTML inside the name field. + * + * NOTE: you must handle escaping user text if you use this method. + */ + public function setRawName($raw_name) { + $this->rawName = $raw_name; + return $this; + } public function setName($name) { $this->name = $name; return $this; } + public function getNameForRender() { + return nonempty($this->rawName, phutil_escape_html($this->name)); + } + public function setHref($href) { $this->href = $href; return $this; @@ -53,7 +68,7 @@ final class PhabricatorCrumbView extends AphrontView { array( 'class' => 'phabricator-crumb-name', ), - phutil_escape_html($this->name)); + $this->getNameForRender()); $divider = null; if (!$this->isLastCrumb) { diff --git a/webroot/rsrc/css/aphront/phabricator-nav-view.css b/webroot/rsrc/css/aphront/phabricator-nav-view.css index 29ac93ed51..b964bdf770 100644 --- a/webroot/rsrc/css/aphront/phabricator-nav-view.css +++ b/webroot/rsrc/css/aphront/phabricator-nav-view.css @@ -78,7 +78,7 @@ } .device-desktop .local-nav-collapsed .phabricator-nav-content { - margin-left: 2.5em !important; + margin-left: 0px !important; } .device .phabricator-nav-col {