Page MenuHomePhabricator
Files F15361071

D17890.diff
No OneTemporary
Actions

D17890.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' => '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 @@
'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 @@
'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 @@
'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 @@
'javelin-dom',
'javelin-typeahead-normalizer',
),
- 12575699 => array(
- 'javelin-install',
- ),
'1499a8cb' => array(
'javelin-behavior',
'javelin-stratcom',
@@ -1301,6 +1298,9 @@
'javelin-typeahead-source',
'javelin-util',
),
+ '50bc5b50' => array(
+ 'javelin-install',
+ ),
'519705ea' => array(
'javelin-install',
'javelin-dom',
@@ -1342,6 +1342,17 @@
'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 @@
'javelin-workflow',
'javelin-dom',
),
+ '64dfc791' => array(
+ 'javelin-dom',
+ ),
'680ea2c8' => array(
'javelin-install',
'javelin-dom',
@@ -1530,17 +1544,6 @@
'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 @@
'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 @@
'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 @@
'javelin-vector',
'phui-hovercard',
),
- 'bd1b3258' => array(
- 'javelin-dom',
- ),
'bdaf4d04' => array(
'javelin-behavior',
'javelin-dom',
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -322,7 +322,6 @@
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 @@
}
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: {
diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js
--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js
+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js
@@ -35,6 +35,12 @@
'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 @@
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 @@
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 @@
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');
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
@@ -47,6 +47,11 @@
_length: null,
_displaySide: null,
_isNewFile: null,
+ _undoRow: null,
+
+ _isDeleted: false,
+ _isInvisible: false,
+ _isLoading: false,
setHidden: function(hidden) {
this._hidden = hidden;
@@ -106,17 +111,47 @@
},
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 @@
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 @@
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) {
+ 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;
- var cursor = this._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)));
-
- row_meta.listeners.push(
- JX.DOM.listen(
- row,
- 'click',
- 'inline-edit-cancel',
- JX.bind(this, this._oncancel, row_meta)));
-
- row = row.nextSibling;
+ if (!first_meta) {
+ first_meta = 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_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 @@
var handler = JX.bind(this, this._onsubmitresponse, row);
+ this.setLoading(true);
+
JX.Workflow.newFromForm(e.getTarget())
.setHandler(handler)
.start();
+ },
+
+ _onundo: function(row, e) {
+ e.kill();
+
+ this._removeRow(row);
- // TODO: Set state to "loading".
+ 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 @@
// 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 @@
_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 @@
_didUpdate: function() {
// After making changes to inline comments, refresh the transaction
// preview at the bottom of the page.
+ this.getChangeset().getChangesetList().redrawPreview();
+ },
- // 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');
- }
+ _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);
},
- _removeListeners: function(listeners) {
- for (var ii = 0; ii < listeners.length; ii++) {
- listeners[ii].remove();
+ _removeRow: function(row) {
+ JX.DOM.remove(row.node);
+ for (var ii = 0; ii < row.listeners.length; ii++) {
+ row.listeners[ii].remove();
}
},
diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
--- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
+++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
@@ -331,46 +331,6 @@
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 @@
'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],

File Metadata

Mime Type
text/plain
Expires
Wed, Mar 12, 11:59 AM (1 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7585598
Default Alt Text
D17890.diff (21 KB)

Event Timeline

Phabricator · Built by Phacility