From afc3099ee785c0f9cb86c702fc1d04d2be9f6af5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 28 Apr 2018 06:43:09 -0700 Subject: [PATCH] Add a view option to disable blame in Diffusion and fix some view transition bugs Summary: See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off. Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame. Test Plan: - Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL. - Viewed a Jupyter notebook, toggled to "Source" view, saw blame. - Viewed stuff in Files (no blame UI options). - Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130, T13105 Differential Revision: https://secure.phabricator.com/D19414 --- resources/celerity/map.php | 14 +++--- .../DiffusionDocumentRenderingEngine.php | 12 +++-- .../document/PhabricatorDocumentEngine.php | 14 ++++++ .../PhabricatorSourceDocumentEngine.php | 2 +- .../PhabricatorDocumentRenderingEngine.php | 17 +++++++ .../files/behavior-document-engine.js | 45 ++++++++++++++++++- 6 files changed, 91 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 480c4b602d..78b3660c4c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -388,7 +388,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', - 'rsrc/js/application/files/behavior-document-engine.js' => 'ee0deff8', + 'rsrc/js/application/files/behavior-document-engine.js' => '3935d8c4', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '549459b8', @@ -600,7 +600,7 @@ return array( 'javelin-behavior-diffusion-commit-graph' => '75b83cbb', 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', - 'javelin-behavior-document-engine' => 'ee0deff8', + 'javelin-behavior-document-engine' => '3935d8c4', 'javelin-behavior-doorkeeper-tag' => '1db13e70', 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => '2ae077e1', @@ -1080,6 +1080,11 @@ return array( 'javelin-dom', 'javelin-vector', ), + '3935d8c4' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -2105,11 +2110,6 @@ return array( 'javelin-behavior', 'javelin-uri', ), - 'ee0deff8' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - ), 'efe49472' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php index 9615b35799..800d195726 100644 --- a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php +++ b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php @@ -69,7 +69,7 @@ final class DiffusionDocumentRenderingEngine return; } - protected function willRenderRef(PhabricatorDocumentRef $ref) { + protected function willStageRef(PhabricatorDocumentRef $ref) { $drequest = $this->getDiffusionRequest(); $blame_uri = (string)$drequest->generateURI( @@ -78,9 +78,13 @@ final class DiffusionDocumentRenderingEngine 'stable' => true, )); - $ref - ->setSymbolMetadata($this->getSymbolMetadata()) - ->setBlameURI($blame_uri); + $ref->setBlameURI($blame_uri); + } + + protected function willRenderRef(PhabricatorDocumentRef $ref) { + $drequest = $this->getDiffusionRequest(); + + $ref->setSymbolMetadata($this->getSymbolMetadata()); $coverage = $drequest->loadCoverage(); if (strlen($coverage)) { diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index 85219b904c..c869f5f0d7 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -7,6 +7,7 @@ abstract class PhabricatorDocumentEngine private $highlightedLines = array(); private $encodingConfiguration; private $highlightingConfiguration; + private $blameConfiguration = true; final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -60,6 +61,19 @@ abstract class PhabricatorDocumentEngine return $this->highlightingConfiguration; } + final public function setBlameConfiguration($blame_configuration) { + $this->blameConfiguration = $blame_configuration; + return $this; + } + + final public function getBlameConfiguration() { + return $this->blameConfiguration; + } + + final protected function getBlameEnabled() { + return $this->blameConfiguration; + } + public function shouldRenderAsync(PhabricatorDocumentRef $ref) { return false; } diff --git a/src/applications/files/document/PhabricatorSourceDocumentEngine.php b/src/applications/files/document/PhabricatorSourceDocumentEngine.php index c88d979b4e..5d7c0cdb75 100644 --- a/src/applications/files/document/PhabricatorSourceDocumentEngine.php +++ b/src/applications/files/document/PhabricatorSourceDocumentEngine.php @@ -50,7 +50,7 @@ final class PhabricatorSourceDocumentEngine } $options = array(); - if ($ref->getBlameURI()) { + if ($ref->getBlameURI() && $this->getBlameEnabled()) { $content = phutil_split_lines($content); $blame = range(1, count($content)); $blame = array_fuse($blame); diff --git a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php index 155ed28dfa..2524f77821 100644 --- a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php +++ b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php @@ -69,6 +69,9 @@ abstract class PhabricatorDocumentRenderingEngine $engine->setHighlightingConfiguration($highlight_setting); } + $blame_setting = ($request->getStr('blame') !== 'off'); + $engine->setBlameConfiguration($blame_setting); + $views = array(); foreach ($engines as $candidate_key => $candidate_engine) { $label = $candidate_engine->getViewAsLabel($ref); @@ -104,6 +107,8 @@ abstract class PhabricatorDocumentRenderingEngine 'controlID' => $control_id, ); + $this->willStageRef($ref); + if ($engine->shouldRenderAsync($ref)) { $content = $engine->newLoadingContent($ref); $config['next'] = 'render'; @@ -142,7 +147,11 @@ abstract class PhabricatorDocumentRenderingEngine 'value' => $highlight_setting, ), 'blame' => array( + 'icon' => 'fa-backward', + 'hide' => pht('Hide Blame'), + 'show' => pht('Show Blame'), 'uri' => $ref->getBlameURI(), + 'enabled' => $blame_setting, 'value' => null, ), 'coverage' => array( @@ -180,6 +189,7 @@ abstract class PhabricatorDocumentRenderingEngine } final public function newRenderResponse(PhabricatorDocumentRef $ref) { + $this->willStageRef($ref); $this->willRenderRef($ref); $request = $this->getRequest(); @@ -207,6 +217,9 @@ abstract class PhabricatorDocumentRenderingEngine $engine->setHighlightingConfiguration($highlight_setting); } + $blame_setting = ($request->getStr('blame') !== 'off'); + $engine->setBlameConfiguration($blame_setting); + try { $content = $engine->newDocument($ref); } catch (Exception $ex) { @@ -314,6 +327,10 @@ abstract class PhabricatorDocumentRenderingEngine return; } + protected function willStageRef(PhabricatorDocumentRef $ref) { + return; + } + protected function willRenderRef(PhabricatorDocumentRef $ref) { return; } diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js index 8b957e0ea0..caad5977bc 100644 --- a/webroot/rsrc/js/application/files/behavior-document-engine.js +++ b/webroot/rsrc/js/application/files/behavior-document-engine.js @@ -105,6 +105,29 @@ JX.behavior('document-engine', function(config, statics) { list.addItem(highlight_item); + var blame_item; + if (data.blame.uri) { + blame_item = new JX.PHUIXActionView() + .setIcon(data.blame.icon); + + var onblame = JX.bind(null, function(data, e) { + e.prevent(); + + if (blame_item.getDisabled()) { + return; + } + + data.blame.enabled = !data.blame.enabled; + onview(data); + + menu.close(); + }, data); + + blame_item.setHandler(onblame); + + list.addItem(blame_item); + } + menu.setContent(list.getNode()); menu.listen('open', function() { @@ -118,8 +141,22 @@ JX.behavior('document-engine', function(config, statics) { if (is_selected) { encode_item.setDisabled(!engine.spec.canEncode); highlight_item.setDisabled(!engine.spec.canHighlight); + if (blame_item) { + blame_item.setDisabled(!engine.spec.canBlame); + } } } + + if (blame_item) { + var blame_label; + if (data.blame.enabled) { + blame_label = data.blame.hide; + } else { + blame_label = data.blame.show; + } + + blame_item.setName(blame_label); + } }); data.menu = menu; @@ -137,6 +174,12 @@ JX.behavior('document-engine', function(config, statics) { uri.setQueryParam('encode', data.encode.value); } + if (data.blame.enabled) { + uri.setQueryParam('blame', null); + } else { + uri.setQueryParam('blame', 'off'); + } + return uri.toString(); } @@ -211,7 +254,7 @@ JX.behavior('document-engine', function(config, statics) { JX.DOM.setContent(viewport, JX.$H(r.markup)); // If this engine supports rendering blame, populate or draw it. - if (spec.canBlame) { + if (spec.canBlame && data.blame.enabled) { blame(data); } }