From d30c3a961cd9b11ef5f41c2859e4f825bb92fcb2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Mar 2021 12:39:08 -0700 Subject: [PATCH] Make the client authoritative for "Cancel" actions Summary: Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty). Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync. Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete". Test Plan: Cancelled empty and nonempty comments, etc. See followup changes. Maniphest Tasks: T13559 Differential Revision: https://secure.phabricator.com/D21649 --- resources/celerity/map.php | 14 ++-- .../PhabricatorInlineCommentController.php | 4 - .../rsrc/js/application/diff/DiffInline.js | 84 ++++++++++--------- 3 files changed, 53 insertions(+), 49 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8e94ccd067..99a8b77714 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => 'ab3502fe', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'ffb69e3d', - 'differential.pkg.js' => '7747755e', + 'differential.pkg.js' => 'fdec5f60', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', 'maniphest.pkg.css' => '35995d6d', @@ -385,7 +385,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', 'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75', 'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5', - 'rsrc/js/application/diff/DiffInline.js' => '34ccdeda', + 'rsrc/js/application/diff/DiffInline.js' => '2a6fac17', 'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', @@ -788,7 +788,7 @@ return array( 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => 'd7d3ba75', 'phabricator-diff-changeset-list' => 'cc2c5de5', - 'phabricator-diff-inline' => '34ccdeda', + 'phabricator-diff-inline' => '2a6fac17', 'phabricator-diff-inline-content-state' => '68e6339d', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', @@ -1166,6 +1166,10 @@ return array( 'javelin-stratcom', 'javelin-behavior', ), + '2a6fac17' => array( + 'javelin-dom', + 'phabricator-diff-inline-content-state', + ), '2a8b62d9' => array( 'multirow-row-manager', 'javelin-install', @@ -1220,10 +1224,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '34ccdeda' => array( - 'javelin-dom', - 'phabricator-diff-inline-content-state', - ), '34e2a838' => array( 'aphront-typeahead-control-css', 'phui-tag-view-css', diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 2076c1964e..353a8d4430 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -248,10 +248,6 @@ abstract class PhabricatorInlineCommentController // to set the stored state back to "A". $this->updateCommentContentState($inline); - if ($inline->isVoidComment($viewer)) { - $inline->setIsDeleted(1); - } - $this->saveComment($inline); return $this->buildEmptyResponse(); diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index c17217adfe..0255b00ed9 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -829,9 +829,10 @@ JX.install('DiffInline', { if (this._shouldDeleteOnSave()) { this._setPreventUndo(true); this._applyDelete(); - } else { - this._applySave(); + return; } + + this._applySave(); }, _shouldDeleteOnSave: function() { @@ -843,6 +844,29 @@ JX.install('DiffInline', { return !state.getText().length; }, + _shouldDeleteOnCancel: function() { + var state = this._getActiveContentState(); + + // TODO: This is greatly simplified, too. + + return !state.getText().length; + }, + + _shouldUndoOnCancel: function() { + var state = this._getActiveContentState().getWireFormat(); + + // TODO: This is also simplified. + + var is_empty = this._isVoidContentState(state); + var is_same = this._isSameContentState(state, this._originalState); + + if (!is_empty && !is_same) { + return true; + } + + return false; + }, + _applySave: function() { var handler = JX.bind(this, this._onsaveresponse); @@ -860,6 +884,14 @@ JX.install('DiffInline', { this._applyCall(handler, data); }, + _applyCancel: function(state) { + var handler = JX.bind(this, this._onCancelResponse); + + var data = this._newRequestData('cancel', state); + + this._applyCall(handler, data); + }, + _applyCall: function(handler, data) { var uri = this._getInlineURI(); @@ -903,57 +935,34 @@ JX.install('DiffInline', { }, cancel: function() { - var state = this._getActiveContentState().getWireFormat(); - JX.DOM.remove(this._editRow); this._editRow = null; - var is_empty = this._isVoidContentState(state); - var is_same = this._isSameContentState(state, this._originalState); - if (!is_empty && !is_same) { - this._drawUneditRows(state); + if (this._shouldDeleteOnCancel()) { + this._setPreventUndo(true); + this._applyDelete(); + return; } - // If this was an empty box and we typed some text and then hit cancel, - // don't show the empty concrete inline. - if (this._isVoidContentState(this._originalState)) { - this.setInvisible(true); - } else { - this.setInvisible(false); + if (this._shouldUndoOnCancel()) { + var state = this._getActiveContentState().getWireFormat(); + this._drawUneditRows(state); } // If you "undo" to restore text ("AB") and then "Cancel", we put you // back in the original text state ("A"). We also send the original // text ("A") to the server as the current persistent state. - var uri = this._getInlineURI(); - var data = this._newRequestData('cancel', this._originalState); - var handler = JX.bind(this, this._onCancelResponse); + this.setEditing(false); + this.setInvisible(false); - this.setLoading(true); - - new JX.Request(uri, handler) - .setData(data) - .send(); + this._applyCancel(this._originalState); this._didUpdate(true); }, _onCancelResponse: function(response) { - this.setEditing(false); - this.setLoading(false); - - // If the comment was empty when we started editing it (there's no - // original text) and empty when we finished editing it (there's no - // undo row), just delete the comment. - if (this._isVoidContentState(this._originalState) && !this.isUndo()) { - this.setDeleted(true); - - JX.DOM.remove(this._row); - this._row = null; - - this._didUpdate(); - } + // Nothing to do. }, _getSuggestionNode: function(row) { @@ -970,9 +979,8 @@ JX.install('DiffInline', { this._editRow = null; } - this.setLoading(false); - this.setInvisible(false); this.setEditing(false); + this.setInvisible(false); var new_row = this._drawContentRows(JX.$H(response.view).getNode()); JX.DOM.remove(this._row);