Page MenuHomePhabricator
Files F15415558

D17938.id43142.diff
No OneTemporary
Actions

D17938.id43142.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' => '0f87a6eb',
'darkconsole.pkg.js' => '1f9a31bc',
'differential.pkg.css' => 'ea471cb0',
- 'differential.pkg.js' => '9ff5b8d2',
+ 'differential.pkg.js' => '31e1b646',
'diffusion.pkg.css' => 'b93d9b8c',
'diffusion.pkg.js' => '84c8f8fd',
'favicon.ico' => '30672e08',
@@ -390,9 +390,9 @@
'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' => 'dc969d3e',
+ 'rsrc/js/application/diff/DiffChangeset.js' => 'ec32b333',
'rsrc/js/application/diff/DiffChangesetList.js' => '796922e0',
- 'rsrc/js/application/diff/DiffInline.js' => '1afe9760',
+ 'rsrc/js/application/diff/DiffInline.js' => '3337c065',
'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
@@ -777,9 +777,9 @@
'phabricator-darklog' => 'c8e1ffe3',
'phabricator-darkmessage' => 'c48cccdd',
'phabricator-dashboard-css' => 'fe5b1869',
- 'phabricator-diff-changeset' => 'dc969d3e',
+ 'phabricator-diff-changeset' => 'ec32b333',
'phabricator-diff-changeset-list' => '796922e0',
- 'phabricator-diff-inline' => '1afe9760',
+ 'phabricator-diff-inline' => '3337c065',
'phabricator-drag-and-drop-file-upload' => '58dea2fa',
'phabricator-draggable-list' => 'bea6e7f4',
'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1028,9 +1028,6 @@
'javelin-util',
'phabricator-keyboard-shortcut-manager',
),
- '1afe9760' => array(
- 'javelin-dom',
- ),
'1bd28176' => array(
'javelin-install',
'javelin-dom',
@@ -1130,6 +1127,9 @@
'javelin-dom',
'javelin-workflow',
),
+ '3337c065' => array(
+ 'javelin-dom',
+ ),
'3ab51e2c' => array(
'javelin-behavior',
'javelin-behavior-device',
@@ -2055,17 +2055,6 @@
'javelin-util',
'phabricator-shaped-request',
),
- 'dc969d3e' => array(
- 'javelin-dom',
- 'javelin-util',
- 'javelin-stratcom',
- 'javelin-install',
- 'javelin-workflow',
- 'javelin-router',
- 'javelin-behavior-device',
- 'javelin-vector',
- 'phabricator-diff-inline',
- ),
'de2e896f' => array(
'javelin-behavior',
'javelin-dom',
@@ -2142,6 +2131,17 @@
'javelin-dom',
'phabricator-draggable-list',
),
+ 'ec32b333' => array(
+ 'javelin-dom',
+ 'javelin-util',
+ 'javelin-stratcom',
+ 'javelin-install',
+ 'javelin-workflow',
+ 'javelin-router',
+ 'javelin-behavior-device',
+ 'javelin-vector',
+ 'phabricator-diff-inline',
+ ),
'eded9ee8' => array(
'javelin-behavior',
'javelin-typeahead-ondemand-source',
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
@@ -581,8 +581,16 @@
},
getInlineByID: function(id) {
+ return this._queryInline('id', id);
+ },
+
+ getInlineByPHID: function(phid) {
+ return this._queryInline('phid', phid);
+ },
+
+ _queryInline: function(field, value) {
// First, look for the inline in the objects we've already built.
- var inline = this._findInlineByID(id);
+ var inline = this._findInline(field, value);
if (inline) {
return inline;
}
@@ -590,13 +598,24 @@
// If we haven't found a matching inline yet, rebuild all the inlines
// present in the document, then look again.
this._rebuildAllInlines();
- return this._findInlineByID(id);
+ return this._findInline(field, value);
},
- _findInlineByID: function(id) {
+ _findInline: function(field, value) {
for (var ii = 0; ii < this._inlines.length; ii++) {
var inline = this._inlines[ii];
- if (inline.getID() == id) {
+
+ var target;
+ switch (field) {
+ case 'id':
+ target = inline.getID();
+ break;
+ case 'phid':
+ target = inline.getPHID();
+ break;
+ }
+
+ if (target == value) {
return inline;
}
}
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
@@ -63,6 +63,8 @@
(this.getDisplaySide() == 'right') ||
(data.left != data.right);
+ this._replyToCommentPHID = data.replyToCommentPHID;
+
this.setInvisible(false);
return this;
@@ -100,11 +102,45 @@
this._replyToCommentPHID = inline._phid;
- // TODO: This should insert correctly into the thread, not just at the
- // bottom.
+ var changeset = this.getChangeset();
+
+ // We're going to figure out where in the document to position the new
+ // inline. Normally, it goes after any existing inline rows (so if
+ // several inlines reply to the same line, they appear in chronological
+ // order).
+
+ // However: if inlines are threaded, we want to put the new inline in
+ // the right place in the thread. This might be somewhere in the middle,
+ // so we need to do a bit more work to figure it out.
+
+ // To find the right place in the thread, we're going to look for any
+ // inline which is at or above the level of the comment we're replying
+ // to. This means we've reached a new fork of the thread, and should
+ // put our new inline before the comment we found.
+ var ancestor_map = {};
+ var ancestor = inline;
+ var reply_phid;
+ while (ancestor) {
+ reply_phid = ancestor.getReplyToCommentPHID();
+ if (!reply_phid) {
+ break;
+ }
+ ancestor_map[reply_phid] = true;
+ ancestor = changeset.getInlineByPHID(reply_phid);
+ }
+
var parent_row = inline._row;
var target_row = parent_row.nextSibling;
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
+ var target = changeset.getInlineForRow(target_row);
+ reply_phid = target.getReplyToCommentPHID();
+
+ // If we found an inline which is replying directly to some ancestor
+ // of this new comment, this is where the new rows go.
+ if (ancestor_map.hasOwnProperty(reply_phid)) {
+ break;
+ }
+
target_row = target_row.nextSibling;
}
@@ -318,6 +354,10 @@
return this._id;
},
+ getPHID: function() {
+ return this._phid;
+ },
+
getChangesetID: function() {
return this._changesetID;
},

File Metadata

Mime Type
text/plain
Expires
Fri, Mar 21, 6:39 AM (1 w, 18 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7715106
Default Alt Text
D17938.id43142.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility