D21654.id51545.diff
No OneTemporary
Actions

Size

14 KB

Referenced Files

None

Subscribers

None

D21654.id51545.diff
View Options

	diff --git a/resources/celerity/map.php b/resources/celerity/map.php
	--- a/resources/celerity/map.php
	+++ b/resources/celerity/map.php
	@@ -13,7 +13,7 @@
	'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 @@
	'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 @@
	'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 @@
	'javelin-json',
	'phabricator-prefab',
	),
	- '26664c24' => array(
	- 'javelin-dom',
	- 'phabricator-diff-inline-content-state',
	- ),
	'289bf236' => array(
	'javelin-install',
	'javelin-util',
	@@ -1549,9 +1545,6 @@
	'javelin-install',
	'javelin-dom',
	),
	- '68e6339d' => array(
	- 'javelin-dom',
	- ),
	'6a1583a8' => array(
	'javelin-behavior',
	'javelin-history',
	@@ -1823,6 +1816,10 @@
	'javelin-dom',
	'javelin-workflow',
	),
	+ '9c775532' => array(
	+ 'javelin-dom',
	+ 'phabricator-diff-inline-content-state',
	+ ),
	'9cec214e' => array(
	'javelin-behavior',
	'javelin-stratcom',
	@@ -1913,6 +1910,9 @@
	'javelin-typeahead-ondemand-source',
	'javelin-dom',
	),
	+ 'aa51efb4' => array(
	+ 'javelin-dom',
	+ ),
	'aa6d2308' => array(
	'javelin-behavior',
	'javelin-dom',
	diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php
	--- a/src/infrastructure/diff/PhabricatorInlineCommentController.php
	+++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php
	@@ -172,7 +172,9 @@
	$inline = $this->loadCommentByIDForEdit($this->getCommentID());

	if ($is_delete) {
	- $inline->setIsDeleted(1);
	+ $inline
	+ ->setIsEditing(false)
	+ ->setIsDeleted(1);
	} else {
	$inline->setIsDeleted(0);
	}
	diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
	--- a/webroot/rsrc/js/application/diff/DiffInline.js
	+++ b/webroot/rsrc/js/application/diff/DiffInline.js
	@@ -44,7 +44,6 @@
	_undoRow: null,
	_undoType: null,
	_undoState: null,
	- _preventUndo: false,

	_draftRequest: null,
	_skipFocus: false,
	@@ -161,14 +160,6 @@
	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 @@
	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 @@
	this._readInlineState(response.inline);
	this._drawEditRows(rows);

	- this.setLoading(false);
	this.setInvisible(true);
	},

	@@ -617,24 +598,24 @@
	return new JX.DiffInlineContentState().readWireFormat(map);
	},

	- _ondeleteresponse: function() {
	- // If there's an existing "unedit" undo element, remove it.
	- if (this._undoRow) {
	- JX.DOM.remove(this._undoRow);
	- this._undoRow = null;
	- }
	+ _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);
	+ this._undoRow = null;
	+ }

	- // If there's an existing editor, remove it. This happens when you
	- // delete a comment from the comment preview area. In this case, we
	- // read and preserve the text so "Undo" restores it.
	- var state = null;
	- if (this._editRow) {
	- state = this._getActiveContentState().getWireFormat();
	- JX.DOM.remove(this._editRow);
	- this._editRow = null;
	- }
	+ // If there's an existing editor, remove it. This happens when you
	+ // delete a comment from the comment preview area. In this case, we
	+ // read and preserve the text so "Undo" restores it.
	+ var state = null;
	+ if (this._editRow) {
	+ state = this._getActiveContentState().getWireFormat();
	+ JX.DOM.remove(this._editRow);
	+ this._editRow = null;
	+ }

	- if (!this._getPreventUndo()) {
	this._drawUndeleteRows(state);
	}

	@@ -693,7 +674,6 @@
	var anchor = cursor \|\| this._row;
	cursor = cursor \|\| this._row.nextSibling;

	-
	var result_row;
	var next_row;
	while (row) {
	@@ -837,8 +817,10 @@

	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 @@
	},

	_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;
	- },
	-
	- _shouldDeleteOnCancel: function() {
	- var state = this._getActiveContentState();
	+ // 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;
	+ }

	- // TODO: This is greatly simplified, too.
	+ // 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;
	+ }
	+ }

	- 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();
	-
	- // TODO: This is also simplified.
	+ var committed = this._getCommittedContentState();
	+ var active = this._getActiveContentState();
	+ var initial = this._getInitialContentState();

	- var is_empty = this._isVoidContentState(new_state);
	- var is_same = this._isSameContentState(new_state, old_state);
	+ // When a user clicks "Cancel", we only offer to let them "Undo" the
	+ // action if the undo would be substantive.

	- 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 @@
	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 @@
	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,31 +957,41 @@
	},

	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.

	- this.setEditing(false);
	- this.setInvisible(false);
	+ if (is_undo) {
	+ this._drawUneditRows(state);
	+ }

	- var old_state = this._getCommittedContentState();
	- this._applyCancel(old_state.getWireFormat());
	+ 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);

	- this._didUpdate(true);
	+ var old_state = this._getCommittedContentState();
	+ this._applyCancel(old_state.getWireFormat());
	+
	+ this._didUpdate(true);
	+ }
	},

	_onCancelResponse: function(response) {
	@@ -1067,8 +1088,8 @@
	return null;
	}

	- var state = this._getActiveContentState().getWireFormat();
	- if (this._isVoidContentState(state)) {
	+ var state = this._getActiveContentState();
	+ if (state.isStateEmpty()) {
	return null;
	}

	@@ -1077,7 +1098,7 @@
	id: this.getID(),
	};

	- JX.copy(draft_data, state);
	+ JX.copy(draft_data, state.getWireFormat());

	return draft_data;
	},
	@@ -1193,22 +1214,8 @@
	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));
	}
	+
	}

	});
	diff --git a/webroot/rsrc/js/application/diff/DiffInlineContentState.js b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
	--- a/webroot/rsrc/js/application/diff/DiffInlineContentState.js
	+++ b/webroot/rsrc/js/application/diff/DiffInlineContentState.js
	@@ -59,6 +59,72 @@
	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');

File Metadata

Mime Type: text/plain
Expires: Sat, Mar 8, 6:39 PM (3 d, 13 h ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 7383787
Default Alt Text: D21654.id51545.diff (14 KB)

D21654.id51545.diffNo OneTemporaryActions

D21654.id51545.diffView Options

File Metadata

Event Timeline

D21654.id51545.diff
No OneTemporary
Actions

D21654.id51545.diff
View Options