Add Differential keyboard shortcuts for "mark done" and "hide/show"

Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").

(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)

Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.

Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8130

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '8c5f913d',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '58712637',
-    'differential.pkg.js' => '0bfd141c',
+    'differential.pkg.js' => 'e486afd0',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -391,8 +391,8 @@ return array(
     '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' => '145c34e2',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'ca3b6387',
-    'rsrc/js/application/diff/DiffInline.js' => 'b5b1f167',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'd93e34c2',
+    'rsrc/js/application/diff/DiffInline.js' => 'bdf6b568',
     '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',
@@ -782,8 +782,8 @@ return array(
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
     'phabricator-diff-changeset' => '145c34e2',
-    'phabricator-diff-changeset-list' => 'ca3b6387',
-    'phabricator-diff-inline' => 'b5b1f167',
+    'phabricator-diff-changeset-list' => 'd93e34c2',
+    'phabricator-diff-inline' => 'bdf6b568',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1842,9 +1842,6 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
-    'b5b1f167' => array(
-      'javelin-dom',
-    ),
     'b5c256b8' => array(
       'javelin-install',
       'javelin-dom',
@@ -1893,6 +1890,9 @@ return array(
       'javelin-util',
       'javelin-request',
     ),
+    'bdf6b568' => array(
+      'javelin-dom',
+    ),
     'bea6e7f4' => array(
       'javelin-install',
       'javelin-dom',
@@ -1984,9 +1984,6 @@ return array(
       'phabricator-shaped-request',
       'conpherence-thread-manager',
     ),
-    'ca3b6387' => array(
-      'javelin-install',
-    ),
     'ca3f91eb' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2082,6 +2079,9 @@ return array(
       'javelin-util',
       'phabricator-shaped-request',
     ),
+    'd93e34c2' => array(
+      'javelin-install',
+    ),
     'de2e896f' => array(
       'javelin-behavior',
       'javelin-dom',

src/applications/differential/view/DifferentialChangesetListView.php

@@ -252,6 +252,21 @@ final class DifferentialChangesetListView extends AphrontView {
           pht('You must select a comment to reply to.'),
         'You must select a comment to edit.' =>
           pht('You must select a comment to edit.'),
+
+        '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.' =>
+          pht('You must select a comment to mark done.'),
+
+        'Hide or show inline comment.' =>
+          pht('Hide or show inline comment.'),
+        'You must select a comment to hide.' =>
+          pht('You must select a comment to hide.'),
+
+        'Jump to next inline comment, including hidden comments.' =>
+          pht('Jump to next inline comment, including hidden comments.'),
+        'Jump to previous inline comment, including hidden comments.' =>
+          pht('Jump to previous inline comment, including hidden comments.'),
       ),
     ));
 

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

@@ -106,14 +106,27 @@ JX.install('DiffChangesetList', {
       label = pht('Jump to previous inline comment.');
       this._installJumpKey('p', label, -1, 'comment');
 
+      label = pht('Jump to next inline comment, including hidden comments.');
+      this._installJumpKey('N', label, 1, 'comment', true);
+
+      label = pht(
+        'Jump to previous inline comment, including hidden comments.');
+      this._installJumpKey('P', label, -1, 'comment', true);
+
       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);
+      this._installKey('r', label, this._onkeyreply);
 
       label = pht('Edit selected inline comment.');
-      this._installKey('e', label, this._onedit);
+      this._installKey('e', label, this._onkeyedit);
+
+      label = pht('Mark or unmark selected inline comment as done.');
+      this._installKey('w', label, this._onkeydone);
+
+      label = pht('Hide or show inline comment.');
+      this._installKey('q', label, this._onkeyhide);
     },
 
     isAsleep: function() {
@@ -190,9 +203,9 @@ JX.install('DiffChangesetList', {
         .register();
     },
 
-    _installJumpKey: function(key, label, delta, filter) {
+    _installJumpKey: function(key, label, delta, filter, show_hidden) {
       filter = filter || null;
-      var handler = JX.bind(this, this._onjumpkey, delta, filter);
+      var handler = JX.bind(this, this._onjumpkey, delta, filter, show_hidden);
       return this._installKey(key, label, handler);
     },
 
@@ -201,7 +214,7 @@ JX.install('DiffChangesetList', {
       manager.scrollTo(toc);
     },
 
-    _onreply: function(manager) {
+    _onkeyreply: function() {
       var cursor = this._cursorItem;
 
       if (cursor) {
@@ -220,7 +233,7 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment to reply to.'));
     },
 
-    _onedit: function(manager) {
+    _onkeyedit: function() {
       var cursor = this._cursorItem;
 
       if (cursor) {
@@ -239,6 +252,44 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment to edit.'));
     },
 
+    _onkeydone: function() {
+      var cursor = this._cursorItem;
+
+      if (cursor) {
+        if (cursor.type == 'comment') {
+          var inline = cursor.target;
+          if (inline.canDone()) {
+            this.setFocus(null);
+
+            inline.toggleDone();
+            return;
+          }
+        }
+      }
+
+      var pht = this.getTranslations();
+      this._warnUser(pht('You must select a comment to mark done.'));
+    },
+
+    _onkeyhide: function() {
+      var cursor = this._cursorItem;
+
+      if (cursor) {
+        if (cursor.type == 'comment') {
+          var inline = cursor.target;
+          if (inline.canHide()) {
+            this.setFocus(null);
+
+            inline.setHidden(!inline.isHidden());
+            return;
+          }
+        }
+      }
+
+      var pht = this.getTranslations();
+      this._warnUser(pht('You must select a comment to hide.'));
+    },
+
     _warnUser: function(message) {
       new JX.Notification()
         .setContent(message)
@@ -247,7 +298,7 @@ JX.install('DiffChangesetList', {
         .show();
     },
 
-    _onjumpkey: function(delta, filter, manager) {
+    _onjumpkey: function(delta, filter, show_hidden, manager) {
       var state = this._getSelectionState();
 
       var cursor = state.cursor;
@@ -286,8 +337,10 @@ JX.install('DiffChangesetList', {
 
         // If the item is hidden, don't select it when iterating with jump
         // keys. It can still potentially be selected in other ways.
-        if (items[cursor].hidden) {
-          continue;
+        if (!show_hidden) {
+          if (items[cursor].hidden) {
+            continue;
+          }
         }
 
         // Otherwise, we've found a valid item to select.

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

@@ -130,6 +130,22 @@ JX.install('DiffInline', {
       return true;
     },
 
+    canDone: function() {
+      if (!JX.DOM.scry(this._row, 'input', 'differential-inline-done').length) {
+        return false;
+      }
+
+      return true;
+    },
+
+    canHide: function() {
+      if (!JX.DOM.scry(this._row, 'a', 'hide-inline').length) {
+        return false;
+      }
+
+      return true;
+    },
+
     _hasAction: function(action) {
       var nodes = JX.DOM.scry(this._row, 'a', 'differential-inline-' + action);
       return (nodes.length > 0);
@@ -171,7 +187,7 @@ JX.install('DiffInline', {
         .setHandler(JX.bag)
         .start();
 
-      JX.Stratcom.invoke('resize');
+      this._didUpdate(true);
     },
 
     isHidden: function() {
@@ -514,10 +530,12 @@ JX.install('DiffInline', {
       this._didUpdate();
     },
 
-    _didUpdate: function() {
+    _didUpdate: function(local_only) {
       // After making changes to inline comments, refresh the transaction
       // preview at the bottom of the page.
-      this.getChangeset().getChangesetList().redrawPreview();
+      if (!local_only) {
+        this.getChangeset().getChangesetList().redrawPreview();
+      }
 
       this.getChangeset().getChangesetList().redrawCursor();