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' => '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 @@ '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 @@ '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 @@ '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 @@ 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + '1afe9760' => array( + 'javelin-dom', + ), '1bd28176' => array( 'javelin-install', 'javelin-dom', @@ -1037,6 +1040,13 @@ '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 @@ '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 @@ 'javelin-vector', 'javelin-dom', ), + '57c491b5' => array( + 'javelin-install', + ), '58dea2fa' => array( 'javelin-install', 'javelin-util', @@ -1406,6 +1408,17 @@ '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 @@ 'javelin-dom', 'javelin-vector', ), - 'c2e9ff4c' => array( - 'javelin-dom', - ), 'c420b0b9' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -2036,13 +2046,6 @@ '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 @@ '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 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -244,15 +244,19 @@ 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 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -526,7 +526,37 @@ 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 @@ 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 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -136,8 +136,11 @@ 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 @@ manager.scrollTo(toc); }, - _onkeyreply: function() { + _onkeyreply: function(is_quote) { var cursor = this._cursorItem; if (cursor) { @@ -243,14 +246,77 @@ 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 @@ // 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 @@ 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 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -148,6 +148,10 @@ 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 @@ .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 @@ }, _oncreateresponse: function(response) { - try { var rows = JX.$H(response).getNode(); this._drawEditRows(rows); - } catch (e) { - JX.log(e); - } }, _ondeleteresponse: function() { @@ -471,7 +471,11 @@ '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 --- 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 @@ 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 @@ 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;