From 3963c86ad5e52536cdf11983037d173ce7bb12f7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Jan 2019 16:48:53 -0800 Subject: [PATCH] Pass timeline view data to comment previews, restoring Differential comment previews Summary: Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render. Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything. After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke. This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future. Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19943 --- resources/celerity/map.php | 35 ++---- resources/celerity/packages.php | 1 - .../editengine/PhabricatorEditEngine.php | 8 ++ ...bricatorApplicationTransactionResponse.php | 13 +- ...catorApplicationTransactionCommentView.php | 8 ++ .../differential/behavior-comment-preview.js | 116 ------------------ .../transactions/behavior-comment-actions.js | 2 + 7 files changed, 42 insertions(+), 141 deletions(-) delete mode 100644 webroot/rsrc/js/application/differential/behavior-comment-preview.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 69896ca0b1..cbe14c82a7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => '47535fd5', 'core.pkg.js' => 'bd89cb1d', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => 'ef0b989b', + 'differential.pkg.js' => '853c3461', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -376,7 +376,6 @@ return array( 'rsrc/js/application/diff/DiffChangesetList.js' => '0a84bcc1', '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' => 'f0eb6708', 'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d', @@ -422,7 +421,7 @@ return array( 'rsrc/js/application/repository/repository-crossreference.js' => '9a860428', 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', - 'rsrc/js/application/transactions/behavior-comment-actions.js' => '59e27e74', + 'rsrc/js/application/transactions/behavior-comment-actions.js' => 'd848ec84', 'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243', 'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96', 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => '0e1eca96', @@ -575,7 +574,7 @@ return array( 'javelin-behavior-bulk-job-reload' => 'edf8a145', 'javelin-behavior-calendar-month-view' => 'fe33e256', 'javelin-behavior-choose-control' => '327a00d1', - 'javelin-behavior-comment-actions' => '59e27e74', + 'javelin-behavior-comment-actions' => 'd848ec84', 'javelin-behavior-config-reorder-fields' => 'b6993408', 'javelin-behavior-conpherence-menu' => '4047cd35', 'javelin-behavior-conpherence-participant-pane' => 'd057e45a', @@ -593,7 +592,6 @@ return array( 'javelin-behavior-device' => 'a3714c76', 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', - 'javelin-behavior-differential-feedback-preview' => '51c5ad07', 'javelin-behavior-differential-populate' => 'f0eb6708', 'javelin-behavior-differential-user-select' => 'a8d8459d', 'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04', @@ -1246,14 +1244,6 @@ return array( 'javelin-typeahead-source', 'javelin-util', ), - '51c5ad07' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-request', - 'javelin-util', - 'phabricator-shaped-request', - ), '522431f7' => array( 'javelin-behavior', 'javelin-util', @@ -1323,15 +1313,6 @@ return array( 'javelin-vector', 'javelin-dom', ), - '59e27e74' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-dom', - 'phuix-form-control-view', - 'phuix-icon-view', - 'javelin-behavior-phabricator-gesture', - ), '5c54cbf3' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1991,6 +1972,15 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), + 'd848ec84' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-dom', + 'phuix-form-control-view', + 'phuix-icon-view', + 'javelin-behavior-phabricator-gesture', + ), 'db34a142' => array( 'phui-inline-comment-view-css', ), @@ -2335,7 +2325,6 @@ return array( 'differential.pkg.js' => array( 'phabricator-drag-and-drop-file-upload', 'phabricator-shaped-request', - 'javelin-behavior-differential-feedback-preview', 'javelin-behavior-differential-populate', 'javelin-behavior-differential-diff-radios', 'javelin-behavior-aphront-drag-and-drop-textarea', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index 958b1d6afa..4005e064ba 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -193,7 +193,6 @@ return array( 'phabricator-drag-and-drop-file-upload', 'phabricator-shaped-request', - 'javelin-behavior-differential-feedback-preview', 'javelin-behavior-differential-populate', 'javelin-behavior-differential-diff-radios', 'javelin-behavior-aphront-drag-and-drop-textarea', diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 8c4a557e83..9aa60872dc 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1958,11 +1958,19 @@ abstract class PhabricatorEditEngine if ($request->isAjax() && $is_preview) { $preview_content = $this->newCommentPreviewContent($object, $xactions); + $raw_view_data = $request->getStr('viewData'); + try { + $view_data = phutil_json_decode($raw_view_data); + } catch (Exception $ex) { + $view_data = array(); + } + return id(new PhabricatorApplicationTransactionResponse()) ->setObject($object) ->setViewer($viewer) ->setTransactions($xactions) ->setIsPreview($is_preview) + ->setViewData($view_data) ->setPreviewContent($preview_content); } else { return id(new AphrontRedirectResponse()) diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php index 5d81314532..00adb3a388 100644 --- a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php +++ b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php @@ -8,6 +8,7 @@ final class PhabricatorApplicationTransactionResponse private $isPreview; private $previewContent; private $object; + private $viewData = array(); protected function buildProxy() { return new AphrontAjaxResponse(); @@ -56,6 +57,15 @@ final class PhabricatorApplicationTransactionResponse return $this->previewContent; } + public function setViewData(array $view_data) { + $this->viewData = $view_data; + return $this; + } + + public function getViewData() { + return $this->viewData; + } + public function reduceProxyResponse() { $object = $this->getObject(); $viewer = $this->getViewer(); @@ -63,7 +73,8 @@ final class PhabricatorApplicationTransactionResponse $timeline_engine = PhabricatorTimelineEngine::newForObject($object) ->setViewer($viewer) - ->setTransactions($xactions); + ->setTransactions($xactions) + ->setViewData($this->viewData); $view = $timeline_engine->buildTimelineView(); diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index 227854c79f..d830309119 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -391,6 +391,13 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { $form->appendChild($invisi_bar); $form->addClass('phui-comment-has-actions'); + $timeline = $this->transactionTimeline; + + $view_data = array(); + if ($timeline) { + $view_data = $timeline->getViewData(); + } + Javelin::initBehavior( 'comment-actions', array( @@ -405,6 +412,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'actionURI' => $this->getAction(), 'drafts' => $draft_keys, 'defaultButtonText' => $this->getSubmitButtonName(), + 'viewData' => $view_data, )); } diff --git a/webroot/rsrc/js/application/differential/behavior-comment-preview.js b/webroot/rsrc/js/application/differential/behavior-comment-preview.js deleted file mode 100644 index beb9f9a5d9..0000000000 --- a/webroot/rsrc/js/application/differential/behavior-comment-preview.js +++ /dev/null @@ -1,116 +0,0 @@ -/** - * @provides javelin-behavior-differential-feedback-preview - * @requires javelin-behavior - * javelin-stratcom - * javelin-dom - * javelin-request - * javelin-util - * phabricator-shaped-request - */ - -JX.behavior('differential-feedback-preview', function(config) { - - var action = JX.$(config.action); - var content = JX.$(config.content); - var previewTokenizers = {}; - var field; - for (field in config.previewTokenizers) { - var tokenizer = JX.$(config.previewTokenizers[field]); - previewTokenizers[field] = JX.Stratcom.getData(tokenizer).tokenizer; - } - - var callback = function(r) { - var preview = JX.$(config.preview); - var data = getdata(); - var hide = true; - for (var field in data) { - if (field == 'action') { - continue; - } - if (data[field]) { - hide = false; - } - } - if (hide) { - JX.DOM.hide(preview); - } else { - JX.DOM.setContent(preview, JX.$H(r)); - JX.Stratcom.invoke('differential-preview-update', null, { - container: preview - }); - JX.DOM.show(preview); - } - }; - - var getdata = function() { - var data = { - content : content.value, - action : action.value - }; - for (var field in previewTokenizers) { - data[field] = JX.keys(previewTokenizers[field].getTokens()).join(','); - } - return data; - }; - - var request = new JX.PhabricatorShapedRequest(config.uri, callback, getdata); - var trigger = JX.bind(request, request.trigger); - - JX.DOM.listen(content, 'keydown', null, trigger); - JX.DOM.listen(action, 'change', null, trigger); - for (field in previewTokenizers) { - previewTokenizers[field].listen('change', trigger); - } - - request.start(); - - function refreshInlinePreview() { - new JX.Request(config.inlineuri, function(r) { - var inline = JX.$(config.inline); - - JX.DOM.setContent(inline, JX.$H(r)); - JX.Stratcom.invoke('differential-preview-update', null, { - container: inline - }); - - updateLinks(); - - JX.Stratcom.invoke('resize'); - }) - .setTimeout(5000) - .send(); - } - - function updateLinks() { - var inline = JX.$(config.inline); - - var links = JX.DOM.scry( - inline, - 'a', - 'differential-inline-preview-jump'); - - for (var ii = 0; ii < links.length; ii++) { - var data = JX.Stratcom.getData(links[ii]); - try { - JX.$(data.anchor); - links[ii].href = '#' + data.anchor; - JX.DOM.setContent(links[ii], 'View'); - } catch (ignored) { - // This inline comment isn't visible, e.g. on some other diff. - } - } - } - - - JX.Stratcom.listen( - 'differential-inline-comment-update', - null, - refreshInlinePreview); - - JX.Stratcom.listen( - 'differential-inline-comment-refresh', - null, - updateLinks); - - refreshInlinePreview(); -}); diff --git a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js index ab962592c7..e5d7b27b25 100644 --- a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js +++ b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js @@ -90,6 +90,8 @@ JX.behavior('comment-actions', function(config) { data.__preview__ = 1; data[input_node.name] = serialize_actions(); + data.viewData = JX.JSON.stringify(config.viewData); + return data; }