From 4757b358739c87c7decfb49d72fef1eb9e8fb6b7 Mon Sep 17 00:00:00 2001 From: Dhruv Srivastava Date: Fri, 12 Jun 2020 15:41:42 +0200 Subject: [PATCH] Add keyboard shortcuts for diffing against base and latest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add v + s/↓ to diff base against "right". Add v + w/↑ to diff "left" against "latest". Add v + a/← to diff base against "left" Add v + d/→ to diff "right" against latest The user is viewing diff of "left" against "right". "latest" is the most recent patchset created by the user. Change-Id: I86d61934e9132ac53f39c9ae88c49c46195b65c0 --- .../keyboard-shortcut-behavior.js | 141 ++++++++++++++---- .../change/gr-change-view/gr-change-view.js | 83 +++++++++++ .../gr-change-view/gr-change-view_test.js | 75 ++++++++++ .../gr-key-binding-display.js | 5 +- .../diff/gr-diff-view/gr-diff-view.js | 88 +++++++++++ .../diff/gr-diff-view/gr-diff-view_test.html | 75 ++++++++++ polygerrit-ui/app/elements/gr-app-element.js | 14 +- 7 files changed, 452 insertions(+), 29 deletions(-) diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js index 49f27bc39d..92ecb8d154 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js @@ -101,11 +101,14 @@ import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js'; const DOC_ONLY = 'DOC_ONLY'; const GO_KEY = 'GO_KEY'; +const V_KEY = 'V_KEY'; // The maximum age of a keydown event to be used in a jump navigation. This // is only for cases when the keyup event is lost. const GO_KEY_TIMEOUT_MS = 1000; +const V_KEY_TIMEOUT_MS = 1000; + const ShortcutSection = { ACTIONS: 'Actions', DIFFS: 'Diffs', @@ -141,6 +144,11 @@ const Shortcut = { TOGGLE_DIFF_MODE: 'TOGGLE_DIFF_MODE', REFRESH_CHANGE: 'REFRESH_CHANGE', EDIT_TOPIC: 'EDIT_TOPIC', + DIFF_AGAINST_BASE: 'DIFF_AGAINST_BASE', + DIFF_AGAINST_LATEST: 'DIFF_AGAINST_LATEST', + DIFF_BASE_AGAINST_LEFT: 'DIFF_BASE_AGAINST_LEFT', + DIFF_RIGHT_AGAINST_LATEST: 'DIFF_RIGHT_AGAINST_LATEST', + DIFF_BASE_AGAINST_LATEST: 'DIFF_BASE_AGAINST_LATEST', NEXT_LINE: 'NEXT_LINE', PREV_LINE: 'PREV_LINE', @@ -233,9 +241,29 @@ _describe(Shortcut.TOGGLE_CHANGE_STAR, ShortcutSection.ACTIONS, 'Star/unstar change'); _describe(Shortcut.EDIT_TOPIC, ShortcutSection.ACTIONS, 'Add a change topic'); +_describe(Shortcut.DIFF_AGAINST_BASE, ShortcutSection.ACTIONS, + 'Diff against base'); +_describe(Shortcut.DIFF_AGAINST_LATEST, ShortcutSection.ACTIONS, + 'Diff against latest patchset'); +_describe(Shortcut.DIFF_BASE_AGAINST_LEFT, ShortcutSection.ACTIONS, + 'Diff base against left'); +_describe(Shortcut.DIFF_RIGHT_AGAINST_LATEST, ShortcutSection.ACTIONS, + 'Diff right against latest'); +_describe(Shortcut.DIFF_BASE_AGAINST_LATEST, ShortcutSection.ACTIONS, + 'Diff base against latest'); _describe(Shortcut.NEXT_LINE, ShortcutSection.DIFFS, 'Go to next line'); _describe(Shortcut.PREV_LINE, ShortcutSection.DIFFS, 'Go to previous line'); +_describe(Shortcut.DIFF_AGAINST_BASE, ShortcutSection.DIFFS, + 'Diff against base'); +_describe(Shortcut.DIFF_AGAINST_LATEST, ShortcutSection.DIFFS, + 'Diff against latest patchset'); +_describe(Shortcut.DIFF_BASE_AGAINST_LEFT, ShortcutSection.DIFFS, + 'Diff base against left'); +_describe(Shortcut.DIFF_RIGHT_AGAINST_LATEST, ShortcutSection.DIFFS, + 'Diff right against latest'); +_describe(Shortcut.DIFF_BASE_AGAINST_LATEST, ShortcutSection.DIFFS, + 'Diff base against latest'); _describe(Shortcut.VISIBLE_LINE, ShortcutSection.DIFFS, 'Move cursor to currently visible code'); _describe(Shortcut.NEXT_CHUNK, ShortcutSection.DIFFS, @@ -435,39 +463,52 @@ class ShortcutManager { const bindings = this.bindings.get(shortcut); if (!bindings) { return null; } if (bindings[0] === GO_KEY) { - return [['g'].concat(bindings.slice(1))]; + return bindings.slice(1).map( + binding => this._describeKey(binding) + ) + .map(binding => ['g'].concat(binding)); + } + if (bindings[0] === V_KEY) { + return bindings.slice(1).map( + binding => this._describeKey(binding) + ) + .map(binding => ['v'].concat(binding)); } return bindings .filter(binding => binding !== DOC_ONLY) .map(binding => this.describeBinding(binding)); } + _describeKey(key) { + switch (key) { + case 'shift': + return 'Shift'; + case 'meta': + return 'Meta'; + case 'ctrl': + return 'Ctrl'; + case 'enter': + return 'Enter'; + case 'up': + return '↑'; + case 'down': + return '↓'; + case 'left': + return '←'; + case 'right': + return '→'; + default: + return key; + } + } + describeBinding(binding) { if (binding.length === 1) { return [binding]; } - return binding.split(':')[0].split('+').map(part => { - switch (part) { - case 'shift': - return 'Shift'; - case 'meta': - return 'Meta'; - case 'ctrl': - return 'Ctrl'; - case 'enter': - return 'Enter'; - case 'up': - return '↑'; - case 'down': - return '↓'; - case 'left': - return '←'; - case 'right': - return '→'; - default: - return part; - } - }); + return binding.split(':')[0].split('+').map(part => + this._describeKey(part) + ); } notifyListeners() { @@ -489,6 +530,8 @@ export const KeyboardShortcutBehavior = [ // eslint-disable-next-line object-shorthand GO_KEY: GO_KEY, // eslint-disable-next-line object-shorthand + V_KEY: V_KEY, + // eslint-disable-next-line object-shorthand Shortcut: Shortcut, // eslint-disable-next-line object-shorthand ShortcutSection: ShortcutSection, @@ -498,17 +541,20 @@ export const KeyboardShortcutBehavior = [ type: Number, value: null, }, - _shortcut_go_table: { type: Array, value() { return new Map(); }, }, + _shortcut_v_table: { + type: Array, + value() { return new Map(); }, + }, }, modifierPressed(e) { e = getKeyboardEvent(e); return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey || - !!this._inGoKeyMode(); + !!this._inGoKeyMode() || !!this._inVKeyMode(); }, isModifierPressed(e, modifier) { @@ -566,7 +612,14 @@ export const KeyboardShortcutBehavior = [ return; } if (bindings[0] === GO_KEY) { - this._shortcut_go_table.set(bindings[1], handler); + bindings.slice(1).forEach(binding => + this._shortcut_go_table.set(binding, handler)); + } else if (bindings[0] === V_KEY) { + // for each binding added with the go/v key, we set the handler to be + // handleVKeyAction. handleVKeyAction then looks up in th + // shortcut_table to see what the relevant handler should be + bindings.slice(1).forEach(binding => + this._shortcut_v_table.set(binding, handler)); } else { this.addOwnKeyBinding(bindings.join(' '), handler); } @@ -593,6 +646,14 @@ export const KeyboardShortcutBehavior = [ this.addOwnKeyBinding(key, '_handleGoAction'); }); } + + this.addOwnKeyBinding('v:keydown', '_handleVKeyDown'); + this.addOwnKeyBinding('v:keyup', '_handleVKeyUp'); + if (this._shortcut_v_table.size > 0) { + this._shortcut_v_table.forEach((handler, key) => { + this.addOwnKeyBinding(key, '_handleVAction'); + }); + } }, /** @override */ @@ -614,6 +675,33 @@ export const KeyboardShortcutBehavior = [ shortcutManager.removeListener(listener); }, + _handleVKeyDown(e) { + this._shortcut_v_key_last_pressed = Date.now(); + }, + + _handleVKeyUp(e) { + setTimeout(() => { + this._shortcut_v_key_last_pressed = null; + }, V_KEY_TIMEOUT_MS); + }, + + _inVKeyMode() { + return this._shortcut_v_key_last_pressed && + (Date.now() - this._shortcut_v_key_last_pressed <= + V_KEY_TIMEOUT_MS); + }, + + _handleVAction(e) { + if (!this._inVKeyMode() || + !this._shortcut_v_table.has(e.detail.key) || + this.shouldSuppressKeyboardShortcut(e)) { + return; + } + e.preventDefault(); + const handler = this._shortcut_v_table.get(e.detail.key); + this[handler](e); + }, + _handleGoKeyDown(e) { this._shortcut_go_key_last_pressed = Date.now(); }, @@ -648,6 +736,7 @@ export const KeyboardShortcutBehavior = [ export const KeyboardShortcutBinder = { DOC_ONLY, GO_KEY, + V_KEY, Shortcut, ShortcutManager, ShortcutSection, diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index 799addefb1..1da1f350ec 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js @@ -441,6 +441,13 @@ class GrChangeView extends mixinBehaviors( [ [this.Shortcut.COLLAPSE_ALL_MESSAGES]: '_handleCollapseAllMessages', [this.Shortcut.OPEN_DIFF_PREFS]: '_handleOpenDiffPrefsShortcut', [this.Shortcut.EDIT_TOPIC]: '_handleEditTopic', + [this.Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase', + [this.Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest', + [this.Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft', + [this.Shortcut.DIFF_RIGHT_AGAINST_LATEST]: + '_handleDiffRightAgainstLatest', + [this.Shortcut.DIFF_BASE_AGAINST_LATEST]: + '_handleDiffBaseAgainstLatest', }; } @@ -1404,6 +1411,82 @@ class GrChangeView extends mixinBehaviors( [ this.$.metadata.editTopic(); } + _handleDiffAgainstBase(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + if (this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Base is already selected.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToChange(this._change, this._patchRange.patchNum); + } + + _handleDiffBaseAgainstLeft(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + if (this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Left is already base.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToChange(this._change, this._patchRange.basePatchNum); + } + + _handleDiffAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum)) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Latest is already selected.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToChange(this._change, latestPatchNum, + this._patchRange.basePatchNum); + } + + _handleDiffRightAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum)) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Right is already latest.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToChange(this._change, latestPatchNum, + this._patchRange.patchNum); + } + + _handleDiffBaseAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum) && + this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Already diffing base against latest.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToChange(this._change, latestPatchNum); + } + _handleRefreshChange(e) { if (this.shouldSuppressKeyboardShortcut(e)) { return; } e.preventDefault(); diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js index acd16d719f..5624cc776b 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js @@ -333,6 +333,81 @@ suite('gr-change-view tests', () => { assert.isTrue(replaceStateStub.called); }); + test('_handleDiffAgainstBase', () => { + element._changeNum = '1'; + element._patchRange = { + patchNum: 3, + basePatchNum: 1, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + element._handleDiffAgainstBase(new CustomEvent('')); + assert(navigateToChangeStub.called); + const args = navigateToChangeStub.getCall(0).args; + assert.equal(args[1], 3); + assert.isNotOk(args[0]); + }); + + test('_handleDiffAgainstLatest', () => { + element._changeNum = '1'; + element._patchRange = { + basePatchNum: 1, + patchNum: 3, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + element._handleDiffAgainstLatest(new CustomEvent('')); + assert(navigateToChangeStub.called); + const args = navigateToChangeStub.getCall(0).args; + assert.equal(args[1], 10); + assert.equal(args[2], 1); + }); + + test('_handleDiffBaseAgainstLeft', () => { + element._changeNum = '1'; + element._patchRange = { + patchNum: 3, + basePatchNum: 1, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + element._handleDiffBaseAgainstLeft(new CustomEvent('')); + assert(navigateToChangeStub.called); + const args = navigateToChangeStub.getCall(0).args; + assert.equal(args[1], 1); + assert.isNotOk(args[0]); + }); + + test('_handleDiffRightAgainstLatest', () => { + element._changeNum = '1'; + element._patchRange = { + basePatchNum: 1, + patchNum: 3, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + element._handleDiffRightAgainstLatest(new CustomEvent('')); + assert(navigateToChangeStub.called); + const args = navigateToChangeStub.getCall(0).args; + assert.equal(args[1], 10); + assert.equal(args[2], 3); + }); + + test('_handleDiffBaseAgainstLatest', () => { + element._changeNum = '1'; + element._patchRange = { + basePatchNum: 1, + patchNum: 3, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + element._handleDiffBaseAgainstLatest(new CustomEvent('')); + assert(navigateToChangeStub.called); + const args = navigateToChangeStub.getCall(0).args; + assert.equal(args[1], 10); + assert.isNotOk(args[2]); + }); + suite('plugins adding to file tab', () => { setup(done => { // Resolving it here instead of during setup() as other tests depend diff --git a/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js index 5c54011ce6..b8b414d70d 100644 --- a/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js +++ b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js @@ -30,7 +30,10 @@ class GrKeyBindingDisplay extends GestureEventListeners( static get properties() { return { - /** @type {Array} */ + /** @type {Array>} + * Each entry in the binding represents an array that is a keyboard + * shortcut containing [modifier, combination] + */ binding: Array, }; } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index cf89c6308a..12ee74d0dc 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -300,6 +300,13 @@ class GrDiffView extends mixinBehaviors( [ [this.Shortcut.TOGGLE_BLAME]: '_handleToggleBlame', [this.Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]: '_handleToggleHideAllCommentThreads', + [this.Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase', + [this.Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest', + [this.Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft', + [this.Shortcut.DIFF_RIGHT_AGAINST_LATEST]: + '_handleDiffRightAgainstLatest', + [this.Shortcut.DIFF_BASE_AGAINST_LATEST]: + '_handleDiffBaseAgainstLatest', // Final two are actually handled by gr-comment-thread. [this.Shortcut.EXPAND_ALL_COMMENT_THREADS]: null, @@ -1287,6 +1294,87 @@ class GrDiffView extends mixinBehaviors( [ this.toggleClass('hideComments'); } + _handleDiffAgainstBase(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + if (this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Base is already selected.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToDiff( + this._change, this._path, this._patchRange.patchNum); + } + + _handleDiffBaseAgainstLeft(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + if (this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Left is already base.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToDiff(this._change, this._path, + this._patchRange.basePatchNum); + } + + _handleDiffAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum)) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Latest is already selected.', + }, + composed: true, bubbles: true, + })); + return; + } + + GerritNav.navigateToDiff( + this._change, this._path, latestPatchNum, + this._patchRange.basePatchNum); + } + + _handleDiffRightAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum)) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Right is already latest.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToDiff(this._change, this._path, latestPatchNum, + this._patchRange.patchNum); + } + + _handleDiffBaseAgainstLatest(e) { + if (this.shouldSuppressKeyboardShortcut(e)) { return; } + const latestPatchNum = this.computeLatestPatchNum(this._allPatchSets); + if (this.patchNumEquals(this._patchRange.patchNum, latestPatchNum) && + this.patchNumEquals(this._patchRange.basePatchNum, 'PARENT')) { + this.dispatchEvent(new CustomEvent('show-alert', { + detail: { + message: 'Already diffing base against latest.', + }, + composed: true, bubbles: true, + })); + return; + } + GerritNav.navigateToDiff(this._change, this._path, latestPatchNum); + } + _computeBlameLoaderClass(isImageDiff, path) { return !this.isMagicPath(path) && !isImageDiff ? 'show' : ''; } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html index 21d1144c72..0dce3f7f96 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html @@ -287,6 +287,81 @@ suite('gr-diff-view tests', () => { assert.isTrue(expandStub.called); }); + test('diff against base', () => { + element._patchRange = { + basePatchNum: '5', + patchNum: '10', + }; + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + const diffNavStub = sandbox.stub(GerritNav, 'navigateToDiff'); + element._handleDiffAgainstBase(new CustomEvent('')); + const args = diffNavStub.getCall(0).args; + assert.equal(args[2], 10); + assert.isNotOk(args[3]); + }); + + test('diff against latest', () => { + element._patchRange = { + basePatchNum: '5', + patchNum: '10', + }; + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + sandbox.stub(element, 'computeLatestPatchNum').returns(12); + const diffNavStub = sandbox.stub(GerritNav, 'navigateToDiff'); + element._handleDiffAgainstLatest(new CustomEvent('')); + const args = diffNavStub.getCall(0).args; + assert.equal(args[2], 12); + assert.equal(args[3], 5); + }); + + test('_handleDiffBaseAgainstLeft', () => { + element._changeNum = '1'; + element._patchRange = { + patchNum: 3, + basePatchNum: 1, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + const diffNavStub = sandbox.stub(GerritNav, 'navigateToDiff'); + element._handleDiffBaseAgainstLeft(new CustomEvent('')); + assert(diffNavStub.called); + const args = diffNavStub.getCall(0).args; + assert.equal(args[2], 1); + assert.isNotOk(args[3]); + }); + + test('_handleDiffRightAgainstLatest', () => { + element._changeNum = '1'; + element._patchRange = { + basePatchNum: 1, + patchNum: 3, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + const diffNavStub = sandbox.stub(GerritNav, 'navigateToDiff'); + element._handleDiffRightAgainstLatest(new CustomEvent('')); + assert(diffNavStub.called); + const args = diffNavStub.getCall(0).args; + assert.equal(args[2], 10); + assert.equal(args[3], 3); + }); + + test('_handleDiffBaseAgainstLatest', () => { + element._changeNum = '1'; + element._patchRange = { + basePatchNum: 1, + patchNum: 3, + }; + sandbox.stub(element, 'computeLatestPatchNum').returns(10); + sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false); + const diffNavStub = sandbox.stub(GerritNav, 'navigateToDiff'); + element._handleDiffBaseAgainstLatest(new CustomEvent('')); + assert(diffNavStub.called); + const args = diffNavStub.getCall(0).args; + assert.equal(args[2], 10); + assert.isNotOk(args[3]); + }); + test('keyboard shortcuts with patch range', () => { element._changeNum = '42'; element._patchRange = { diff --git a/polygerrit-ui/app/elements/gr-app-element.js b/polygerrit-ui/app/elements/gr-app-element.js index 91af97c077..a01e8a4d19 100644 --- a/polygerrit-ui/app/elements/gr-app-element.js +++ b/polygerrit-ui/app/elements/gr-app-element.js @@ -270,9 +270,9 @@ class GrAppElement extends mixinBehaviors( [ this.Shortcut.EDIT_TOPIC, 't'); this.bindShortcut( - this.Shortcut.OPEN_REPLY_DIALOG, 'a'); + this.Shortcut.OPEN_REPLY_DIALOG, 'a:keyup'); this.bindShortcut( - this.Shortcut.OPEN_DOWNLOAD_DIALOG, 'd'); + this.Shortcut.OPEN_DOWNLOAD_DIALOG, 'd:keyup'); this.bindShortcut( this.Shortcut.EXPAND_ALL_MESSAGES, 'x'); this.bindShortcut( @@ -285,6 +285,16 @@ class GrAppElement extends mixinBehaviors( [ this.Shortcut.UP_TO_CHANGE, 'u'); this.bindShortcut( this.Shortcut.TOGGLE_DIFF_MODE, 'm:keyup'); + this.bindShortcut( + this.Shortcut.DIFF_AGAINST_BASE, this.V_KEY, 'down', 's'); + this.bindShortcut( + this.Shortcut.DIFF_AGAINST_LATEST, this.V_KEY, 'up', 'w'); + this.bindShortcut( + this.Shortcut.DIFF_BASE_AGAINST_LEFT, this.V_KEY, 'left', 'a'); + this.bindShortcut( + this.Shortcut.DIFF_RIGHT_AGAINST_LATEST, this.V_KEY, 'right', 'd'); + this.bindShortcut( + this.Shortcut.DIFF_BASE_AGAINST_LATEST, this.V_KEY, 'b'); this.bindShortcut( this.Shortcut.NEXT_LINE, 'j', 'down');