From 9f2efd0fa04807514e2202154e4a49e845a014cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 21 Jun 2014 12:50:40 -0700 Subject: [PATCH] Remove ajaxey comment magic in Pholio, and also some bugs Summary: Fixes T5424. - One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts. - This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now. - Also remove it from other applications where it's currently used. - Fix an issue with inline previews. Test Plan: Made some comments on a mock, everything worked normally like I expected it to. Reviewers: chad Reviewed By: chad Subscribers: epriestley Maniphest Tasks: T5424 Differential Revision: https://secure.phabricator.com/D9649 --- resources/celerity/map.php | 46 ++++---- .../PhabricatorFileCommentController.php | 2 +- .../LegalpadDocumentCommentController.php | 2 +- .../PhabricatorMacroCommentController.php | 2 +- .../PhabricatorPasteCommentController.php | 2 +- .../PholioMockCommentController.php | 6 +- .../PonderAnswerCommentController.php | 2 +- .../PonderQuestionCommentController.php | 2 +- .../ReleephRequestCommentController.php | 2 +- .../PhabricatorSlowvoteCommentController.php | 2 +- ...bricatorApplicationTransactionResponse.php | 14 ++- ...catorApplicationTransactionCommentView.php | 6 +- .../behavior-transaction-comment-form.js | 32 +----- .../transactions/behavior-transaction-list.js | 107 ------------------ 14 files changed, 53 insertions(+), 174 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index eed28a0957..72a1d9a6b9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => '8feeb38a', - 'core.pkg.js' => '07b01d4f', + 'core.pkg.js' => 'b440d8d7', 'darkconsole.pkg.js' => 'ca8671ce', 'differential.pkg.css' => '4a93db37', 'differential.pkg.js' => '5b252007', @@ -418,8 +418,8 @@ return array( 'rsrc/js/application/repository/repository-crossreference.js' => '8ab282be', 'rsrc/js/application/search/behavior-reorder-queries.js' => '37871df4', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => 'a51fdb2e', - 'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => '9084a36f', - 'rsrc/js/application/transactions/behavior-transaction-list.js' => 'cf656c84', + 'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => '9f7309fb', + 'rsrc/js/application/transactions/behavior-transaction-list.js' => 'fd6c2b32', 'rsrc/js/application/uiexample/JavelinViewExample.js' => 'd4a14807', 'rsrc/js/application/uiexample/ReactorButtonExample.js' => '44524435', 'rsrc/js/application/uiexample/ReactorCheckboxExample.js' => '7ba325ee', @@ -621,8 +621,8 @@ return array( 'javelin-behavior-phabricator-search-typeahead' => 'fbeabd1e', 'javelin-behavior-phabricator-show-all-transactions' => '7c273581', 'javelin-behavior-phabricator-tooltips' => '48db4145', - 'javelin-behavior-phabricator-transaction-comment-form' => '9084a36f', - 'javelin-behavior-phabricator-transaction-list' => 'cf656c84', + 'javelin-behavior-phabricator-transaction-comment-form' => '9f7309fb', + 'javelin-behavior-phabricator-transaction-list' => 'fd6c2b32', 'javelin-behavior-phabricator-watch-anchor' => '06e05112', 'javelin-behavior-phame-post-preview' => '61d927ec', 'javelin-behavior-pholio-mock-edit' => '1e1e8bb0', @@ -1520,15 +1520,6 @@ return array( 3 => 'javelin-mask', 4 => 'phabricator-drag-and-drop-file-upload', ), - '9084a36f' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-dom', - 2 => 'javelin-util', - 3 => 'javelin-fx', - 4 => 'javelin-request', - 5 => 'phabricator-shaped-request', - ), '929d95eb' => array( 0 => 'javelin-behavior', @@ -1595,6 +1586,14 @@ return array( 2 => 'javelin-uri', 3 => 'javelin-request', ), + '9f7309fb' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-dom', + 2 => 'javelin-util', + 3 => 'javelin-request', + 4 => 'phabricator-shaped-request', + ), 'a3e2244e' => array( 0 => 'javelin-behavior', @@ -1851,16 +1850,6 @@ return array( 0 => 'javelin-install', 1 => 'javelin-typeahead-source', ), - 'cf656c84' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-stratcom', - 2 => 'javelin-workflow', - 3 => 'javelin-dom', - 4 => 'javelin-fx', - 5 => 'javelin-uri', - 6 => 'phabricator-textareautils', - ), 'cf76cfd5' => array( 0 => 'javelin-behavior', @@ -2093,6 +2082,15 @@ return array( 5 => 'javelin-util', 6 => 'javelin-stratcom', ), + 'fd6c2b32' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-workflow', + 3 => 'javelin-dom', + 4 => 'javelin-uri', + 5 => 'phabricator-textareautils', + ), 'fe2e0ba4' => array( 0 => 'javelin-behavior', diff --git a/src/applications/files/controller/PhabricatorFileCommentController.php b/src/applications/files/controller/PhabricatorFileCommentController.php index ab47f9fe0e..1895f6c75d 100644 --- a/src/applications/files/controller/PhabricatorFileCommentController.php +++ b/src/applications/files/controller/PhabricatorFileCommentController.php @@ -58,7 +58,7 @@ final class PhabricatorFileCommentController $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) diff --git a/src/applications/legalpad/controller/LegalpadDocumentCommentController.php b/src/applications/legalpad/controller/LegalpadDocumentCommentController.php index a7b9efa66d..c5b5613b4f 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentCommentController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentCommentController.php @@ -68,7 +68,7 @@ final class LegalpadDocumentCommentController extends LegalpadController { $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) diff --git a/src/applications/macro/controller/PhabricatorMacroCommentController.php b/src/applications/macro/controller/PhabricatorMacroCommentController.php index 3d40791c1d..a53adc689e 100644 --- a/src/applications/macro/controller/PhabricatorMacroCommentController.php +++ b/src/applications/macro/controller/PhabricatorMacroCommentController.php @@ -55,7 +55,7 @@ final class PhabricatorMacroCommentController $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) diff --git a/src/applications/paste/controller/PhabricatorPasteCommentController.php b/src/applications/paste/controller/PhabricatorPasteCommentController.php index 4590d6f70c..f87795ce98 100644 --- a/src/applications/paste/controller/PhabricatorPasteCommentController.php +++ b/src/applications/paste/controller/PhabricatorPasteCommentController.php @@ -58,7 +58,7 @@ final class PhabricatorPasteCommentController $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) diff --git a/src/applications/pholio/controller/PholioMockCommentController.php b/src/applications/pholio/controller/PholioMockCommentController.php index 3fd514205e..62d717abb0 100644 --- a/src/applications/pholio/controller/PholioMockCommentController.php +++ b/src/applications/pholio/controller/PholioMockCommentController.php @@ -76,10 +76,14 @@ final class PholioMockCommentController extends PholioController { $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { + $xaction_view = id(new PholioTransactionView()) + ->setMock($mock); + return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) + ->setTransactionView($xaction_view) ->setIsPreview($is_preview) ->setAnchorOffset($request->getStr('anchor')); } else { diff --git a/src/applications/ponder/controller/PonderAnswerCommentController.php b/src/applications/ponder/controller/PonderAnswerCommentController.php index e05c319ea6..6c22111833 100644 --- a/src/applications/ponder/controller/PonderAnswerCommentController.php +++ b/src/applications/ponder/controller/PonderAnswerCommentController.php @@ -56,7 +56,7 @@ final class PonderAnswerCommentController extends PonderController { // $draft->replaceOrDelete(); // } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($viewer) ->setTransactions($xactions) diff --git a/src/applications/ponder/controller/PonderQuestionCommentController.php b/src/applications/ponder/controller/PonderQuestionCommentController.php index 173f277bb7..e2cf806be0 100644 --- a/src/applications/ponder/controller/PonderQuestionCommentController.php +++ b/src/applications/ponder/controller/PonderQuestionCommentController.php @@ -55,7 +55,7 @@ final class PonderQuestionCommentController extends PonderController { // $draft->replaceOrDelete(); // } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($viewer) ->setTransactions($xactions) diff --git a/src/applications/releeph/controller/request/ReleephRequestCommentController.php b/src/applications/releeph/controller/request/ReleephRequestCommentController.php index cb60a61356..5e7e6e7b2c 100644 --- a/src/applications/releeph/controller/request/ReleephRequestCommentController.php +++ b/src/applications/releeph/controller/request/ReleephRequestCommentController.php @@ -55,7 +55,7 @@ final class ReleephRequestCommentController $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($viewer) ->setTransactions($xactions) diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php index a0b0a52051..f0046df374 100644 --- a/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php +++ b/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php @@ -55,7 +55,7 @@ final class PhabricatorSlowvoteCommentController $draft->replaceOrDelete(); } - if ($request->isAjax()) { + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($user) ->setTransactions($xactions) diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php index 44e5baa300..cf936d9a11 100644 --- a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php +++ b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php @@ -7,6 +7,16 @@ final class PhabricatorApplicationTransactionResponse private $transactions; private $anchorOffset; private $isPreview; + private $transactionView; + + public function setTransactionView($transaction_view) { + $this->transactionView = $transaction_view; + return $this; + } + + public function getTransactionView() { + return $this->transactionView; + } protected function buildProxy() { return new AphrontAjaxResponse(); @@ -47,7 +57,9 @@ final class PhabricatorApplicationTransactionResponse } public function reduceProxyResponse() { - if ($this->getTransactions()) { + if ($this->transactionView) { + $view = $this->transactionView; + } else if ($this->getTransactions()) { $view = head($this->getTransactions()) ->getApplicationTransactionViewObject(); } else { diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index a8bc51febf..f85daa0773 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -124,9 +124,6 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'showPreview' => $this->getShowPreview(), 'actionURI' => $this->getAction(), - 'draftKey' => $this->getDraft() - ? $this->getDraft()->getDraftKey() - : null, )); $comment_box = id(new PHUIObjectBoxView()) @@ -146,8 +143,10 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { ''); $draft_comment = ''; + $draft_key = null; if ($this->getDraft()) { $draft_comment = $this->getDraft()->getDraft(); + $draft_key = $this->getDraft()->getDraftKey(); } if (!$this->getObjectPHID()) { @@ -164,6 +163,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { )) ->setAction($this->getAction()) ->setID($this->getFormID()) + ->addHiddenInput('__draft__', $draft_key) ->appendChild( id(new PhabricatorRemarkupControl()) ->setID($this->getCommentID()) diff --git a/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js b/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js index 93f5da08ad..119ec02d21 100644 --- a/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js +++ b/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js @@ -3,7 +3,6 @@ * @requires javelin-behavior * javelin-dom * javelin-util - * javelin-fx * javelin-request * phabricator-shaped-request */ @@ -12,36 +11,9 @@ JX.behavior('phabricator-transaction-comment-form', function(config) { var form = JX.$(config.formID); - JX.DOM.listen(form, 'willSubmit', null, function (e) { - e.kill(); - if (config.showPreview) { - var preview = JX.$(config.panelID); - preview.style.opacity = 0.5; - } - }); - - JX.DOM.listen(form, 'willClear', null, function(e) { - JX.$(config.commentID).value = ''; - - if (config.showPreview) { - var preview = JX.$(config.panelID); - new JX.FX(preview) - .setDuration(500) - .then(function () { - new JX.FX(preview).setDuration(1000).start({opacity: [0, 1]}); - }) - .start({opacity: [0.5, 0]}); - } - }); - var getdata = function() { var obj = JX.DOM.convertFormToDictionary(form); obj.__preview__ = 1; - - if (config.draftKey) { - obj.__draft__ = config.draftKey; - } - return obj; }; @@ -70,8 +42,8 @@ JX.behavior('phabricator-transaction-comment-form', function(config) { JX.DOM.listen(form, 'keydown', null, trigger); var always_trigger = function() { new JX.Request(config.actionURI, onresponse) - .setData(getdata()) - .send(); + .setData(getdata()) + .send(); }; JX.DOM.listen(form, 'shouldRefresh', null, always_trigger); diff --git a/webroot/rsrc/js/application/transactions/behavior-transaction-list.js b/webroot/rsrc/js/application/transactions/behavior-transaction-list.js index 8c56da6a74..b101f703fa 100644 --- a/webroot/rsrc/js/application/transactions/behavior-transaction-list.js +++ b/webroot/rsrc/js/application/transactions/behavior-transaction-list.js @@ -4,7 +4,6 @@ * javelin-stratcom * javelin-workflow * javelin-dom - * javelin-fx * javelin-uri * phabricator-textareautils */ @@ -15,55 +14,6 @@ JX.behavior('phabricator-transaction-list', function(config) { var xaction_nodes = null; var next_anchor = config.nextAnchor; - function get_xaction_nodes() { - if (xaction_nodes === null) { - xaction_nodes = {}; - var xactions = JX.DOM.scry(list, 'div', 'transaction'); - for (var ii = 0; ii < xactions.length; ii++) { - xaction_nodes[JX.Stratcom.getData(xactions[ii]).phid] = xactions[ii]; - } - } - return xaction_nodes; - } - - function ontransactions(response) { - var fade_in = []; - var first_new = null; - - var nodes = get_xaction_nodes(); - for (var phid in response.xactions) { - var new_node = JX.$H(response.xactions[phid]).getFragment().firstChild; - fade_in.push(new_node); - - if (nodes[phid]) { - JX.DOM.replace(nodes[phid], new_node); - } else { - if (first_new === null) { - first_new = new_node; - } - list.appendChild(new_node); - - // Add a spacer after new transactions. - var spacer = JX.$H(response.spacer).getFragment().firstChild; - list.appendChild(spacer); - fade_in.push(spacer); - - next_anchor++; - } - nodes[phid] = new_node; - } - - // Scroll to the first new transaction, if transactions were added. - if (first_new) { - JX.DOM.scrollTo(first_new); - } - - // Make any new or updated transactions fade in. - for (var ii = 0; ii < fade_in.length; ii++) { - new JX.FX(fade_in[ii]).setDuration(500).start({opacity: [0, 1]}); - } - } - JX.Stratcom.listen( 'click', [['transaction-edit'], ['transaction-remove']], @@ -120,61 +70,4 @@ JX.behavior('phabricator-transaction-list', function(config) { .start(); }); - JX.Stratcom.listen( - ['submit', 'didSyntheticSubmit'], - 'transaction-append', - function(e) { - var form = e.getTarget(); - if (JX.Stratcom.getData(form).objectPHID != config.objectPHID) { - // This indicates there are several forms on the page, and the user - // submitted a different one than the one we're in control of. - return; - } - - e.kill(); - - JX.DOM.invoke(form, 'willSubmit'); - - JX.Workflow.newFromForm(form, { anchor : next_anchor }) - .setHandler(function(response) { - ontransactions(response); - - var e = JX.DOM.invoke(form, 'willClear'); - if (!e.getPrevented()) { - var ii; - var textareas = JX.DOM.scry(form, 'textarea'); - for (ii = 0; ii < textareas.length; ii++) { - textareas[ii].value = ''; - } - - var inputs = JX.DOM.scry(form, 'input'); - for (ii = 0; ii < inputs.length; ii++) { - switch (inputs[ii].type) { - case 'password': - case 'text': - inputs[ii].value = ''; - break; - case 'checkbox': - case 'radio': - inputs[ii].checked = false; - break; - } - } - - var selects = JX.DOM.scry(form, 'select'); - var jj; - for (ii = 0; ii < selects.length; ii++) { - if (selects[ii].type == 'select-one') { - selects[ii].selectedIndex = 0; - } else { - for (jj = 0; jj < selects[ii].options.length; jj++) { - selects[ii].options[jj].selected = false; - } - } - } - } - }) - .start(); - - }); });