From bba1b185f8345e381461d1181568aeb903d433b2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 13:44:14 -0700 Subject: [PATCH] Improve minor client behaviors for document rendering Summary: Ref T13105. This adds various small client-side improvements to document rendering. - In the menu, show which renderer is in use. - Make linking to lines work. - Make URIs persist information about which rendering engine is in use. - Improve the UI feedback for transitions between document types. - Load slower documents asynchronously by default. - Discard irrelevant requests if you spam the view menu. Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19256 --- resources/celerity/map.php | 36 ++++---- src/__phutil_library_map__.php | 4 +- .../PhabricatorFilesApplication.php | 9 +- ....php => PhabricatorFileViewController.php} | 39 +++++++-- .../document/PhabricatorDocumentEngine.php | 34 ++++++++ .../PhabricatorJupyterDocumentEngine.php | 8 ++ .../PhabricatorTextDocumentEngine.php | 4 +- src/view/layout/PhabricatorSourceCodeView.php | 6 +- .../rsrc/css/phui/phui-property-list-view.css | 17 ++++ .../files/behavior-document-engine.js | 87 +++++++++++++++++-- webroot/rsrc/js/core/behavior-line-linker.js | 4 + 11 files changed, 210 insertions(+), 38 deletions(-) rename src/applications/files/controller/{PhabricatorFileInfoController.php => PhabricatorFileViewController.php} (91%) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 798f961f90..dabb6f1d5b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'da541195', + 'core.pkg.css' => '3fd3b7b8', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '54c071ed', + 'rsrc/css/phui/phui-property-list-view.css' => 'de4754d8', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -392,7 +392,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' => '396ef112', + 'rsrc/js/application/files/behavior-document-engine.js' => 'd3f8623c', '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' => '191b4909', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-keyboard-pager.js' => 'a8da01f0', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0', 'rsrc/js/core/behavior-lightbox-attachments.js' => '6b31879a', - 'rsrc/js/core/behavior-line-linker.js' => 'a9b946f8', + 'rsrc/js/core/behavior-line-linker.js' => '13e39479', 'rsrc/js/core/behavior-more.js' => 'a80d0378', 'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', @@ -607,7 +607,7 @@ return array( 'javelin-behavior-diffusion-jump-to' => '73d09eef', 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', - 'javelin-behavior-document-engine' => '396ef112', + 'javelin-behavior-document-engine' => 'd3f8623c', 'javelin-behavior-doorkeeper-tag' => '1db13e70', 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => '2ae077e1', @@ -638,7 +638,7 @@ return array( 'javelin-behavior-phabricator-gesture-example' => '558829c2', 'javelin-behavior-phabricator-keyboard-pager' => 'a8da01f0', 'javelin-behavior-phabricator-keyboard-shortcuts' => '01fca1f0', - 'javelin-behavior-phabricator-line-linker' => 'a9b946f8', + 'javelin-behavior-phabricator-line-linker' => '13e39479', 'javelin-behavior-phabricator-nav' => '836f966d', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => '77c1f0b0', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '54c071ed', + 'phui-property-list-view-css' => 'de4754d8', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', @@ -964,6 +964,12 @@ return array( 'javelin-install', 'javelin-util', ), + '13e39479' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + ), '15d5ff71' => array( 'aphront-typeahead-control-css', 'phui-tag-view-css', @@ -1103,11 +1109,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - '396ef112' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1751,12 +1752,6 @@ return array( 'javelin-uri', 'phabricator-keyboard-shortcut', ), - 'a9b946f8' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-history', - ), 'a9f88de2' => array( 'javelin-behavior', 'javelin-dom', @@ -2007,6 +2002,11 @@ return array( 'd254d646' => array( 'javelin-util', ), + 'd3f8623c' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + ), 'd4505101' => array( 'javelin-stratcom', 'javelin-install', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4c6efe4be2..b1afef3600 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3015,7 +3015,6 @@ phutil_register_library_map(array( 'PhabricatorFileImageMacro' => 'applications/macro/storage/PhabricatorFileImageMacro.php', 'PhabricatorFileImageProxyController' => 'applications/files/controller/PhabricatorFileImageProxyController.php', 'PhabricatorFileImageTransform' => 'applications/files/transform/PhabricatorFileImageTransform.php', - 'PhabricatorFileInfoController' => 'applications/files/controller/PhabricatorFileInfoController.php', 'PhabricatorFileIntegrityException' => 'applications/files/exception/PhabricatorFileIntegrityException.php', 'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', @@ -3051,6 +3050,7 @@ phutil_register_library_map(array( 'PhabricatorFileUploadException' => 'applications/files/exception/PhabricatorFileUploadException.php', 'PhabricatorFileUploadSource' => 'applications/files/uploadsource/PhabricatorFileUploadSource.php', 'PhabricatorFileUploadSourceByteLimitException' => 'applications/files/uploadsource/PhabricatorFileUploadSourceByteLimitException.php', + 'PhabricatorFileViewController' => 'applications/files/controller/PhabricatorFileViewController.php', 'PhabricatorFileinfoSetupCheck' => 'applications/config/check/PhabricatorFileinfoSetupCheck.php', 'PhabricatorFilesApplication' => 'applications/files/application/PhabricatorFilesApplication.php', 'PhabricatorFilesApplicationStorageEnginePanel' => 'applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php', @@ -8622,7 +8622,6 @@ phutil_register_library_map(array( ), 'PhabricatorFileImageProxyController' => 'PhabricatorFileController', 'PhabricatorFileImageTransform' => 'PhabricatorFileTransform', - 'PhabricatorFileInfoController' => 'PhabricatorFileController', 'PhabricatorFileIntegrityException' => 'Exception', 'PhabricatorFileLightboxController' => 'PhabricatorFileController', 'PhabricatorFileLinkView' => 'AphrontTagView', @@ -8658,6 +8657,7 @@ phutil_register_library_map(array( 'PhabricatorFileUploadException' => 'Exception', 'PhabricatorFileUploadSource' => 'Phobject', 'PhabricatorFileUploadSourceByteLimitException' => 'Exception', + 'PhabricatorFileViewController' => 'PhabricatorFileController', 'PhabricatorFileinfoSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorFilesApplication' => 'PhabricatorApplication', 'PhabricatorFilesApplicationStorageEnginePanel' => 'PhabricatorApplicationConfigurationPanel', diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index d95972d10e..5bf1ebccc4 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -69,9 +69,15 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { public function getRoutes() { return array( - '/F(?P[1-9]\d*)' => 'PhabricatorFileInfoController', + '/F(?P[1-9]\d*)(?:\$(?P\d+(?:-\d+)?))?' + => 'PhabricatorFileViewController', '/file/' => array( '(query/(?P[^/]+)/)?' => 'PhabricatorFileListController', + 'view/(?P[^/]+)/'. + '(?:(?P[^/]+)/)?'. + '(?:\$(?P\d+(?:-\d+)?))?' + => 'PhabricatorFileViewController', + 'info/(?P[^/]+)/' => 'PhabricatorFileViewController', 'upload/' => 'PhabricatorFileUploadController', 'dropupload/' => 'PhabricatorFileDropUploadController', 'compose/' => 'PhabricatorFileComposeController', @@ -80,7 +86,6 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { 'delete/(?P[1-9]\d*)/' => 'PhabricatorFileDeleteController', $this->getEditRoutePattern('edit/') => 'PhabricatorFileEditController', - 'info/(?P[^/]+)/' => 'PhabricatorFileInfoController', 'imageproxy/' => 'PhabricatorFileImageProxyController', 'transforms/(?P[1-9]\d*)/' => 'PhabricatorFileTransformListController', diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileViewController.php similarity index 91% rename from src/applications/files/controller/PhabricatorFileInfoController.php rename to src/applications/files/controller/PhabricatorFileViewController.php index e25cf8d74c..5251fdd8f1 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileViewController.php @@ -1,6 +1,6 @@ getViewer(); + $request = $this->getRequest(); $ref = id(new PhabricatorDocumentRef()) ->setFile($file); $engines = PhabricatorDocumentEngine::getEnginesForRef($viewer, $ref); - $engine = head($engines); - $content = $engine->newDocument($ref); + $engine_key = $request->getURIData('engineKey'); + if (!isset($engines[$engine_key])) { + $engine_key = head_key($engines); + } + $engine = $engines[$engine_key]; - $icon = $engine->newDocumentIcon($ref); + $lines = $request->getURILineRange('lines', 1000); + if ($lines) { + $engine->setHighlightedLines(range($lines[0], $lines[1])); + } $views = array(); - foreach ($engines as $candidate_engine) { + foreach ($engines as $candidate_key => $candidate_engine) { $label = $candidate_engine->getViewAsLabel($ref); if ($label === null) { continue; } + $view_uri = '/file/view/'.$file->getID().'/'.$candidate_key.'/'; + $view_icon = $candidate_engine->getViewAsIconIcon($ref); $view_color = $candidate_engine->getViewAsIconColor($ref); + $loading = $candidate_engine->newLoadingContent($ref); $views[] = array( 'viewKey' => $candidate_engine->getDocumentEngineKey(), @@ -431,12 +441,26 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { 'color' => $view_color, 'name' => $label, 'engineURI' => $candidate_engine->getRenderURI($ref), + 'viewURI' => $view_uri, + 'loadingMarkup' => hsprintf('%s', $loading), ); } - Javelin::initBehavior('document-engine'); - $viewport_id = celerity_generate_unique_node_id(); + $control_id = celerity_generate_unique_node_id(); + $icon = $engine->newDocumentIcon($ref); + + if ($engine->shouldRenderAsync($ref)) { + $content = $engine->newLoadingContent($ref); + $config = array( + 'renderControlID' => $control_id, + ); + } else { + $content = $engine->newDocument($ref); + $config = array(); + } + + Javelin::initBehavior('document-engine', $config); $viewport = phutil_tag( 'div', @@ -457,6 +481,7 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { ->setText(pht('View Options')) ->setIcon('fa-file-image-o') ->setColor(PHUIButtonView::GREY) + ->setID($control_id) ->setMetadata($meta) ->setDropdown(true) ->addSigil('document-engine-view-dropdown'); diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index 6ad7f74f9a..a225d55ea9 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -4,6 +4,7 @@ abstract class PhabricatorDocumentEngine extends Phobject { private $viewer; + private $highlightedLines = array(); final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -14,10 +15,23 @@ abstract class PhabricatorDocumentEngine return $this->viewer; } + final public function setHighlightedLines(array $highlighted_lines) { + $this->highlightedLines = $highlighted_lines; + return $this; + } + + final public function getHighlightedLines() { + return $this->highlightedLines; + } + final public function canRenderDocument(PhabricatorDocumentRef $ref) { return $this->canRenderDocumentType($ref); } + public function shouldRenderAsync(PhabricatorDocumentRef $ref) { + return false; + } + abstract protected function canRenderDocumentType( PhabricatorDocumentRef $ref); @@ -49,6 +63,10 @@ abstract class PhabricatorDocumentEngine return 'fa-file-o'; } + protected function getDocumentRenderingText(PhabricatorDocumentRef $ref) { + return pht('Loading...'); + } + final public function getDocumentEngineKey() { return $this->getPhobjectClassConstant('ENGINEKEY'); } @@ -177,4 +195,20 @@ abstract class PhabricatorDocumentEngine $message); } + final public function newLoadingContent(PhabricatorDocumentRef $ref) { + $spinner = id(new PHUIIconView()) + ->setIcon('fa-gear') + ->addClass('ph-spin'); + + return phutil_tag( + 'div', + array( + 'class' => 'document-engine-loading', + ), + array( + $spinner, + $this->getDocumentRenderingText($ref), + )); + } + } diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index 7b2246a315..f960f5c8c0 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -13,6 +13,14 @@ final class PhabricatorJupyterDocumentEngine return 'fa-sun-o'; } + protected function getDocumentRenderingText(PhabricatorDocumentRef $ref) { + return pht('Rendering Jupyter Notebook...'); + } + + public function shouldRenderAsync(PhabricatorDocumentRef $ref) { + return true; + } + protected function getContentScore(PhabricatorDocumentRef $ref) { $name = $ref->getName(); diff --git a/src/applications/files/document/PhabricatorTextDocumentEngine.php b/src/applications/files/document/PhabricatorTextDocumentEngine.php index 506c73badb..4fb8d052e1 100644 --- a/src/applications/files/document/PhabricatorTextDocumentEngine.php +++ b/src/applications/files/document/PhabricatorTextDocumentEngine.php @@ -11,8 +11,8 @@ abstract class PhabricatorTextDocumentEngine $lines = phutil_split_lines($content); $view = id(new PhabricatorSourceCodeView()) - ->setLines($lines) - ->disableHighlightOnClick(); + ->setHighlights($this->getHighlightedLines()) + ->setLines($lines); $container = phutil_tag( 'div', diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index f5f6c22b51..ce26fa3df7 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -85,7 +85,11 @@ final class PhabricatorSourceCodeView extends AphrontView { } if ($this->canClickHighlight) { - $line_href = $base_uri.'$'.$line_number; + if ($base_uri) { + $line_href = $base_uri.'$'.$line_number; + } else { + $line_href = null; + } $tag_number = phutil_tag( 'a', diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index c1f9944cbd..15632c902f 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -267,6 +267,23 @@ div.phui-property-list-stacked .phui-property-list-properties margin: 20px; } +.document-engine-in-flight { + opacity: 0.25; +} + +.document-engine-loading { + margin: 20px; + text-align: center; + color: {$lightgreytext}; +} + +.document-engine-loading .phui-icon-view { + display: block; + font-size: 48px; + color: {$lightgreyborder}; + padding: 8px; +} + .jupyter-cell-raw { white-space: pre-wrap; background: {$lightgreybackground}; diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js index 6b9b3eeb24..4cb7d17723 100644 --- a/webroot/rsrc/js/application/files/behavior-document-engine.js +++ b/webroot/rsrc/js/application/files/behavior-document-engine.js @@ -5,7 +5,9 @@ * javelin-stratcom */ -JX.behavior('document-engine', function() { +JX.behavior('document-engine', function(config, statics) { + + function onmenu(e) { var node = e.getNode('document-engine-view-dropdown'); @@ -21,6 +23,7 @@ JX.behavior('document-engine', function() { var list = new JX.PHUIXActionListView(); var view; + var engines = []; for (var ii = 0; ii < data.views.length; ii++) { var spec = data.views[ii]; @@ -38,31 +41,103 @@ JX.behavior('document-engine', function() { e.prevent(); menu.close(); - onview(data, spec); + onview(data, spec, false); }, spec)); list.addItem(view); + + engines.push({ + spec: spec, + view: view + }); } menu.setContent(list.getNode()); + menu.listen('open', function() { + for (var ii = 0; ii < engines.length; ii++) { + var engine = engines[ii]; + + // Highlight the current rendering engine. + var is_selected = (engine.spec.viewKey == data.viewKey); + engine.view.setSelected(is_selected); + } + }); + data.menu = menu; menu.open(); } - function onview(data, spec) { - var handler = JX.bind(null, onrender, data); + function onview(data, spec, immediate) { + data.sequence = (data.sequence || 0) + 1; + var handler = JX.bind(null, onrender, data, data.sequence); + + data.viewKey = spec.viewKey; + JX.History.replace(spec.viewURI); new JX.Request(spec.engineURI, handler) .send(); + + if (data.loadingView) { + // If we're already showing "Loading...", immediately change it to + // show the new document type. + onloading(data, spec); + } else if (!immediate) { + // Otherwise, grey out the document and show "Loading..." after a + // short delay. This prevents the content from flickering when rendering + // is fast. + var viewport = JX.$(data.viewportID); + JX.DOM.alterClass(viewport, 'document-engine-in-flight', true); + + var load = JX.bind(null, onloading, data, spec); + data.loadTimer = setTimeout(load, 333); + } } - function onrender(data, r) { + function onloading(data, spec) { + data.loadingView = true; + var viewport = JX.$(data.viewportID); + JX.DOM.alterClass(viewport, 'document-engine-in-flight', false); + JX.DOM.setContent(viewport, JX.$H(spec.loadingMarkup)); + } + + function onrender(data, sequence, r) { + // If this isn't the most recent request we sent, throw it away. This can + // happen if the user makes multiple selections from the menu while we are + // still rendering the first view. + if (sequence != data.sequence) { + return; + } + + if (data.loadTimer) { + clearTimeout(data.loadTimer); + data.loadTimer = null; + } + + var viewport = JX.$(data.viewportID); + + JX.DOM.alterClass(viewport, 'document-engine-in-flight', false); + data.loadingView = false; JX.DOM.setContent(viewport, JX.$H(r.markup)); } - JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu); + if (!statics.initialized) { + JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu); + statics.initialized = true; + } + + if (config.renderControlID) { + var control = JX.$(config.renderControlID); + var data = JX.Stratcom.getData(control); + + for (var ii = 0; ii < data.views.length; ii++) { + if (data.views[ii].viewKey == data.viewKey) { + onview(data, data.views[ii], true); + break; + } + } + } }); diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js index a0f99fb3bc..8a68cec842 100644 --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/webroot/rsrc/js/core/behavior-line-linker.js @@ -145,6 +145,10 @@ JX.behavior('phabricator-line-linker', function() { var t = getRowNumber(target); var uri = JX.Stratcom.getData(root).uri; + if (!uri) { + uri = ('' + window.location).split('$')[0]; + } + origin = null; target = null; root = null;