From 80c329c94294445f2fbf6ac8f18ca4810fa2168d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 May 2017 11:59:00 -0700 Subject: [PATCH] When replying to a threaded inline, put the new inline in the right place in the thread Summary: Fixes T10563. If you have a thread like this: ``` > A > B > C ``` ...and you reply to "B", we should put the new inline below "B". We currently do when you reload the page, but the initial edit goes at the bottom always (below "C"). Test Plan: {F4963015} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10563 Differential Revision: https://secure.phabricator.com/D17938 --- resources/celerity/map.php | 38 ++++++++-------- .../rsrc/js/application/diff/DiffChangeset.js | 27 ++++++++++-- .../rsrc/js/application/diff/DiffInline.js | 44 ++++++++++++++++++- 3 files changed, 84 insertions(+), 25 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4e06cdc7f2..10ff2b3e47 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '0f87a6eb', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => 'ea471cb0', - 'differential.pkg.js' => '4a466790', + 'differential.pkg.js' => '31e1b646', 'diffusion.pkg.css' => 'b93d9b8c', 'diffusion.pkg.js' => '84c8f8fd', 'favicon.ico' => '30672e08', @@ -390,9 +390,9 @@ 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' => '68758d99', + '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 @@ return array( 'phabricator-darklog' => 'c8e1ffe3', 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', - 'phabricator-diff-changeset' => '68758d99', + '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 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), - '1afe9760' => array( - 'javelin-dom', - ), '1bd28176' => array( 'javelin-install', 'javelin-dom', @@ -1130,6 +1127,9 @@ return array( 'javelin-dom', 'javelin-workflow', ), + '3337c065' => array( + 'javelin-dom', + ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1403,17 +1403,6 @@ return array( 'javelin-dom', 'phabricator-notification', ), - '68758d99' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - ), '6882e80a' => array( 'javelin-dom', ), @@ -2142,6 +2131,17 @@ return array( '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 index f591eebb67..aca035ea0c 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -581,8 +581,16 @@ JX.install('DiffChangeset', { }, 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 @@ JX.install('DiffChangeset', { // 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 index edd31942f9..b46bc00b4f 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -63,6 +63,8 @@ JX.install('DiffInline', { (this.getDisplaySide() == 'right') || (data.left != data.right); + this._replyToCommentPHID = data.replyToCommentPHID; + this.setInvisible(false); return this; @@ -100,11 +102,45 @@ JX.install('DiffInline', { 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 @@ JX.install('DiffInline', { return this._id; }, + getPHID: function() { + return this._phid; + }, + getChangesetID: function() { return this._changesetID; },