From e4e91ebf6f15936a90ab7fff4e249b3fb6dd0058 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 May 2017 16:52:31 -0700 Subject: [PATCH] In Differential, allow "r" to create comments and "R" to quote Summary: Ref T11401. Fixes T5232. Ref T12616. Partly, this moves more code over to the new stuff. This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment. You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe). With the new click-to-select, you can click + "R" to reply-with-quote. Test Plan: Used "r" and "R" to reply to comments and code. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12616, T11401, T5232 Differential Revision: https://secure.phabricator.com/D17920 --- resources/celerity/map.php | 66 +++++++------- .../view/DifferentialChangesetListView.php | 12 ++- .../rsrc/js/application/diff/DiffChangeset.js | 36 +++++++- .../js/application/diff/DiffChangesetList.js | 91 +++++++++++++++++-- .../rsrc/js/application/diff/DiffInline.js | 18 ++-- .../behavior-edit-inline-comments.js | 15 +-- 6 files changed, 170 insertions(+), 68 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 575ce9a2b1..125568ab64 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '8c5f913d', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => 'ea471cb0', - 'differential.pkg.js' => '7f24021f', + 'differential.pkg.js' => 'ae6f5198', 'diffusion.pkg.css' => 'b93d9b8c', 'diffusion.pkg.js' => '84c8f8fd', 'favicon.ico' => '30672e08', @@ -390,13 +390,13 @@ 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' => '34e513e2', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'fcbf80e9', - 'rsrc/js/application/diff/DiffInline.js' => 'c2e9ff4c', + 'rsrc/js/application/diff/DiffChangeset.js' => '68758d99', + 'rsrc/js/application/diff/DiffChangesetList.js' => '57c491b5', + 'rsrc/js/application/diff/DiffInline.js' => '1afe9760', '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', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'd59300f5', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '1c6bc8cf', 'rsrc/js/application/differential/behavior-populate.js' => '5e41c819', 'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb', 'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d', @@ -619,7 +619,7 @@ return array( 'javelin-behavior-device' => 'bb1dd507', 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', - 'javelin-behavior-differential-edit-inline-comments' => 'd59300f5', + 'javelin-behavior-differential-edit-inline-comments' => '1c6bc8cf', 'javelin-behavior-differential-feedback-preview' => 'b064af76', 'javelin-behavior-differential-populate' => '5e41c819', 'javelin-behavior-differential-toggle-files' => 'ca3f91eb', @@ -779,9 +779,9 @@ return array( 'phabricator-darklog' => 'c8e1ffe3', 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', - 'phabricator-diff-changeset' => '34e513e2', - 'phabricator-diff-changeset-list' => 'fcbf80e9', - 'phabricator-diff-inline' => 'c2e9ff4c', + 'phabricator-diff-changeset' => '68758d99', + 'phabricator-diff-changeset-list' => '57c491b5', + 'phabricator-diff-inline' => '1afe9760', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', 'phabricator-fatal-config-template-css' => '8f18fa41', @@ -1030,6 +1030,9 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + '1afe9760' => array( + 'javelin-dom', + ), '1bd28176' => array( 'javelin-install', 'javelin-dom', @@ -1037,6 +1040,13 @@ return array( 'javelin-request', 'javelin-uri', ), + '1c6bc8cf' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + ), '1def2711' => array( 'javelin-install', 'javelin-dom', @@ -1129,17 +1139,6 @@ return array( 'javelin-dom', 'javelin-workflow', ), - '34e513e2' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1332,6 +1331,9 @@ return array( 'javelin-vector', 'javelin-dom', ), + '57c491b5' => array( + 'javelin-install', + ), '58dea2fa' => array( 'javelin-install', 'javelin-util', @@ -1406,6 +1408,17 @@ 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', ), @@ -1910,9 +1923,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - 'c2e9ff4c' => array( - 'javelin-dom', - ), 'c420b0b9' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -2036,13 +2046,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'd59300f5' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - ), 'd5a2d665' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2221,9 +2224,6 @@ return array( 'javelin-dom', 'phortune-credit-card-form', ), - 'fcbf80e9' => array( - 'javelin-install', - ), 'fe287620' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index c69eef008f..1e40595bd0 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -244,15 +244,19 @@ final class DifferentialChangesetListView extends AphrontView { pht('Jump to previous inline comment.'), 'Jump to the table of contents.' => pht('Jump to the table of contents.'), - 'Reply to selected inline comment.' => - pht('Reply to selected inline comment.'), + 'Edit selected inline comment.' => pht('Edit selected inline comment.'), - 'You must select a comment to reply to.' => - pht('You must select a comment to reply to.'), 'You must select a comment to edit.' => pht('You must select a comment to edit.'), + 'Reply to selected inline comment or change.' => + pht('Reply to selected inline comment or change.'), + 'You must select a comment or change to reply to.' => + pht('You must select a comment or change to reply to.'), + 'Reply and quote selected inline comment.' => + pht('Reply and quote selected inline comment.'), + 'Mark or unmark selected inline comment as done.' => pht('Mark or unmark selected inline comment as done.'), 'You must select a comment to mark done.' => diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index 8176e4f12b..f6d1200c67 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -526,7 +526,37 @@ JX.install('DiffChangeset', { return data.inline; }, - newInlineForRange: function(data) { + newInlineForRange: function(origin, target) { + var list = this.getChangesetList(); + + var src = list.getLineNumberFromHeader(origin); + var dst = list.getLineNumberFromHeader(target); + + var changeset_id = null; + var side = list.getDisplaySideFromHeader(origin); + if (side == 'right') { + changeset_id = this.getRightChangesetID(); + } else { + changeset_id = this.getLeftChangesetID(); + } + + var is_new = false; + if (side == 'right') { + is_new = true; + } else if (this.getRightChangesetID() != this.getLeftChangesetID()) { + is_new = true; + } + + var data = { + origin: origin, + target: target, + number: src, + length: dst - src, + changesetID: changeset_id, + displaySide: side, + isNewFile: is_new + }; + var inline = new JX.DiffInline() .setChangeset(this) .bindToRange(data); @@ -538,14 +568,14 @@ JX.install('DiffChangeset', { return inline; }, - newInlineReply: function(original) { + newInlineReply: function(original, text) { var inline = new JX.DiffInline() .setChangeset(this) .bindToReply(original); this._inlines.push(inline); - inline.create(); + inline.create(text); return inline; }, diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index d9f1e35991..6067217419 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -136,8 +136,11 @@ JX.install('DiffChangesetList', { label = pht('Jump to the table of contents.'); this._installKey('t', label, this._ontoc); - label = pht('Reply to selected inline comment.'); - this._installKey('r', label, this._onkeyreply); + label = pht('Reply to selected inline comment or change.'); + this._installKey('r', label, JX.bind(this, this._onkeyreply, false)); + + label = pht('Reply and quote selected inline comment.'); + this._installKey('R', label, JX.bind(this, this._onkeyreply, true)); label = pht('Edit selected inline comment.'); this._installKey('e', label, this._onkeyedit); @@ -234,7 +237,7 @@ JX.install('DiffChangesetList', { manager.scrollTo(toc); }, - _onkeyreply: function() { + _onkeyreply: function(is_quote) { var cursor = this._cursorItem; if (cursor) { @@ -243,14 +246,77 @@ JX.install('DiffChangesetList', { if (inline.canReply()) { this.setFocus(null); - inline.reply(); + var text; + if (is_quote) { + text = inline.getRawText(); + text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n'; + } else { + text = ''; + } + + inline.reply(text); return; } } + + // If the keyboard cursor is selecting a range of lines, we may have + // a mixture of old and new changes on the selected rows. It is not + // entirely unambiguous what the user means when they say they want + // to reply to this, but we use this logic: reply on the new file if + // there are any new lines. Otherwise (if there are only removed + // lines) reply on the old file. + + if (cursor.type == 'change') { + var origin = cursor.nodes.begin; + var target = cursor.nodes.end; + + // The "origin" and "target" are entire rows, but we need to find + // a range of "" nodes to actually create an inline, so go + // fishing. + + var old_list = []; + var new_list = []; + + var row = origin; + while (row) { + var header = row.firstChild; + while (header) { + if (JX.DOM.isType(header, 'th')) { + if (header.className.indexOf('old') !== -1) { + old_list.push(header); + } else if (header.className.indexOf('new') !== -1) { + new_list.push(header); + } + } + header = header.nextSibling; + } + + if (row == target) { + break; + } + + row = row.nextSibling; + } + + var use_list; + if (new_list.length) { + use_list = new_list; + } else { + use_list = old_list; + } + + var src = use_list[0]; + var dst = use_list[use_list.length - 1]; + + cursor.changeset.newInlineForRange(src, dst); + + this.setFocus(null); + return; + } } var pht = this.getTranslations(); - this._warnUser(pht('You must select a comment to reply to.')); + this._warnUser(pht('You must select a comment or change to reply to.')); }, _onkeyedit: function() { @@ -804,7 +870,7 @@ JX.install('DiffChangesetList', { // Clear the mouse hover reticle after a substantive edit: we don't get // a "mouseout" event if the row vanished because of row being removed // after an edit. - this.reseHover(); + this.resetHover(); }, setFocus: function(node, extended_node) { @@ -978,8 +1044,19 @@ JX.install('DiffChangesetList', { var inline_row = e.getNode('inline-row'); return changeset.getInlineForRow(inline_row); - } + }, + getLineNumberFromHeader: function(th) { + try { + return parseInt(th.id.match(/^C\d+[ON]L(\d+)$/)[1], 10); + } catch (x) { + return null; + } + }, + + getDisplaySideFromHeader: function(th) { + return (th.parentNode.firstChild != th) ? 'right' : 'left'; + } } }); diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 38c2d7c82b..edd31942f9 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -148,6 +148,10 @@ JX.install('DiffInline', { return true; }, + getRawText: function() { + return this._originalText; + }, + _hasAction: function(action) { var nodes = JX.DOM.scry(this._row, 'a', 'differential-inline-' + action); return (nodes.length > 0); @@ -247,9 +251,9 @@ JX.install('DiffInline', { .send(); }, - reply: function() { + reply: function(text) { var changeset = this.getChangeset(); - return changeset.newInlineReply(this); + return changeset.newInlineReply(this, text); }, edit: function(text) { @@ -365,13 +369,9 @@ JX.install('DiffInline', { }, _oncreateresponse: function(response) { - try { var rows = JX.$H(response).getNode(); this._drawEditRows(rows); - } catch (e) { - JX.log(e); - } }, _ondeleteresponse: function() { @@ -471,7 +471,11 @@ JX.install('DiffInline', { 'textarea', 'differential-inline-comment-edit-textarea'); if (textareas.length) { - textareas[0].focus(); + var area = textareas[0]; + area.focus(); + + var length = area.value.length; + JX.TextAreaUtils.setSelectionRange(area, length, length); } row = next_row; diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 4b7b59decb..08c09d6626 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -72,11 +72,6 @@ JX.behavior('differential-edit-inline-comments', function(config) { return node.parentNode.firstChild != node; } - function isNewFile(node) { - var data = JX.Stratcom.getData(root); - return isOnRight(node) || (data.left != data.right); - } - function getRowNumber(th_node) { try { return parseInt(th_node.id.match(/^C\d+[ON]L(\d+)$/)[1], 10); @@ -192,15 +187,7 @@ JX.behavior('differential-edit-inline-comments', function(config) { var view = JX.DiffChangeset.getForNode(root); - view.newInlineForRange({ - origin: origin, - target: target, - number: o, - length: len, - changesetID: changeset, - isNewFile: isNewFile(target), - displaySide: isOnRight(target) ? 'right' : 'left' - }); + view.newInlineForRange(origin, target); selecting = false; origin = null;