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

@@ -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/DiffChangeset.js' => '68758d99',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'fcbf80e9',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '57c491b5',
-    'rsrc/js/application/diff/DiffInline.js' => 'c2e9ff4c',
+    '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' => '68758d99',
-    'phabricator-diff-changeset-list' => 'fcbf80e9',
+    'phabricator-diff-changeset-list' => '57c491b5',
-    'phabricator-diff-inline' => 'c2e9ff4c',
+    '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',

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.' =>

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;
     },

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.');
+      label = pht('Reply to selected inline comment or change.');
-      this._installKey('r', label, this._onkeyreply);
+      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 "<th />" 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';
+    }
   }
 
 });

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;

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({
+      view.newInlineForRange(origin, target);
-        origin: origin,
-        target: target,
-        number: o,
-        length: len,
-        changesetID: changeset,
-        isNewFile: isNewFile(target),
-        displaySide: isOnRight(target) ? 'right' : 'left'
-      });
 
       selecting = false;
       origin = null;