From 6dea2ba3b37f0590efb607ea5e27c623ba19bca7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Apr 2018 06:29:29 -0700 Subject: [PATCH] Fix DocumentEngine line behaviors in Diffusion Summary: Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine: - Adding `$1-3` to the URI didn't work correctly with query parameters. - Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally. Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection. Reviewers: mydeveloperday Reviewed By: mydeveloperday Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19305 --- resources/celerity/map.php | 16 +++++----- src/aphront/AphrontRequest.php | 4 +++ .../DiffusionDocumentRenderingEngine.php | 13 +++++--- .../PhabricatorDocumentRenderingEngine.php | 30 +++++++++++-------- webroot/rsrc/js/core/behavior-line-linker.js | 13 ++++++-- 5 files changed, 49 insertions(+), 27 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 79e645a927..ca75f4ee09 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -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' => '13e39479', + 'rsrc/js/core/behavior-line-linker.js' => 'febf4ae7', 'rsrc/js/core/behavior-more.js' => 'a80d0378', 'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', @@ -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' => '13e39479', + 'javelin-behavior-phabricator-line-linker' => 'febf4ae7', 'javelin-behavior-phabricator-nav' => '836f966d', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => '77c1f0b0', @@ -964,12 +964,6 @@ 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', @@ -2174,6 +2168,12 @@ return array( 'javelin-view-visitor', 'javelin-util', ), + 'febf4ae7' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + ), ), 'packages' => array( 'conpherence.pkg.css' => array( diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index c1dc0d1305..78e6dac9d3 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -61,6 +61,10 @@ final class AphrontRequest extends Phobject { */ public function getURILineRange($key, $limit) { $range = $this->getURIData($key); + return self::parseURILineRange($range, $limit); + } + + public static function parseURILineRange($range, $limit) { if (!strlen($range)) { return null; } diff --git a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php index d0b5a4b33a..b3b77c9301 100644 --- a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php +++ b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php @@ -14,10 +14,6 @@ final class DiffusionDocumentRenderingEngine return $this->diffusionRequest; } - protected function getSelectedDocumentEngineKey() { - return $this->getRequest()->getStr('as'); - } - protected function newRefViewURI( PhabricatorDocumentRef $ref, PhabricatorDocumentEngine $engine) { @@ -58,6 +54,15 @@ final class DiffusionDocumentRenderingEngine )); } + protected function getSelectedDocumentEngineKey() { + return $this->getRequest()->getStr('as'); + } + + protected function getSelectedLineRange() { + $range = $this->getDiffusionRequest()->getLine(); + return AphrontRequest::parseURILineRange($range, 1000); + } + protected function addApplicationCrumbs( PHUICrumbsView $crumbs, PhabricatorDocumentRef $ref = null) { diff --git a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php index fb6fe67e62..d95d93f74a 100644 --- a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php +++ b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php @@ -54,7 +54,7 @@ abstract class PhabricatorDocumentRenderingEngine } $engine = $engines[$engine_key]; - $lines = $request->getURILineRange('lines', 1000); + $lines = $this->getSelectedLineRange(); if ($lines) { $engine->setHighlightedLines(range($lines[0], $lines[1])); } @@ -157,18 +157,6 @@ abstract class PhabricatorDocumentRenderingEngine ->appendChild($viewport); } - abstract protected function newRefViewURI( - PhabricatorDocumentRef $ref, - PhabricatorDocumentEngine $engine); - - abstract protected function newRefRenderURI( - PhabricatorDocumentRef $ref, - PhabricatorDocumentEngine $engine); - - protected function getSelectedDocumentEngineKey() { - return $this->getRequest()->getURIData('engineKey'); - } - final public function newRenderResponse(PhabricatorDocumentRef $ref) { $request = $this->getRequest(); $viewer = $request->getViewer(); @@ -280,6 +268,22 @@ abstract class PhabricatorDocumentRenderingEngine return $crumbs; } + abstract protected function newRefViewURI( + PhabricatorDocumentRef $ref, + PhabricatorDocumentEngine $engine); + + abstract protected function newRefRenderURI( + PhabricatorDocumentRef $ref, + PhabricatorDocumentEngine $engine); + + protected function getSelectedDocumentEngineKey() { + return $this->getRequest()->getURIData('engineKey'); + } + + protected function getSelectedLineRange() { + return $this->getRequest()->getURILineRange('lines', 1000); + } + protected function addApplicationCrumbs( PHUICrumbsView $crumbs, PhabricatorDocumentRef $ref = null) { diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js index 8a68cec842..6130bdd859 100644 --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/webroot/rsrc/js/core/behavior-line-linker.js @@ -144,9 +144,14 @@ JX.behavior('phabricator-line-linker', function() { var o = getRowNumber(origin); var t = getRowNumber(target); var uri = JX.Stratcom.getData(root).uri; + var path; if (!uri) { - uri = ('' + window.location).split('$')[0]; + uri = JX.$U(window.location); + path = uri.getPath(); + path = path.replace(/\$[\d-]+$/, ''); + uri.setPath(path); + uri = uri.toString(); } origin = null; @@ -154,7 +159,11 @@ JX.behavior('phabricator-line-linker', function() { root = null; var lines = (o == t ? o : Math.min(o, t) + '-' + Math.max(o, t)); - uri = uri + '$' + lines; + + uri = JX.$U(uri); + path = uri.getPath(); + path = path + '$' + lines; + uri = uri.setPath(path).toString(); JX.History.replace(uri);