Change expand diff context shortcut to be a toggle
Also removes this shortcut from the change view because
it was duplicating only the expand part of the
toggleAllInlineDiffs functionality instead of expanding
diff context.
If the user's context preference is already the whole
file, it will collapse to the default 10 line context
when toggled.
Change-Id: I911d5fb57edd427f00543a6eb59e8a684f8a8caf
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 61006f9..bf7f53f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -571,7 +571,6 @@
[Shortcut.UP_TO_DASHBOARD]: '_handleUpToDashboard',
[Shortcut.EXPAND_ALL_MESSAGES]: '_handleExpandAllMessages',
[Shortcut.COLLAPSE_ALL_MESSAGES]: '_handleCollapseAllMessages',
- [Shortcut.EXPAND_ALL_DIFF_CONTEXT]: '_expandAllDiffs',
[Shortcut.OPEN_DIFF_PREFS]: '_handleOpenDiffPrefsShortcut',
[Shortcut.EDIT_TOPIC]: '_handleEditTopic',
[Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index ae1e438..ff60531 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -564,8 +564,8 @@
this.$.diff.clearDiffContent();
}
- expandAllContext() {
- this.$.diff.expandAllContext();
+ toggleAllContext() {
+ this.$.diff.toggleAllContext();
}
_getLoggedIn() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index 99486ad..01c78f0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -796,9 +796,9 @@
assert.equal(stub.lastCall.args.length, 0);
});
- test('delegates expandAllContext()', () => {
- const stub = sinon.stub(element.$.diff, 'expandAllContext');
- element.expandAllContext();
+ test('delegates toggleAllContext()', () => {
+ const stub = sinon.stub(element.$.diff, 'toggleAllContext');
+ element.toggleAllContext();
assert.isTrue(stub.calledOnce);
assert.equal(stub.lastCall.args.length, 0);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 1550108..b31333c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -291,7 +291,7 @@
[Shortcut.OPEN_DIFF_PREFS]: '_handleCommaKey',
[Shortcut.TOGGLE_DIFF_MODE]: '_handleToggleDiffMode',
[Shortcut.TOGGLE_FILE_REVIEWED]: '_throttledToggleFileReviewed',
- [Shortcut.EXPAND_ALL_DIFF_CONTEXT]: '_handleExpandAllDiffContext',
+ [Shortcut.TOGGLE_ALL_DIFF_CONTEXT]: '_handleToggleAllDiffContext',
[Shortcut.NEXT_UNREVIEWED_FILE]: '_handleNextUnreviewedFile',
[Shortcut.TOGGLE_BLAME]: '_handleToggleBlame',
[Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]:
@@ -1744,10 +1744,10 @@
return '';
}
- _handleExpandAllDiffContext(e: CustomKeyboardEvent) {
+ _handleToggleAllDiffContext(e: CustomKeyboardEvent) {
if (this.shouldSuppressKeyboardShortcut(e)) return;
- this.$.diffHost.expandAllContext();
+ this.$.diffHost.toggleAllContext();
}
_computeDiffPrefsDisabled(disableDiffPrefs?: boolean, loggedIn?: boolean) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 46a670d..b5473a3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -63,7 +63,7 @@
kb.bindShortcut(Shortcut.OPEN_DIFF_PREFS, ',');
kb.bindShortcut(Shortcut.TOGGLE_DIFF_MODE, 'm');
kb.bindShortcut(Shortcut.TOGGLE_FILE_REVIEWED, 'r');
- kb.bindShortcut(Shortcut.EXPAND_ALL_DIFF_CONTEXT, 'shift+x');
+ kb.bindShortcut(Shortcut.TOGGLE_ALL_DIFF_CONTEXT, 'shift+x');
kb.bindShortcut(Shortcut.EXPAND_ALL_COMMENT_THREADS, 'e');
kb.bindShortcut(Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS, 'h');
kb.bindShortcut(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, 'shift+e');
@@ -508,11 +508,11 @@
assert.isTrue(diffChangeStub.called);
});
- test('shift+x shortcut expands all diff context', () => {
- const expandStub = sinon.stub(element.$.diffHost, 'expandAllContext');
+ test('shift+x shortcut toggles all diff context', () => {
+ const toggleStub = sinon.stub(element.$.diffHost, 'toggleAllContext');
MockInteractions.pressAndReleaseKeyOn(element, 88, 'shift', 'x');
flush();
- assert.isTrue(expandStub.called);
+ assert.isTrue(toggleStub.called);
});
test('diff against base', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 3fb1a50..189bac7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -55,7 +55,11 @@
PolymerDomWrapper,
} from '../../../types/types';
import {CommentRangeLayer} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
-import {DiffViewMode, Side} from '../../../constants/constants';
+import {
+ createDefaultDiffPrefs,
+ DiffViewMode,
+ Side,
+} from '../../../constants/constants';
import {KeyLocations} from '../gr-diff-processor/gr-diff-processor';
import {FlattenedNodesObserver} from '@polymer/polymer/lib/utils/flattened-nodes-observer';
import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces';
@@ -77,7 +81,6 @@
const LARGE_DIFF_THRESHOLD_LINES = 10000;
const FULL_CONTEXT = -1;
-const LIMITED_CONTEXT = 10;
const COMMIT_MSG_PATH = '/COMMIT_MSG';
/**
@@ -968,8 +971,12 @@
this._debounceRenderDiffTable();
}
- _handleLimitedBypass() {
- this._safetyBypass = LIMITED_CONTEXT;
+ _collapseContext() {
+ // Uses the default context amount if the preference is for the entire file.
+ this._safetyBypass =
+ this.prefs?.context && this.prefs.context >= 0
+ ? null
+ : createDefaultDiffPrefs().context;
this._debounceRenderDiffTable();
}
@@ -981,8 +988,15 @@
return errorMessage ? 'showError' : '';
}
- expandAllContext() {
- this._handleFullBypass();
+ toggleAllContext() {
+ if (!this.prefs) {
+ return;
+ }
+ if (this._getBypassPrefs(this.prefs).context < 0) {
+ this._collapseContext();
+ } else {
+ this._handleFullBypass();
+ }
}
_computeNewlineWarning(warnLeft: boolean, warnRight: boolean) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
index 4b477cd..4d0e566 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
@@ -623,7 +623,7 @@
Prevented render because "Whole file" is enabled and this diff is very
large (about [[_diffLength]] lines).
</p>
- <gr-button on-click="_handleLimitedBypass">
+ <gr-button on-click="_collapseContext">
Render with limited context
</gr-button>
<gr-button on-click="_handleFullBypass">
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
index ec44f92..1bdf498 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
@@ -797,6 +797,43 @@
element.addEventListener('render', rendered);
element._renderDiffTable();
});
+
+ test('toggles expand context using bypass', async () => {
+ element.prefs = {...MINIMAL_PREFS, context: 3};
+
+ element.toggleAllContext();
+ element._renderDiffTable();
+ await flush();
+
+ assert.equal(element.prefs.context, 3);
+ assert.equal(element._safetyBypass, -1);
+ assert.equal(renderStub.firstCall.lastArg.context, -1);
+ });
+
+ test('toggles collapse context from bypass', async () => {
+ element.prefs = {...MINIMAL_PREFS, context: 3};
+ element._safetyBypass = -1;
+
+ element.toggleAllContext();
+ element._renderDiffTable();
+ await flush();
+
+ assert.equal(element.prefs.context, 3);
+ assert.isNull(element._safetyBypass);
+ assert.equal(renderStub.firstCall.lastArg.context, 3);
+ });
+
+ test('toggles collapse context from pref using default', async () => {
+ element.prefs = {...MINIMAL_PREFS, context: -1};
+
+ element.toggleAllContext();
+ element._renderDiffTable();
+ await flush();
+
+ assert.equal(element.prefs.context, -1);
+ assert.equal(element._safetyBypass, 10);
+ assert.equal(renderStub.firstCall.lastArg.context, 10);
+ });
});
suite('blame', () => {
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index cdc9f98..e4d72e1 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -392,7 +392,7 @@
}
this.bindShortcut(Shortcut.NEXT_CHUNK, 'n');
this.bindShortcut(Shortcut.PREV_CHUNK, 'p');
- this.bindShortcut(Shortcut.EXPAND_ALL_DIFF_CONTEXT, 'shift+x');
+ this.bindShortcut(Shortcut.TOGGLE_ALL_DIFF_CONTEXT, 'shift+x');
this.bindShortcut(Shortcut.NEXT_COMMENT_THREAD, 'shift+n');
this.bindShortcut(Shortcut.PREV_COMMENT_THREAD, 'shift+p');
this.bindShortcut(
diff --git a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
index 881e102..63a05aa 100644
--- a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
+++ b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
@@ -175,7 +175,7 @@
VISIBLE_LINE = 'VISIBLE_LINE',
NEXT_CHUNK = 'NEXT_CHUNK',
PREV_CHUNK = 'PREV_CHUNK',
- EXPAND_ALL_DIFF_CONTEXT = 'EXPAND_ALL_DIFF_CONTEXT',
+ TOGGLE_ALL_DIFF_CONTEXT = 'TOGGLE_ALL_DIFF_CONTEXT',
NEXT_COMMENT_THREAD = 'NEXT_COMMENT_THREAD',
PREV_COMMENT_THREAD = 'PREV_COMMENT_THREAD',
EXPAND_ALL_COMMENT_THREADS = 'EXPAND_ALL_COMMENT_THREADS',
@@ -403,9 +403,9 @@
'Go to previous diff chunk'
);
_describe(
- Shortcut.EXPAND_ALL_DIFF_CONTEXT,
+ Shortcut.TOGGLE_ALL_DIFF_CONTEXT,
ShortcutSection.DIFFS,
- 'Expand all diff context'
+ 'Toggle all diff context'
);
_describe(
Shortcut.NEXT_COMMENT_THREAD,
