mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-21 22:32:41 +01:00
Update client logic for inline comment "Save" and "Cancel" actions
Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases. Test Plan: Quoting behavior: - Quoted a comment. - Cancelled the quoted comment without modifying anything. - Reloaded page. - Before changes: quoted comment still exists. - After changes: quoted comment is deleted. - Looked at comment count in header, saw consistent behavior (before: weird behavior). Empty suggestion behavior: - Created a new comment on a suggestable file. - Clicked "Suggest Edit" to enable suggestions. - Without making any text or suggestion changes, clicked "Save". - Before changes: comment saves, but is empty. - After changes: comment deletes itself without undo. General behavior: - Created and saved an empty comment (deletes itself). - Created and saved a nonempty comment (saves as draft). - Created and saved an empty comment with an edit suggestion (saves). - Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves). - Edited a comment, saved without changes (save). - Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident). - Cancel editing an unchanged comment (cancels without undo). - Cancel editing a changed comment (cancels with undo). - Undo'd, got text back. - Cancel new comment with no text (deletes without undo). - Cancel new comment with text (deletes with undo). - Undo'd, got text back. - Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later). Maniphest Tasks: T13559 Differential Revision: https://secure.phabricator.com/D21654
This commit is contained in:
epriestley
parent
6fd55d692f
commit
1308a5555f
4 changed files with 179 additions and 104 deletions
|
|
@ -13,7 +13,7 @@ return array(
|
|||
'core.pkg.js' => '68f29322',
|
||||
'dark-console.pkg.js' => '187792c2',
|
||||
'differential.pkg.css' => 'ffb69e3d',
|
||||
'differential.pkg.js' => '59453886',
|
||||
'differential.pkg.js' => '8deec4cd',
|
||||
'diffusion.pkg.css' => '42c75c37',
|
||||
'diffusion.pkg.js' => '78c9885d',
|
||||
'maniphest.pkg.css' => '35995d6d',
|
||||
|
|
@ -385,8 +385,8 @@ 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' => '26664c24',
|
||||
'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
|
||||
'rsrc/js/application/diff/DiffInline.js' => '9c775532',
|
||||
'rsrc/js/application/diff/DiffInlineContentState.js' => 'aa51efb4',
|
||||
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
|
||||
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
|
||||
'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
|
||||
|
|
@ -788,8 +788,8 @@ return array(
|
|||
'phabricator-dashboard-css' => '5a205b9d',
|
||||
'phabricator-diff-changeset' => 'd7d3ba75',
|
||||
'phabricator-diff-changeset-list' => 'cc2c5de5',
|
||||
'phabricator-diff-inline' => '26664c24',
|
||||
'phabricator-diff-inline-content-state' => '68e6339d',
|
||||
'phabricator-diff-inline' => '9c775532',
|
||||
'phabricator-diff-inline-content-state' => 'aa51efb4',
|
||||
'phabricator-diff-path-view' => '8207abf9',
|
||||
'phabricator-diff-tree-view' => '5d83623b',
|
||||
'phabricator-drag-and-drop-file-upload' => '4370900d',
|
||||
|
|
@ -1162,10 +1162,6 @@ return array(
|
|||
'javelin-json',
|
||||
'phabricator-prefab',
|
||||
),
|
||||
'26664c24' => array(
|
||||
'javelin-dom',
|
||||
'phabricator-diff-inline-content-state',
|
||||
),
|
||||
'289bf236' => array(
|
||||
'javelin-install',
|
||||
'javelin-util',
|
||||
|
|
@ -1549,9 +1545,6 @@ return array(
|
|||
'javelin-install',
|
||||
'javelin-dom',
|
||||
),
|
||||
'68e6339d' => array(
|
||||
'javelin-dom',
|
||||
),
|
||||
'6a1583a8' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-history',
|
||||
|
|
@ -1823,6 +1816,10 @@ return array(
|
|||
'javelin-dom',
|
||||
'javelin-workflow',
|
||||
),
|
||||
'9c775532' => array(
|
||||
'javelin-dom',
|
||||
'phabricator-diff-inline-content-state',
|
||||
),
|
||||
'9cec214e' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-stratcom',
|
||||
|
|
@ -1913,6 +1910,9 @@ return array(
|
|||
'javelin-typeahead-ondemand-source',
|
||||
'javelin-dom',
|
||||
),
|
||||
'aa51efb4' => array(
|
||||
'javelin-dom',
|
||||
),
|
||||
'aa6d2308' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-dom',
|
||||
|
|
|
|||
|
|
@ -172,7 +172,9 @@ abstract class PhabricatorInlineCommentController
|
|||
$inline = $this->loadCommentByIDForEdit($this->getCommentID());
|
||||
|
||||
if ($is_delete) {
|
||||
$inline->setIsDeleted(1);
|
||||
$inline
|
||||
->setIsEditing(false)
|
||||
->setIsDeleted(1);
|
||||
} else {
|
||||
$inline->setIsDeleted(0);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -44,7 +44,6 @@ JX.install('DiffInline', {
|
|||
_undoRow: null,
|
||||
_undoType: null,
|
||||
_undoState: null,
|
||||
_preventUndo: false,
|
||||
|
||||
_draftRequest: null,
|
||||
_skipFocus: false,
|
||||
|
|
@ -161,14 +160,6 @@ JX.install('DiffInline', {
|
|||
return this._endOffset;
|
||||
},
|
||||
|
||||
_setPreventUndo: function(prevent_undo) {
|
||||
this._preventUndo = prevent_undo;
|
||||
},
|
||||
|
||||
_getPreventUndo: function() {
|
||||
return this._preventUndo;
|
||||
},
|
||||
|
||||
setIsSelected: function(is_selected) {
|
||||
this._isSelected = is_selected;
|
||||
|
||||
|
|
@ -452,21 +443,12 @@ JX.install('DiffInline', {
|
|||
this._undoText = null;
|
||||
}
|
||||
|
||||
var uri = this._getInlineURI();
|
||||
var handler = JX.bind(this, this._oneditresponse);
|
||||
|
||||
var data = this._newRequestData('edit', content_state);
|
||||
|
||||
this.setLoading(true);
|
||||
|
||||
new JX.Request(uri, handler)
|
||||
.setData(data)
|
||||
.send();
|
||||
this._applyEdit(content_state);
|
||||
},
|
||||
|
||||
delete: function(is_ref) {
|
||||
var uri = this._getInlineURI();
|
||||
var handler = JX.bind(this, this._ondeleteresponse);
|
||||
var handler = JX.bind(this, this._ondeleteresponse, false);
|
||||
|
||||
// NOTE: This may be a direct delete (the user clicked on the inline
|
||||
// itself) or a "refdelete" (the user clicked somewhere else, like the
|
||||
|
|
@ -586,7 +568,6 @@ JX.install('DiffInline', {
|
|||
this._readInlineState(response.inline);
|
||||
this._drawEditRows(rows);
|
||||
|
||||
this.setLoading(false);
|
||||
this.setInvisible(true);
|
||||
},
|
||||
|
||||
|
|
@ -617,7 +598,8 @@ JX.install('DiffInline', {
|
|||
return new JX.DiffInlineContentState().readWireFormat(map);
|
||||
},
|
||||
|
||||
_ondeleteresponse: function() {
|
||||
_ondeleteresponse: function(prevent_undo) {
|
||||
if (!prevent_undo) {
|
||||
// If there's an existing "unedit" undo element, remove it.
|
||||
if (this._undoRow) {
|
||||
JX.DOM.remove(this._undoRow);
|
||||
|
|
@ -634,7 +616,6 @@ JX.install('DiffInline', {
|
|||
this._editRow = null;
|
||||
}
|
||||
|
||||
if (!this._getPreventUndo()) {
|
||||
this._drawUndeleteRows(state);
|
||||
}
|
||||
|
||||
|
|
@ -693,7 +674,6 @@ JX.install('DiffInline', {
|
|||
var anchor = cursor || this._row;
|
||||
cursor = cursor || this._row.nextSibling;
|
||||
|
||||
|
||||
var result_row;
|
||||
var next_row;
|
||||
while (row) {
|
||||
|
|
@ -837,8 +817,10 @@ JX.install('DiffInline', {
|
|||
|
||||
save: function() {
|
||||
if (this._shouldDeleteOnSave()) {
|
||||
this._setPreventUndo(true);
|
||||
this._applyDelete();
|
||||
JX.DOM.remove(this._editRow);
|
||||
this._editRow = null;
|
||||
|
||||
this._applyDelete(true);
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -846,35 +828,56 @@ JX.install('DiffInline', {
|
|||
},
|
||||
|
||||
_shouldDeleteOnSave: function() {
|
||||
var state = this._getActiveContentState();
|
||||
var active = this._getActiveContentState();
|
||||
var initial = this._getInitialContentState();
|
||||
|
||||
// TODO: This is greatly simplified because we don't track all the
|
||||
// state we need yet.
|
||||
// When a user clicks "Save", it counts as a "delete" if the content
|
||||
// of the comment is functionally empty.
|
||||
|
||||
return !state.getText().length;
|
||||
},
|
||||
// This isn't a delete if there's any text. Even if the text is a
|
||||
// quote (so the state is the same as the initial state), we preserve
|
||||
// it when the user clicks "Save".
|
||||
if (!active.isTextEmpty()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
_shouldDeleteOnCancel: function() {
|
||||
var state = this._getActiveContentState();
|
||||
// This isn't a delete if there's a suggestion and that suggestion is
|
||||
// different from the initial state. (This means that an inline which
|
||||
// purely suggests a block of code should be deleted is non-empty.)
|
||||
if (active.getHasSuggestion()) {
|
||||
if (!active.isSuggestionSimilar(initial)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// TODO: This is greatly simplified, too.
|
||||
|
||||
return !state.getText().length;
|
||||
// Otherwise, this comment is functionally empty, so we can just treat
|
||||
// a "Save" as a "delete".
|
||||
return true;
|
||||
},
|
||||
|
||||
_shouldUndoOnCancel: function() {
|
||||
var new_state = this._getActiveContentState().getWireFormat();
|
||||
var old_state = this._getCommittedContentState().getWireFormat();
|
||||
var committed = this._getCommittedContentState();
|
||||
var active = this._getActiveContentState();
|
||||
var initial = this._getInitialContentState();
|
||||
|
||||
// TODO: This is also simplified.
|
||||
// When a user clicks "Cancel", we only offer to let them "Undo" the
|
||||
// action if the undo would be substantive.
|
||||
|
||||
var is_empty = this._isVoidContentState(new_state);
|
||||
var is_same = this._isSameContentState(new_state, old_state);
|
||||
|
||||
if (!is_empty && !is_same) {
|
||||
// The undo is substantive if the text is nonempty, and not similar to
|
||||
// the last state.
|
||||
var versus = committed || initial;
|
||||
if (!active.isTextEmpty() && !active.isTextSimilar(versus)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// The undo is substantive if there's a suggestion, and the suggestion
|
||||
// is not similar to the last state.
|
||||
if (active.getHasSuggestion()) {
|
||||
if (!active.isSuggestionSimilar(versus)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
},
|
||||
|
||||
|
|
@ -887,8 +890,8 @@ JX.install('DiffInline', {
|
|||
this._applyCall(handler, data);
|
||||
},
|
||||
|
||||
_applyDelete: function() {
|
||||
var handler = JX.bind(this, this._ondeleteresponse);
|
||||
_applyDelete: function(prevent_undo) {
|
||||
var handler = JX.bind(this, this._ondeleteresponse, prevent_undo);
|
||||
|
||||
var data = this._newRequestData('delete');
|
||||
|
||||
|
|
@ -903,6 +906,14 @@ JX.install('DiffInline', {
|
|||
this._applyCall(handler, data);
|
||||
},
|
||||
|
||||
_applyEdit: function(state) {
|
||||
var handler = JX.bind(this, this._oneditresponse);
|
||||
|
||||
var data = this._newRequestData('edit', state);
|
||||
|
||||
this._applyCall(handler, data);
|
||||
},
|
||||
|
||||
_applyCall: function(handler, data) {
|
||||
var uri = this._getInlineURI();
|
||||
|
||||
|
|
@ -946,24 +957,33 @@ JX.install('DiffInline', {
|
|||
},
|
||||
|
||||
cancel: function() {
|
||||
// NOTE: Read the state before we remove the editor. Otherwise, we might
|
||||
// miss text the user has entered into the textarea.
|
||||
var state = this._getActiveContentState().getWireFormat();
|
||||
|
||||
JX.DOM.remove(this._editRow);
|
||||
this._editRow = null;
|
||||
|
||||
if (this._shouldDeleteOnCancel()) {
|
||||
this._setPreventUndo(true);
|
||||
this._applyDelete();
|
||||
return;
|
||||
}
|
||||
// When a user clicks "Cancel", we delete the comment if it has never
|
||||
// been saved: we don't have a non-empty display state to revert to.
|
||||
var is_delete = (this._getCommittedContentState() === null);
|
||||
|
||||
if (this._shouldUndoOnCancel()) {
|
||||
var state = this._getActiveContentState().getWireFormat();
|
||||
this._drawUneditRows(state);
|
||||
}
|
||||
var is_undo = this._shouldUndoOnCancel();
|
||||
|
||||
// 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.
|
||||
|
||||
if (is_undo) {
|
||||
this._drawUneditRows(state);
|
||||
}
|
||||
|
||||
if (is_delete) {
|
||||
// NOTE: We're always suppressing the undo from "delete". We want to
|
||||
// use the "undo" we just added above instead, which will get us
|
||||
// back to the ephemeral, client-side editor state.
|
||||
this._applyDelete(true);
|
||||
} else {
|
||||
this.setEditing(false);
|
||||
this.setInvisible(false);
|
||||
|
||||
|
|
@ -971,6 +991,7 @@ JX.install('DiffInline', {
|
|||
this._applyCancel(old_state.getWireFormat());
|
||||
|
||||
this._didUpdate(true);
|
||||
}
|
||||
},
|
||||
|
||||
_onCancelResponse: function(response) {
|
||||
|
|
@ -1067,8 +1088,8 @@ JX.install('DiffInline', {
|
|||
return null;
|
||||
}
|
||||
|
||||
var state = this._getActiveContentState().getWireFormat();
|
||||
if (this._isVoidContentState(state)) {
|
||||
var state = this._getActiveContentState();
|
||||
if (state.isStateEmpty()) {
|
||||
return null;
|
||||
}
|
||||
|
||||
|
|
@ -1077,7 +1098,7 @@ JX.install('DiffInline', {
|
|||
id: this.getID(),
|
||||
};
|
||||
|
||||
JX.copy(draft_data, state);
|
||||
JX.copy(draft_data, state.getWireFormat());
|
||||
|
||||
return draft_data;
|
||||
},
|
||||
|
|
@ -1193,22 +1214,8 @@ JX.install('DiffInline', {
|
|||
suggestionText: '',
|
||||
hasSuggestion: false
|
||||
};
|
||||
},
|
||||
|
||||
_isVoidContentState: function(state) {
|
||||
if (!state.text) {
|
||||
return true;
|
||||
}
|
||||
return (!state.text.length && !state.suggestionText.length);
|
||||
},
|
||||
|
||||
_isSameContentState: function(u, v) {
|
||||
return (
|
||||
((u === null) === (v === null)) &&
|
||||
(u.text === v.text) &&
|
||||
(u.suggestionText === v.suggestionText) &&
|
||||
(u.hasSuggestion === v.hasSuggestion));
|
||||
}
|
||||
}
|
||||
|
||||
});
|
||||
|
|
|
|||
|
|
@ -59,6 +59,72 @@ JX.install('DiffInlineContentState', {
|
|||
return text;
|
||||
},
|
||||
|
||||
isStateEmpty: function() {
|
||||
return (this.isTextEmpty() && this.isSuggestionEmpty());
|
||||
},
|
||||
|
||||
isTextEmpty: function() {
|
||||
var text = this.getText();
|
||||
if (text === null) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (this._isStringSimilar(text, '')) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
},
|
||||
|
||||
isSuggestionEmpty: function() {
|
||||
if (!this.getHasSuggestion()) {
|
||||
return true;
|
||||
}
|
||||
|
||||
var suggestion = this.getSuggestionText();
|
||||
if (suggestion === null) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (this._isStringSimilar(suggestion, '')) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
},
|
||||
|
||||
isTextSimilar: function(v) {
|
||||
if (!v) {
|
||||
return false;
|
||||
}
|
||||
|
||||
var us = this.getText();
|
||||
var vs = v.getText();
|
||||
|
||||
return this._isStringSimilar(us, vs);
|
||||
},
|
||||
|
||||
isSuggestionSimilar: function(v) {
|
||||
// If we don't have a comparison state, treat them as dissimilar. This
|
||||
// is expected to occur in old inline comments that did not save an
|
||||
// initial state.
|
||||
|
||||
if (!v) {
|
||||
return false;
|
||||
}
|
||||
|
||||
var us = this.getSuggestionText();
|
||||
var vs = v.getSuggestionText();
|
||||
|
||||
return this._isStringSimilar(us, vs);
|
||||
},
|
||||
|
||||
_isStringSimilar: function(u, v) {
|
||||
u = u || '';
|
||||
v = v || '';
|
||||
return (u === v);
|
||||
},
|
||||
|
||||
_getSuggestionNode: function(row) {
|
||||
try {
|
||||
return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
|
||||
|
|
|
|||
Loading…
Reference in a new issue