Restore the Differential "edit" and "reply" keyboard shortcuts

Summary: Ref T12616. This makes "edit" and "reply" work again.

Test Plan:
Used "e" and "r" to edit and reply.

Also used them in bogus ways and got useful UI feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17895

Author: epriestley

resources/celerity/map.php

@@ -13,7 +13,7 @@
     'core.pkg.js' => '2ff7879f',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '58712637',
-    'differential.pkg.js' => 'e6129b80',
+    'differential.pkg.js' => '5ee318c2',
     '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' => '3d4b3c5e',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'e2c315d9',
-    'rsrc/js/application/diff/DiffInline.js' => '98c12b2f',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'f7100923',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'f10fd7a3',
+    'rsrc/js/application/diff/DiffInline.js' => '00db3c3a',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
     'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
@@ -781,9 +781,9 @@
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => '3d4b3c5e',
-    'phabricator-diff-changeset-list' => 'e2c315d9',
-    'phabricator-diff-inline' => '98c12b2f',
+    'phabricator-diff-changeset' => 'f7100923',
+    'phabricator-diff-changeset-list' => 'f10fd7a3',
+    'phabricator-diff-inline' => '00db3c3a',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -918,6 +918,9 @@
     'unhandled-exception-css' => '4c96257a',
   ),
   'requires' => array(
+    '00db3c3a' => array(
+      'javelin-dom',
+    ),
     '013ffff9' => array(
       'javelin-install',
       'javelin-util',
@@ -1158,17 +1161,6 @@
       'javelin-util',
       'javelin-uri',
     ),
-    '3d4b3c5e' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     '3dbf94d5' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1666,9 +1658,6 @@
       'javelin-dom',
       'javelin-reactor-dom',
     ),
-    '98c12b2f' => array(
-      'javelin-dom',
-    ),
     '9a6dd75c' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2108,9 +2097,6 @@
       'javelin-stratcom',
       'javelin-dom',
     ),
-    'e2c315d9' => array(
-      'javelin-install',
-    ),
     'e2e0a072' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2181,6 +2167,9 @@
       'javelin-workflow',
       'javelin-json',
     ),
+    'f10fd7a3' => array(
+      'javelin-install',
+    ),
     'f12cbc9f' => array(
       'phui-oi-list-view-css',
     ),
@@ -2199,6 +2188,17 @@
       'phuix-icon-view',
       'phabricator-prefab',
     ),
+    'f7100923' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     'f7fc67ec' => array(
       'javelin-install',
       'javelin-typeahead',

src/applications/differential/view/DifferentialChangesetListView.php

@@ -244,6 +244,14 @@ public function render() {
           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.'),
       ),
     ));
 

webroot/rsrc/js/application/diff/DiffChangeset.js

@@ -367,10 +367,19 @@ JX.install('DiffChangeset', {
 
         if (block.type == 'comment') {
           for (var jj = 0; jj < block.items.length; jj++) {
+            var inline = this.getInlineForRow(block.items[jj]);
+
+            // If this inline has been collapsed, don't select it with the
+            // keyboard cursor.
+            if (inline.isHidden()) {
+              continue;
+            }
+
             items.push({
               type: block.type,
               changeset: this,
               target: block.items[jj],
+              inline: inline,
               nodes: {
                 begin: block.items[jj],
                 end: block.items[jj]

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -98,6 +98,12 @@ 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._onreply);
+
+      label = pht('Edit selected inline comment.');
+      this._installKey('e', label, this._onedit);
     },
 
     isAsleep: function() {
@@ -185,6 +191,52 @@ JX.install('DiffChangesetList', {
       manager.scrollTo(toc);
     },
 
+    _onreply: function(manager) {
+      var cursor = this._cursorItem;
+
+      if (cursor) {
+        if (cursor.type == 'comment') {
+          var inline = cursor.inline;
+          if (inline.canReply()) {
+            manager.focusOn(null);
+
+            inline.reply();
+            return;
+          }
+        }
+      }
+
+      var pht = this.getTranslations();
+      this._warnUser(pht('You must select a comment to reply to.'));
+    },
+
+    _onedit: function(manager) {
+      var cursor = this._cursorItem;
+
+      if (cursor) {
+        if (cursor.type == 'comment') {
+          var inline = cursor.inline;
+          if (inline.canEdit()) {
+            manager.focusOn(null);
+
+            inline.edit();
+            return;
+          }
+        }
+      }
+
+      var pht = this.getTranslations();
+      this._warnUser(pht('You must select a comment to edit.'));
+    },
+
+    _warnUser: function(message) {
+      new JX.Notification()
+        .setContent(message)
+        .alterClassName('jx-notification-alert', true)
+        .setDuration(1000)
+        .show();
+    },
+
     _onjumpkey: function(delta, filter, manager) {
       var state = this._getSelectionState();
 
@@ -534,8 +586,7 @@ JX.install('DiffChangesetList', {
     },
 
     _onaction: function(action, e) {
-      // TODO: This can become a kill once things fully switch over..
-      e.prevent();
+      e.kill();
 
       var inline = this._getInlineForEvent(e);
       var is_ref = false;

webroot/rsrc/js/application/diff/DiffInline.js

@@ -114,6 +114,27 @@ JX.install('DiffInline', {
       return this;
     },
 
+    canReply: function() {
+      if (!this._hasAction('reply')) {
+        return false;
+      }
+
+      return true;
+    },
+
+    canEdit: function() {
+      if (!this._hasAction('edit')) {
+        return false;
+      }
+
+      return true;
+    },
+
+    _hasAction: function(action) {
+      var nodes = JX.DOM.scry(this._row, 'a', 'differential-inline-' + action);
+      return (nodes.length > 0);
+    },
+
     _newRow: function() {
       var attributes = {
         sigil: 'inline-row'
@@ -151,6 +172,10 @@ JX.install('DiffInline', {
         .start();
     },
 
+    isHidden: function() {
+      return this._hidden;
+    },
+
     toggleDone: function() {
       var uri = this._getInlineURI();
       var data = {