Merge "Add keyboard shortcuts for diffing against base and latest"

This commit is contained in:
Dhruv Srivastava
2020-06-26 10:36:20 +00:00
committed by Gerrit Code Review
7 changed files with 452 additions and 29 deletions

View File

@@ -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,

View File

@@ -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();

View File

@@ -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

View File

@@ -30,7 +30,10 @@ class GrKeyBindingDisplay extends GestureEventListeners(
static get properties() {
return {
/** @type {Array<string>} */
/** @type {Array<Array<string>>}
* Each entry in the binding represents an array that is a keyboard
* shortcut containing [modifier, combination]
*/
binding: Array,
};
}

View File

@@ -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' : '';
}

View File

@@ -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 = {

View File

@@ -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');