From e5906f4e127eb9bba62397f1228fa5d01f52ce22 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 08:21:16 -0700 Subject: [PATCH] In Differential standalone views, disable some keyboard shortcuts which don't work Summary: Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files. We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI. Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled. Test Plan: - Viewed a revision in normal and standalone views. - No changes in normal view, and all keys still work ("N", "P", etc). - In standalone view, "?" no longer shows nonfunctional key commands. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19571 --- resources/celerity/map.php | 32 +++++++++---------- .../DifferentialChangesetViewController.php | 1 + .../view/DifferentialChangesetListView.php | 11 +++++++ .../js/application/diff/DiffChangesetList.js | 29 +++++++++++------ .../differential/behavior-populate.js | 3 +- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9c0d0eda0a..12d47cd5f9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'f515619b', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => 'ef19e026', + 'differential.pkg.js' => '11a08e85', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -373,12 +373,12 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => 'b49b59d6', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'f0ffe8c3', + 'rsrc/js/application/diff/DiffChangesetList.js' => '7b95a80a', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', - 'rsrc/js/application/differential/behavior-populate.js' => '419998ab', + 'rsrc/js/application/differential/behavior-populate.js' => 'f0eb6708', 'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d', 'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '00676f00', 'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'd835b03a', @@ -594,7 +594,7 @@ return array( 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-feedback-preview' => '51c5ad07', - 'javelin-behavior-differential-populate' => '419998ab', + 'javelin-behavior-differential-populate' => 'f0eb6708', 'javelin-behavior-differential-user-select' => 'a8d8459d', 'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04', 'javelin-behavior-diffusion-commit-graph' => '75b83cbb', @@ -752,7 +752,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => 'b49b59d6', - 'phabricator-diff-changeset-list' => 'f0ffe8c3', + 'phabricator-diff-changeset-list' => '7b95a80a', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -1133,14 +1133,6 @@ return array( 'javelin-workflow', 'phabricator-draggable-list', ), - '419998ab' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'phabricator-tooltip', - 'phabricator-diff-changeset-list', - 'phabricator-diff-changeset', - ), '4250a34e' => array( 'javelin-behavior', 'javelin-dom', @@ -1508,6 +1500,10 @@ return array( 'owners-path-editor', 'javelin-behavior', ), + '7b95a80a' => array( + 'javelin-install', + 'phuix-button-view', + ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -2117,9 +2113,13 @@ return array( 'javelin-workflow', 'javelin-json', ), - 'f0ffe8c3' => array( - 'javelin-install', - 'phuix-button-view', + 'f0eb6708' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'phabricator-tooltip', + 'phabricator-diff-changeset-list', + 'phabricator-diff-changeset', ), 'f1ff5494' => array( 'phui-button-css', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 9d02202f9f..c41f951394 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -276,6 +276,7 @@ final class DifferentialChangesetViewController extends DifferentialController { ->setDiff($diff) ->setTitle(pht('Standalone View')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setIsStandalone(true) ->setParser($parser); if ($revision_id) { diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 59f73b5465..2ed963bae3 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -10,6 +10,7 @@ final class DifferentialChangesetListView extends AphrontView { private $whitespace; private $background; private $header; + private $isStandalone; private $standaloneURI; private $leftRawFileURI; @@ -124,6 +125,15 @@ final class DifferentialChangesetListView extends AphrontView { return $this; } + public function setIsStandalone($is_standalone) { + $this->isStandalone = $is_standalone; + return $this; + } + + public function getIsStandalone() { + return $this->isStandalone; + } + public function setBackground($background) { $this->background = $background; return $this; @@ -219,6 +229,7 @@ final class DifferentialChangesetListView extends AphrontView { 'changesetViewIDs' => $ids, 'inlineURI' => $this->inlineURI, 'inlineListURI' => $this->inlineListURI, + 'isStandalone' => $this->getIsStandalone(), 'pht' => array( 'Open in Editor' => pht('Open in Editor'), 'Show All Context' => pht('Show All Context'), diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 9e87fb0f31..66c80cb9cf 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -89,7 +89,8 @@ JX.install('DiffChangesetList', { properties: { translations: null, inlineURI: null, - inlineListURI: null + inlineListURI: null, + isStandalone: false }, members: { @@ -149,6 +150,12 @@ JX.install('DiffChangesetList', { this._initialized = true; var pht = this.getTranslations(); + // We may be viewing the normal "/D123" view (with all the changesets) + // or the standalone view (with just one changeset). In the standalone + // view, some options (like jumping to next or previous file) do not + // make sense and do not function. + var standalone = this.getIsStandalone(); + var label; label = pht('Jump to next change.'); @@ -157,11 +164,13 @@ JX.install('DiffChangesetList', { label = pht('Jump to previous change.'); this._installJumpKey('k', label, -1); - label = pht('Jump to next file.'); - this._installJumpKey('J', label, 1, 'file'); + if (!standalone) { + label = pht('Jump to next file.'); + this._installJumpKey('J', label, 1, 'file'); - label = pht('Jump to previous file.'); - this._installJumpKey('K', label, -1, 'file'); + label = pht('Jump to previous file.'); + this._installJumpKey('K', label, -1, 'file'); + } label = pht('Jump to next inline comment.'); this._installJumpKey('n', label, 1, 'comment'); @@ -176,11 +185,13 @@ JX.install('DiffChangesetList', { 'Jump to previous inline comment, including collapsed comments.'); this._installJumpKey('P', label, -1, 'comment', true); - label = pht('Hide or show the current file.'); - this._installKey('h', label, this._onkeytogglefile); + if (!standalone) { + label = pht('Hide or show the current file.'); + this._installKey('h', label, this._onkeytogglefile); - label = pht('Jump to the table of contents.'); - this._installKey('t', label, this._ontoc); + label = pht('Jump to the table of contents.'); + this._installKey('t', label, this._ontoc); + } label = pht('Reply to selected inline comment or change.'); this._installKey('r', label, JX.bind(this, this._onkeyreply, false)); diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 0a4a7912e4..5b415fd131 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -61,7 +61,8 @@ JX.behavior('differential-populate', function(config, statics) { var changeset_list = new JX.DiffChangesetList() .setTranslations(JX.phtize(config.pht)) .setInlineURI(config.inlineURI) - .setInlineListURI(config.inlineListURI); + .setInlineListURI(config.inlineListURI) + .setIsStandalone(config.isStandalone); // Install and activate the current page. var page_id = JX.Quicksand.getCurrentPageID();