From acc1fa165519de02e7328ceb43a8c14b97ec1a44 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 May 2020 13:24:53 -0700 Subject: [PATCH] Make "View as Document Type..." only show valid options Summary: Ref T13513. Currently, "View as Document Type..." lists every available engine. This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable. Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21241 --- resources/celerity/map.php | 46 +++++++++---------- .../parser/DifferentialChangesetParser.php | 22 +++++++++ ...habricatorSystemSelectViewAsController.php | 15 ++++-- .../rsrc/js/application/diff/DiffChangeset.js | 6 +++ .../js/application/diff/DiffChangesetList.js | 4 ++ 5 files changed, 66 insertions(+), 27 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9f7db23957..5efb4c8a29 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '1e667bcb', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'd71d4531', - 'differential.pkg.js' => '5ec354a0', + 'differential.pkg.js' => '39781f05', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -379,8 +379,8 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', - 'rsrc/js/application/diff/DiffChangeset.js' => '10ddd7e0', - 'rsrc/js/application/diff/DiffChangesetList.js' => '303efc90', + 'rsrc/js/application/diff/DiffChangeset.js' => '20715b98', + 'rsrc/js/application/diff/DiffChangesetList.js' => '564cbd20', 'rsrc/js/application/diff/DiffInline.js' => 'a0ef0b54', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', @@ -774,8 +774,8 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => '10ddd7e0', - 'phabricator-diff-changeset-list' => '303efc90', + 'phabricator-diff-changeset' => '20715b98', + 'phabricator-diff-changeset-list' => '564cbd20', 'phabricator-diff-inline' => 'a0ef0b54', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', @@ -1020,19 +1020,6 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), - '10ddd7e0' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - 'phabricator-diff-path-view', - 'phuix-button-view', - ), '111bfd2d' => array( 'javelin-install', ), @@ -1095,6 +1082,19 @@ return array( 'javelin-behavior', 'javelin-request', ), + '20715b98' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + 'phabricator-diff-inline', + 'phabricator-diff-path-view', + 'phuix-button-view', + ), '220b85f9' => array( 'syntax-default-css', ), @@ -1188,11 +1188,6 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), - '303efc90' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), '308f9fe4' => array( 'javelin-install', 'javelin-util', @@ -1423,6 +1418,11 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + '564cbd20' => array( + 'javelin-install', + 'phuix-button-view', + 'phabricator-diff-tree-view', + ), '5793d835' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 9ce06be69d..85bc28adf3 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -59,6 +59,7 @@ final class DifferentialChangesetParser extends Phobject { private $viewer; private $viewState; + private $availableDocumentEngines; public function setRange($start, $end) { $this->rangeStart = $start; @@ -1773,6 +1774,8 @@ final class DifferentialChangesetParser extends Phobject { } } + $this->availableDocumentEngines = $shared_engines; + $viewstate = $this->getViewState(); $engine_key = $viewstate->getDocumentEngineKey(); @@ -1878,6 +1881,24 @@ final class DifferentialChangesetParser extends Phobject { $document_engine_key = null; } + $available_keys = array(); + $engines = $this->availableDocumentEngines; + if (!$engines) { + $engines = array(); + } + + $available_keys = mpull($engines, 'getDocumentEngineKey'); + + // TODO: Always include "source" as a usable engine to default to + // the buitin rendering. This is kind of a hack and does not actually + // use the source engine. The source engine isn't a diff engine, so + // selecting it causes us to fall through and render with builtin + // behavior. For now, overall behavir is reasonable. + + $available_keys[] = PhabricatorSourceDocumentEngine::ENGINEKEY; + $available_keys = array_fuse($available_keys); + $available_keys = array_values($available_keys); + $state = array( 'undoTemplates' => $undo_templates, 'rendererKey' => $renderer_key, @@ -1885,6 +1906,7 @@ final class DifferentialChangesetParser extends Phobject { 'characterEncoding' => $viewstate->getCharacterEncoding(), 'requestDocumentEngineKey' => $viewstate->getDocumentEngineKey(), 'responseDocumentEngineKey' => $document_engine_key, + 'availableDocumentEngineKeys' => $available_keys, 'isHidden' => $viewstate->getHidden(), ); diff --git a/src/applications/system/controller/PhabricatorSystemSelectViewAsController.php b/src/applications/system/controller/PhabricatorSystemSelectViewAsController.php index 19f800f820..19e33dd512 100644 --- a/src/applications/system/controller/PhabricatorSystemSelectViewAsController.php +++ b/src/applications/system/controller/PhabricatorSystemSelectViewAsController.php @@ -18,17 +18,24 @@ final class PhabricatorSystemSelectViewAsController $engines = PhabricatorDocumentEngine::getAllEngines(); + $options = $request->getStrList('options'); + $options = array_fuse($options); - // TODO: This controller isn't very good because the valid options depend - // on the file being rendered and most of them can't even diff anything, - // and this ref is completely bogus. + // TODO: This controller is a bit rough because it isn't really using the + // file ref to figure out which engines should work. See also T13513. + // Callers can pass a list of "options" to control which options are + // presented, at least. - // For now, we just show everything. $ref = new PhabricatorDocumentRef(); $map = array(); foreach ($engines as $engine) { $key = $engine->getDocumentEngineKey(); + + if ($options && !isset($options[$key])) { + continue; + } + $label = $engine->getViewAsLabel($ref); if (!strlen($label)) { diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index 3b45d586b5..029e7eb8e6 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -66,6 +66,7 @@ JX.install('DiffChangeset', { _highlight: null, _requestDocumentEngineKey: null, _responseDocumentEngineKey: null, + _availableDocumentEngineKeys: null, _characterEncoding: null, _undoTemplates: null, @@ -420,6 +421,10 @@ JX.install('DiffChangeset', { return this._responseDocumentEngineKey; }, + getAvailableDocumentEngineKeys: function() { + return this._availableDocumentEngineKeys; + }, + getSelectableItems: function() { var items = []; @@ -672,6 +677,7 @@ JX.install('DiffChangeset', { this._characterEncoding = state.characterEncoding; this._requestDocumentEngineKey = state.requestDocumentEngineKey; this._responseDocumentEngineKey = state.responseDocumentEngineKey; + this._availableDocumentEngineKeys = state.availableDocumentEngineKeys; this._isHidden = state.isHidden; var is_hidden = !this.isVisible(); diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index c794ffe1bc..1f9b2ca24a 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -944,8 +944,12 @@ JX.install('DiffChangesetList', { .setIcon('fa-file-image-o') .setName(pht('View As Document Type...')) .setHandler(function(e) { + var options = changeset.getAvailableDocumentEngineKeys() || []; + options = options.join(','); + var params = { engine: changeset.getResponseDocumentEngineKey(), + options: options }; new JX.Workflow('/services/viewas/', params)