From 5efe7fb4c1812c949a48d3691ce446be637aa354 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Mar 2021 10:11:02 -0700 Subject: [PATCH] On inline comments, track an explicit "committed" content state Summary: Ref T13559. To allow the client to make correct decisions about what buttons mean, track an explicit "Committed" content state. This is the last version of the comment that has been saved on the server, and does not exist if the comment has never been saved. Test Plan: Created comments, etc. See followups. Maniphest Tasks: T13559 Differential Revision: https://secure.phabricator.com/D21651 --- resources/celerity/map.php | 30 ++++++++--------- .../rsrc/externals/javelin/lib/Resource.js | 4 +++ .../rsrc/js/application/diff/DiffInline.js | 32 +++++++++++++------ 3 files changed, 42 insertions(+), 24 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 99a8b77714..67887ef683 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,10 +10,10 @@ return array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '0ae696de', - 'core.pkg.js' => 'ab3502fe', + 'core.pkg.js' => '68f29322', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'ffb69e3d', - 'differential.pkg.js' => 'fdec5f60', + 'differential.pkg.js' => 'bbf6d742', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', 'maniphest.pkg.css' => '35995d6d', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/Mask.js' => '7c4d8998', 'rsrc/externals/javelin/lib/Quicksand.js' => 'd3799cb4', 'rsrc/externals/javelin/lib/Request.js' => '84e6891f', - 'rsrc/externals/javelin/lib/Resource.js' => '740956e1', + 'rsrc/externals/javelin/lib/Resource.js' => '20514cc2', 'rsrc/externals/javelin/lib/Routable.js' => '6a18c42e', 'rsrc/externals/javelin/lib/Router.js' => '32755edb', 'rsrc/externals/javelin/lib/Scrollbar.js' => 'a43ae2ae', @@ -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' => '2a6fac17', + 'rsrc/js/application/diff/DiffInline.js' => 'c794b624', 'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', @@ -734,7 +734,7 @@ return array( 'javelin-reactor-node-calmer' => '225bbb98', 'javelin-reactornode' => '72960bc1', 'javelin-request' => '84e6891f', - 'javelin-resource' => '740956e1', + 'javelin-resource' => '20514cc2', 'javelin-routable' => '6a18c42e', 'javelin-router' => '32755edb', 'javelin-scrollbar' => 'a43ae2ae', @@ -788,7 +788,7 @@ return array( 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => 'd7d3ba75', 'phabricator-diff-changeset-list' => 'cc2c5de5', - 'phabricator-diff-inline' => '2a6fac17', + 'phabricator-diff-inline' => 'c794b624', 'phabricator-diff-inline-content-state' => '68e6339d', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', @@ -1106,6 +1106,11 @@ return array( 'javelin-behavior', 'javelin-request', ), + '20514cc2' => array( + 'javelin-util', + 'javelin-uri', + 'javelin-install', + ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1166,10 +1171,6 @@ return array( 'javelin-stratcom', 'javelin-behavior', ), - '2a6fac17' => array( - 'javelin-dom', - 'phabricator-diff-inline-content-state', - ), '2a8b62d9' => array( 'multirow-row-manager', 'javelin-install', @@ -1597,11 +1598,6 @@ return array( 'javelin-stratcom', 'phabricator-tooltip', ), - '740956e1' => array( - 'javelin-util', - 'javelin-uri', - 'javelin-install', - ), 74446546 => array( 'javelin-behavior', 'javelin-dom', @@ -2092,6 +2088,10 @@ return array( 'javelin-workflow', 'javelin-json', ), + 'c794b624' => array( + 'javelin-dom', + 'phabricator-diff-inline-content-state', + ), 'cc2c5de5' => array( 'javelin-install', 'phuix-button-view', diff --git a/webroot/rsrc/externals/javelin/lib/Resource.js b/webroot/rsrc/externals/javelin/lib/Resource.js index 4e7d528740..68219e0389 100644 --- a/webroot/rsrc/externals/javelin/lib/Resource.js +++ b/webroot/rsrc/externals/javelin/lib/Resource.js @@ -149,6 +149,10 @@ JX.install('Resource', { } } + for (var jj = 0; jj < errors.length; jj++) { + JX.log(errors[jj]); + } + if (errors.length) { throw errors[0]; } diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 0255b00ed9..399f159864 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -9,6 +9,7 @@ JX.install('DiffInline', { construct : function() { this._activeContentState = new JX.DiffInlineContentState(); + this._committedContentState = new JX.DiffInlineContentState(); }, members: { @@ -21,7 +22,6 @@ JX.install('DiffInline', { _displaySide: null, _isNewFile: null, _replyToCommentPHID: null, - _originalState: null, _snippet: null, _menuItems: null, _documentEngineKey: null, @@ -56,6 +56,7 @@ JX.install('DiffInline', { _isSelected: false, _canSuggestEdit: false, + _committedContentState: null, _activeContentState: null, bindToRow: function(row) { @@ -336,8 +337,6 @@ JX.install('DiffInline', { this._phid = null; this._isCollapsed = false; - this._originalState = null; - return row; }, @@ -602,7 +601,10 @@ JX.install('DiffInline', { _readInlineState: function(state) { this._id = state.id; - this._originalState = state.contentState; + + // TODO: This is not the correct content state after a reload: it is + // the draft state. + this._getCommittedContentState().readWireFormat(state.contentState); this._getActiveContentState().readWireFormat(state.contentState); @@ -673,7 +675,10 @@ JX.install('DiffInline', { this._editRow = this._drawRows(rows, null, 'edit'); this._drawSuggestionState(this._editRow); - this.setHasSuggestion(this._originalState.hasSuggestion); + + // TODO: We're just doing this for the rendering side effect of drawing + // the button text. + this.setHasSuggestion(this.getHasSuggestion()); }, _drawRows: function(rows, cursor, type) { @@ -800,6 +805,10 @@ JX.install('DiffInline', { return state; }, + _getCommittedContentState: function() { + return this._committedContentState; + }, + setHasSuggestion: function(has_suggestion) { var state = this._getActiveContentState(); state.setHasSuggestion(has_suggestion); @@ -853,12 +862,13 @@ JX.install('DiffInline', { }, _shouldUndoOnCancel: function() { - var state = this._getActiveContentState().getWireFormat(); + var new_state = this._getActiveContentState().getWireFormat(); + var old_state = this._getCommittedContentState().getWireFormat(); // TODO: This is also simplified. - var is_empty = this._isVoidContentState(state); - var is_same = this._isSameContentState(state, this._originalState); + var is_empty = this._isVoidContentState(new_state); + var is_same = this._isSameContentState(new_state, old_state); if (!is_empty && !is_same) { return true; @@ -956,7 +966,8 @@ JX.install('DiffInline', { this.setEditing(false); this.setInvisible(false); - this._applyCancel(this._originalState); + var old_state = this._getCommittedContentState(); + this._applyCancel(old_state.getWireFormat()); this._didUpdate(true); }, @@ -1184,6 +1195,9 @@ JX.install('DiffInline', { }, _isVoidContentState: function(state) { + if (!state.text) { + return true; + } return (!state.text.length && !state.suggestionText.length); },