revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 00:32:42 +01:00
Code Issues Releases Wiki Activity

Make new DiffInline code handle most "delete" operations

Browse source 
Summary:
Ref T12616. This moves the delete actions to the new, more stateful way of doing things.

These are a little tricky because you can click "Delete" on an inline, but you can also click "Delete" from the preview area at the bottom of the page. If you do, the inline you are deleting may or may not be present on the page.

This has a few bugs -- notably, deleting from the preview without interacting with the on-page inline first won't actually delete the on-page inline yet -- but nothing too serious.

Test Plan: Deleted inlines, undid deletion, deleted from preview.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17890
This commit is contained in:
epriestley 2017-05-15 15:03:30 -07:00
parent 3c18cb77fb
commit d97f80bc90
5 changed files with 312 additions and 138 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

68
resources/celerity/map.php
View file

@ -13,7 +13,7 @@ return array(
'core.pkg.js' => '2ff7879f',
'darkconsole.pkg.js' => '1f9a31bc',
'differential.pkg.css' => '58712637',
'differential.pkg.js' => 'e01579c4',
'differential.pkg.js' => '271a1e1e',
'diffusion.pkg.css' => 'b93d9b8c',
'diffusion.pkg.js' => '84c8f8fd',
'favicon.ico' => '30672e08',
@ -390,15 +390,15 @@ return array(
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
'rsrc/js/application/diff/DiffChangeset.js' => '80ac3298',
'rsrc/js/application/diff/DiffChangesetList.js' => '12575699',
'rsrc/js/application/diff/DiffInline.js' => 'bd1b3258',
'rsrc/js/application/diff/DiffChangeset.js' => '57b29223',
'rsrc/js/application/diff/DiffChangesetList.js' => '50bc5b50',
'rsrc/js/application/diff/DiffInline.js' => '64dfc791',
'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '9ed8d2b6',
'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '97f363fc',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '9d9dbc38',
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '92904457',
'rsrc/js/application/differential/behavior-populate.js' => '5e41c819',
'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
@ -624,7 +624,7 @@ return array(
'javelin-behavior-diff-preview-link' => '051c7832',
'javelin-behavior-differential-comment-jump' => '4fdb476d',
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
'javelin-behavior-differential-edit-inline-comments' => '97f363fc',
'javelin-behavior-differential-edit-inline-comments' => '9d9dbc38',
'javelin-behavior-differential-feedback-preview' => 'b064af76',
'javelin-behavior-differential-keyboard-navigation' => '92904457',
'javelin-behavior-differential-populate' => '5e41c819',
@ -785,9 +785,9 @@ return array(
'phabricator-darklog' => 'c8e1ffe3',
'phabricator-darkmessage' => 'c48cccdd',
'phabricator-dashboard-css' => 'fe5b1869',
'phabricator-diff-changeset' => '80ac3298',
'phabricator-diff-changeset-list' => '12575699',
'phabricator-diff-inline' => 'bd1b3258',
'phabricator-diff-changeset' => '57b29223',
'phabricator-diff-changeset-list' => '50bc5b50',
'phabricator-diff-inline' => '64dfc791',
'phabricator-drag-and-drop-file-upload' => '58dea2fa',
'phabricator-draggable-list' => 'bea6e7f4',
'phabricator-fatal-config-template-css' => '8f18fa41',
@ -1000,9 +1000,6 @@ return array(
'javelin-dom',
'javelin-typeahead-normalizer',
),
12575699 => array(
'javelin-install',
),
'1499a8cb' => array(
'javelin-behavior',
'javelin-stratcom',
@ -1301,6 +1298,9 @@ return array(
'javelin-typeahead-source',
'javelin-util',
),
'50bc5b50' => array(
'javelin-install',
),
'519705ea' => array(
'javelin-install',
'javelin-dom',
@ -1342,6 +1342,17 @@ return array(
'javelin-vector',
'javelin-dom',
),
'57b29223' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-workflow',
'javelin-router',
'javelin-behavior-device',
'javelin-vector',
'phabricator-diff-inline',
),
'58dea2fa' => array(
'javelin-install',
'javelin-util',
@ -1411,6 +1422,9 @@ return array(
'javelin-workflow',
'javelin-dom',
),
'64dfc791' => array(
'javelin-dom',
),
'680ea2c8' => array(
'javelin-install',
'javelin-dom',
@ -1530,17 +1544,6 @@ return array(
'javelin-vector',
'javelin-stratcom',
),
'80ac3298' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-workflow',
'javelin-router',
'javelin-behavior-device',
'javelin-vector',
'phabricator-diff-inline',
),
'834a1173' => array(
'javelin-behavior',
'javelin-scrollbar',
@ -1667,14 +1670,6 @@ return array(
'javelin-mask',
'phabricator-drag-and-drop-file-upload',
),
'97f363fc' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-util',
'javelin-vector',
'differential-inline-comment-editor',
),
'988040b4' => array(
'javelin-install',
'javelin-dom',
@ -1703,6 +1698,14 @@ return array(
'9d9685d6' => array(
'phui-oi-list-view-css',
),
'9d9dbc38' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-util',
'javelin-vector',
'differential-inline-comment-editor',
),
'9ed8d2b6' => array(
'javelin-dom',
'javelin-util',
@ -1910,9 +1913,6 @@ return array(
'javelin-vector',
'phui-hovercard',
),
'bd1b3258' => array(
'javelin-dom',
),
'bdaf4d04' => array(
'javelin-behavior',
'javelin-dom',

18
webroot/rsrc/js/application/diff/DiffChangeset.js
View file

@ -322,7 +322,6 @@ JX.install('DiffChangeset', {
return JX.Stratcom.getData(this._node);
},
_onresponse: function(sequence, response) {
if (sequence != this._sequence) {
// If this isn't the most recent request, ignore it. This normally
@ -420,8 +419,25 @@ JX.install('DiffChangeset', {
}
return data.inline;
},
getInlineByID: function(id) {
// TODO: Currently, this will only find inlines which the user has
// already interacted with! Inlines are built lazily as events arrive.
// This can not yet find inlines which are passively present in the
// document.
for (var ii = 0; ii < this._inlines.length; ii++) {
var inline = this._inlines[ii];
if (inline.getID() == id) {
return inline;
}
}
return null;
}
},
statics: {

76
webroot/rsrc/js/application/diff/DiffChangesetList.js
View file

@ -35,6 +35,12 @@ JX.install('DiffChangesetList', {
'click',
['differential-inline-comment', 'differential-inline-done'],
ondone);
var ondelete = JX.bind(this, this._ifawake, this._onaction, 'delete');
JX.Stratcom.listen(
'click',
['differential-inline-comment', 'differential-inline-delete'],
ondelete);
},
properties: {
@ -71,6 +77,19 @@ JX.install('DiffChangesetList', {
return JX.DiffChangeset.getForNode(node);
},
getInlineByID: function(id) {
var inline = null;
for (var ii = 0; ii < this._changesets.length; ii++) {
inline = this._changesets[ii].getInlineByID(id);
if (inline) {
break;
}
}
return inline;
},
_ifawake: function(f) {
// This function takes another function and only calls it if the
// changeset list is awake, so we basically just ignore events when we
@ -351,6 +370,33 @@ JX.install('DiffChangesetList', {
e.prevent();
var inline = this._getInlineForEvent(e);
var is_ref = false;
// If we don't have a natural inline object, the user may have clicked
// an action (like "Delete") inside a preview element at the bottom of
// the page.
// If they did, try to find an associated normal inline to act on, and
// pretend they clicked that instead. This makes the overall state of
// the page more consistent.
// However, there may be no normal inline (for example, because it is
// on a version of the diff which is not visible). In this case, we
// act by reference.
if (inline === null) {
var data = e.getNodeData('differential-inline-comment');
inline = this.getInlineByID(data.id);
if (inline) {
is_ref = true;
} else {
switch (action) {
case 'delete':
this._deleteInlineByID(data.id);
return;
}
}
}
// TODO: For normal operations, highlight the inline range here.
@ -361,11 +407,41 @@ JX.install('DiffChangesetList', {
case 'done':
inline.toggleDone();
break;
case 'delete':
inline.delete(is_ref);
break;
}
},
redrawPreview: function() {
// TODO: This isn't the cleanest way to find the preview form, but
// rendering no longer has direct access to it.
var forms = JX.DOM.scry(document.body, 'form', 'transaction-append');
if (forms.length) {
JX.DOM.invoke(forms[0], 'shouldRefresh');
}
},
_deleteInlineByID: function(id) {
var uri = this.getInlineURI();
var data = {
op: 'refdelete',
id: id
};
var handler = JX.bind(this, this.redrawPreview);
new JX.Workflow(uri, data)
.setHandler(handler)
.start();
},
_getInlineForEvent: function(e) {
var node = e.getNode('differential-changeset');
if (!node) {
return null;
}
var changeset = this.getChangesetForNode(node);
var inline_row = e.getNode('inline-row');

213
webroot/rsrc/js/application/diff/DiffInline.js
View file

@ -47,6 +47,11 @@ JX.install('DiffInline', {
_length: null,
_displaySide: null,
_isNewFile: null,
_undoRow: null,
_isDeleted: false,
_isInvisible: false,
_isLoading: false,
setHidden: function(hidden) {
this._hidden = hidden;
@ -106,17 +111,47 @@ JX.install('DiffInline', {
},
edit: function() {
var handler = JX.bind(this, this._oneditresponse);
var uri = this._getInlineURI();
var data = this._newRequestData();
var handler = JX.bind(this, this._oneditresponse);
var data = this._newRequestData('edit');
// TODO: Set state to "loading".
this.setLoading(true);
new JX.Request(uri, handler)
.setData(data)
.send();
},
delete: function(is_ref) {
var uri = this._getInlineURI();
var handler = JX.bind(this, this._ondeleteresponse);
// NOTE: This may be a direct delete (the user clicked on the inline
// itself) or a "refdelete" (the user clicked somewhere else, like the
// preview, but the inline is present on the page).
// For a "refdelete", we prompt the user to confirm that they want to
// delete the comment, because they can not undo deletions from the
// preview. We could jump the user to the inline instead, but this would
// be somewhat disruptive and make deleting several comments more
// difficult.
var op;
if (is_ref) {
op = 'refdelete';
} else {
op = 'delete';
}
var data = this._newRequestData(op);
this.setLoading(true);
new JX.Workflow(uri, data)
.setHandler(handler)
.start();
},
getDisplaySide: function() {
return this._displaySide;
},
@ -133,9 +168,31 @@ JX.install('DiffInline', {
return this._isNewFile;
},
_newRequestData: function() {
getID: function() {
return this._id;
},
setDeleted: function(deleted) {
this._isDeleted = deleted;
this._redraw();
return this;
},
setInvisible: function(invisible) {
this._isInvisible = invisible;
this._redraw();
return this;
},
setLoading: function(loading) {
this._isLoading = loading;
this._redraw();
return this;
},
_newRequestData: function(operation) {
return {
op: 'edit',
op: operation,
id: this._id,
on_right: ((this.getDisplaySide() == 'right') ? 1 : 0),
renderer: this.getChangeset().getRenderer(),
@ -151,42 +208,89 @@ JX.install('DiffInline', {
this._drawEditRows(rows);
// TODO: Set the row state to "hidden".
this.setLoading(false);
this.setInvisible(true);
},
_ondeleteresponse: function() {
this._drawUndoRows();
this.setLoading(false);
this.setDeleted(true);
this._didUpdate();
},
_drawUndoRows: function() {
var templates = this.getChangeset().getUndoTemplates();
var template;
if (this.getDisplaySide() == 'right') {
template = templates.r;
} else {
template = templates.l;
}
template = JX.$H(template).getNode();
this._undoRow = this._drawRows(template, this._row, 'undo');
},
_drawEditRows: function(rows) {
var first_row = JX.DOM.scry(rows, 'tr')[0];
var row = first_row;
var cursor = this._row;
return this._drawRows(rows, null, 'edit');
},
_drawRows: function(rows, cursor, type) {
var first_row = JX.DOM.scry(rows, 'tr')[0];
var first_meta;
var row = first_row;
cursor = cursor || this._row.nextSibling;
var next_row;
while (row) {
// Grab this first, since it's going to change once we insert the row
// into the document.
next_row = row.nextSibling;
cursor.parentNode.insertBefore(row, cursor.nextSibling);
cursor = row;
var row_meta = {
node: row,
type: 'edit',
type: type,
listeners: []
};
row_meta.listeners.push(
JX.DOM.listen(
row,
['submit', 'didSyntheticSubmit'],
'inline-edit-form',
JX.bind(this, this._onsubmit, row_meta)));
if (!first_meta) {
first_meta = row_meta;
}
row_meta.listeners.push(
JX.DOM.listen(
row,
'click',
'inline-edit-cancel',
JX.bind(this, this._oncancel, row_meta)));
if (type == 'edit') {
row_meta.listeners.push(
JX.DOM.listen(
row,
['submit', 'didSyntheticSubmit'],
'inline-edit-form',
JX.bind(this, this._onsubmit, row_meta)));
row = row.nextSibling;
row_meta.listeners.push(
JX.DOM.listen(
row,
'click',
'inline-edit-cancel',
JX.bind(this, this._oncancel, row_meta)));
} else {
row_meta.listeners.push(
JX.DOM.listen(
row,
'click',
'differential-inline-comment-undo',
JX.bind(this, this._onundo, row_meta)));
}
row = next_row;
}
return first_row;
return first_meta;
},
_onsubmit: function(row, e) {
@ -194,11 +298,33 @@ JX.install('DiffInline', {
var handler = JX.bind(this, this._onsubmitresponse, row);
this.setLoading(true);
JX.Workflow.newFromForm(e.getTarget())
.setHandler(handler)
.start();
},
// TODO: Set state to "loading".
_onundo: function(row, e) {
e.kill();
this._removeRow(row);
var uri = this._getInlineURI();
var data = this._newRequestData('undelete');
var handler = JX.bind(this, this._onundelete);
this.setDeleted(false);
this.setLoading(true);
new JX.Request(uri, handler)
.setData(data)
.send();
},
_onundelete: function() {
this.setLoading(false);
this._didUpdate();
},
_oncancel: function(row, e) {
@ -206,18 +332,15 @@ JX.install('DiffInline', {
// TODO: Capture edited text and offer "undo".
JX.DOM.remove(row.node);
this._removeListeners(row.listeners);
this._removeRow(row);
// TODO: Restore state to "normal".
this.setInvisible(false);
},
_onsubmitresponse: function(row, response) {
this._removeRow(row);
JX.DOM.remove(row.node);
this._removeListeners(row.listeners);
// TODO: Restore state to "normal".
this.setInvisible(false);
this._onupdate(response);
},
@ -225,7 +348,7 @@ JX.install('DiffInline', {
_onupdate: function(response) {
var new_row;
if (response.markup) {
new_row = this._drawEditRows(JX.$H(response.markup).getNode());
new_row = this._drawEditRows(JX.$H(response.markup).getNode()).node;
}
// TODO: Save the old row so the action it's undo-able if it was a
@ -243,18 +366,22 @@ JX.install('DiffInline', {
_didUpdate: function() {
// After making changes to inline comments, refresh the transaction
// preview at the bottom of the page.
// TODO: This isn't the cleanest way to find the preview form, but
// rendering no longer has direct access to it.
var forms = JX.DOM.scry(document.body, 'form', 'transaction-append');
if (forms.length) {
JX.DOM.invoke(forms[0], 'shouldRefresh');
}
this.getChangeset().getChangesetList().redrawPreview();
},
_removeListeners: function(listeners) {
for (var ii = 0; ii < listeners.length; ii++) {
listeners[ii].remove();
_redraw: function() {
var is_invisible = (this._isInvisible || this._isDeleted);
var is_loading = (this._isLoading);
var row = this._row;
JX.DOM.alterClass(row, 'differential-inline-hidden', is_invisible);
JX.DOM.alterClass(row, 'differential-inline-loading', is_loading);
},
_removeRow: function(row) {
JX.DOM.remove(row.node);
for (var ii = 0; ii < row.listeners.length; ii++) {
row.listeners[ii].remove();
}
},

75
webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
View file

@ -331,46 +331,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
var handle_inline_action = function(node, op) {
var data = JX.Stratcom.getData(node);
// If you click an action in the preview at the bottom of the page, we
// find the corresponding node and simulate clicking that, if it's
// present on the page. This gives the editor a more consistent view
// of the document.
if (JX.Stratcom.hasSigil(node, 'differential-inline-comment-preview')) {
var nodes = JX.DOM.scry(
JX.DOM.getContentFrame(),
'div',
'differential-inline-comment');
var found = false;
var node_data;
for (var ii = 0; ii < nodes.length; ++ii) {
if (nodes[ii] == node) {
// Don't match the preview itself.
continue;
}
node_data = JX.Stratcom.getData(nodes[ii]);
if (node_data.id == data.id) {
node = nodes[ii];
data = node_data;
found = true;
break;
}
}
if (!found) {
switch (op) {
case 'delete':
new JX.DifferentialInlineCommentEditor(config.uri)
.deleteByID(data.id);
return;
}
}
if (op == 'delete') {
op = 'refdelete';
}
}
var original = data.original;
var reply_phid = null;
if (op == 'reply') {
@ -387,30 +347,25 @@ JX.behavior('differential-edit-inline-comments', function(config) {
'differential-changeset');
var view = JX.DiffChangeset.getForNode(changeset_root);
if (op == 'edit') {
// This is now handled by DiffChangesetList.
editor = true;
} else {
editor = new JX.DifferentialInlineCommentEditor(config.uri)
.setTemplates(view.getUndoTemplates())
.setOperation(op)
.setID(data.id)
.setChangesetID(data.changesetID)
.setLineNumber(data.number)
.setLength(data.length)
.setOnRight(data.on_right)
.setOriginalText(original)
.setRow(row)
.setTable(row.parentNode)
.setReplyToCommentPHID(reply_phid)
.setRenderer(view.getRenderer())
.start();
}
editor = new JX.DifferentialInlineCommentEditor(config.uri)
.setTemplates(view.getUndoTemplates())
.setOperation(op)
.setID(data.id)
.setChangesetID(data.changesetID)
.setLineNumber(data.number)
.setLength(data.length)
.setOnRight(data.on_right)
.setOriginalText(original)
.setRow(row)
.setTable(row.parentNode)
.setReplyToCommentPHID(reply_phid)
.setRenderer(view.getRenderer())
.start();
set_link_state(true);
};
for (var op in {'edit': 1, 'delete': 1, 'reply': 1}) {
for (var op in {'reply': 1}) {
JX.Stratcom.listen(
'click',
['differential-inline-comment', 'differential-inline-' + op],