From 1b5a276a02d664ff2c48929ccea895022dd61a49 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 May 2017 08:03:43 -0700 Subject: [PATCH] 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 | 22 +++--- .../view/DifferentialChangesetListView.php | 15 ++++ .../js/application/diff/DiffChangesetList.js | 71 ++++++++++++++++--- .../rsrc/js/application/diff/DiffInline.js | 24 ++++++- 4 files changed, 109 insertions(+), 23 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 7741821c83..c16ac91ac1 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' => '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', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index fea7a2da55..48b2ea5e12 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/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.'), ), )); diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index a83e21e0ae..01275578fe 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/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. diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 2a4c444a86..04f9a03db1 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/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();