Move creating a GrDiffProcessor to the diff model
*Replacing* the current GrDiffProcessor will happen in a follow-up.
Release-Notes: skip
Google-Bug-Id: b/280019334
Change-Id: I6863d863c397e638928e36668a02e1996febe30e
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
index d309556..0b688a6 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
@@ -188,6 +188,45 @@
right: {[key: string]: boolean};
}
+/**
+ * "Context" is the number of lines that we are showing around diff chunks and
+ * commented lines. This typically comes from a user preference and is set to
+ * something like 3 or 10.
+ *
+ * `FULL_CONTEXT` means that the user wants to see the entire file. We could
+ * also call this "infinite context".
+ */
+export const FULL_CONTEXT = -1;
+
+export enum FullContext {
+ /** User has opted into showing the full context. */
+ YES = 'YES',
+ /** User has opted into showing only limited context. */
+ NO = 'NO',
+ /**
+ * User has not decided yet. Will see a warning message with two options then,
+ * if the file is too large.
+ */
+ UNDECIDED = 'UNDECIDED',
+}
+
+export function computeContext(
+ prefsContext: number | undefined,
+ showFullContext: FullContext,
+ defaultContext: number
+) {
+ if (showFullContext === FullContext.YES) {
+ return FULL_CONTEXT;
+ }
+ if (
+ prefsContext &&
+ !(showFullContext === FullContext.NO && prefsContext === FULL_CONTEXT)
+ ) {
+ return prefsContext;
+ }
+ return defaultContext;
+}
+
export function computeKeyLocations(
lineOfInterest: DisplayLine | undefined,
comments: GrDiffCommentThread[]
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
index 7e6e7fc..28c6c08 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
@@ -15,6 +15,9 @@
toCommentThreadModel,
compareComments,
GrDiffThreadElement,
+ computeContext,
+ FULL_CONTEXT,
+ FullContext,
} from './gr-diff-utils';
import {FILE, LOST, Side} from '../../../api/diff';
@@ -184,6 +187,26 @@
assert.isUndefined(getRange(threadEl));
});
+ suite('computeContext', () => {
+ test('computeContext 1', () => {
+ assert.equal(computeContext(1, FullContext.YES, 2), FULL_CONTEXT);
+ assert.equal(computeContext(1, FullContext.NO, 2), 1);
+ assert.equal(computeContext(1, FullContext.UNDECIDED, 2), 1);
+ });
+
+ test('computeContext FULL_CONTEXT', () => {
+ assert.equal(
+ computeContext(FULL_CONTEXT, FullContext.YES, 2),
+ FULL_CONTEXT
+ );
+ assert.equal(computeContext(FULL_CONTEXT, FullContext.NO, 2), 2);
+ assert.equal(
+ computeContext(FULL_CONTEXT, FullContext.UNDECIDED, 2),
+ FULL_CONTEXT
+ );
+ });
+ });
+
suite('key locations', () => {
test('lineOfInterest is a key location', () => {
const lineOfInterest = {lineNum: 789, side: Side.LEFT};
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index 1f212b0..54d578a 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -28,6 +28,7 @@
compareComments,
toCommentThreadModel,
KeyLocations,
+ FullContext,
} from '../gr-diff/gr-diff-utils';
import {BlameInfo, CommentRange, ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
@@ -40,11 +41,7 @@
CommentRangeLayer,
GrRangedCommentLayer,
} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
-import {
- createDefaultDiffPrefs,
- DiffViewMode,
- Side,
-} from '../../../constants/constants';
+import {DiffViewMode, Side} from '../../../constants/constants';
import {
GrDiffProcessor,
ProcessingOptions,
@@ -236,17 +233,6 @@
@property({type: Boolean})
override isContentEditable = isSafari();
- /**
- * Whether the safety check for large diffs when whole-file is set has
- * been bypassed. If the value is null, then the safety has not been
- * bypassed. If the value is a number, then that number represents the
- * context preference to use when rendering the bypassed diff.
- *
- * Private but used in tests.
- */
- @state()
- safetyBypass: number | null = null;
-
// Private but used in tests.
@state()
showWarning?: boolean;
@@ -288,7 +274,7 @@
// Private but used in tests.
highlights = new GrDiffHighlight();
- private diffModel = new DiffModel(undefined);
+ private diffModel = new DiffModel();
// visible for testing
builder?: GrDiffBuilder;
@@ -312,6 +298,7 @@
private rangeLayer?: GrRangedCommentLayer;
+ // TODO: Remove. Let the model instantiate the processor.
// visible for testing
processor?: GrDiffProcessor;
@@ -322,8 +309,12 @@
*/
private groups: GrDiffGroup[] = [];
+ // TODO: Can be removed when GrDiffProcessor is not instantiated anymore.
private keyLocations: KeyLocations = {left: {}, right: {}};
+ // TODO: Can be removed when GrDiffProcessor is not instantiated anymore.
+ private context = 3;
+
static override get styles() {
return [
iconStyles,
@@ -342,6 +333,11 @@
() => this.diffModel.keyLocations$,
keyLocations => (this.keyLocations = keyLocations)
);
+ subscribe(
+ this,
+ () => this.diffModel.context$,
+ context => (this.context = context)
+ );
this.addEventListener(
'create-range-comment',
(e: CustomEvent<CreateRangeCommentEventDetail>) =>
@@ -384,13 +380,16 @@
changedProperties.has('prefs') ||
changedProperties.has('lineOfInterest')
) {
- this.diffModel.updateState({
- diff: this.diff,
- path: this.path,
- renderPrefs: this.renderPrefs,
- diffPrefs: this.prefs,
- lineOfInterest: this.lineOfInterest,
- });
+ if (this.diff && this.prefs) {
+ this.diffModel.updateState({
+ diff: this.diff,
+ path: this.path,
+ renderPrefs: this.renderPrefs ?? {},
+ diffPrefs: this.prefs,
+ lineOfInterest: this.lineOfInterest,
+ isImageDiff: this.isImageDiff,
+ });
+ }
}
if (
changedProperties.has('path') ||
@@ -763,7 +762,7 @@
private cleanup() {
this.cancel();
this.blame = null;
- this.safetyBypass = null;
+ this.diffModel.updateState({showFullContext: FullContext.UNDECIDED});
this.showWarning = false;
this.clearDiffContent();
}
@@ -911,10 +910,10 @@
return;
}
if (
- this.getBypassPrefs().context === -1 &&
+ this.prefs.context === FULL_CONTEXT &&
+ this.diffModel.getState().showFullContext === FullContext.UNDECIDED &&
this.diffLength &&
- this.diffLength >= LARGE_DIFF_THRESHOLD_LINES &&
- this.safetyBypass === null
+ this.diffLength >= LARGE_DIFF_THRESHOLD_LINES
) {
this.showWarning = true;
fire(this, 'render', {});
@@ -1048,18 +1047,6 @@
lostCell.insertBefore(div, lostCell.firstChild);
}
- /**
- * Get the preferences object including the safety bypass context (if any).
- */
- // visible for testing
- getBypassPrefs() {
- assertIsDefined(this.prefs, 'prefs');
- if (this.safetyBypass !== null) {
- return {...this.prefs, context: this.safetyBypass};
- }
- return this.prefs;
- }
-
clearDiffContent() {
this.unobserveNodes();
if (!this.diffTable) return;
@@ -1083,28 +1070,23 @@
}
private handleFullBypass() {
- this.safetyBypass = FULL_CONTEXT;
+ this.diffModel.updateState({showFullContext: FullContext.YES});
this.debounceRenderDiffTable();
}
private collapseContext() {
+ this.diffModel.updateState({showFullContext: FullContext.NO});
// 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();
}
+ // TODO: Migrate callers to just update prefs.context.
toggleAllContext() {
- if (!this.prefs) {
- return;
- }
- if (this.getBypassPrefs().context < 0) {
- this.collapseContext();
- } else {
- this.handleFullBypass();
- }
+ const current = this.diffModel.getState().showFullContext;
+ this.diffModel.updateState({
+ showFullContext:
+ current === FullContext.YES ? FullContext.NO : FullContext.YES,
+ });
}
private computeNewlineWarning(): string | undefined {
@@ -1152,7 +1134,7 @@
this.builder.addColumns(this.diffTable, getLineNumberCellWidth(this.prefs));
const options: ProcessingOptions = {
- context: this.getBypassPrefs().context,
+ context: this.context,
keyLocations: this.keyLocations,
isBinary: !!(this.isImageDiff || this.diff.binary),
};
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 99db4e9..5603edb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -3801,89 +3801,6 @@
});
});
- suite('safety and bypass', () => {
- let renderStub: sinon.SinonStub;
-
- setup(async () => {
- renderStub = sinon.stub(element, 'legacyRender').callsFake(() => {
- assertIsDefined(element.diffTable);
- element.diffTable.dispatchEvent(
- new CustomEvent('render', {bubbles: true, composed: true})
- );
- return Promise.resolve();
- });
- sinon.stub(element, 'getDiffLength').returns(10000);
- element.diff = createDiff();
- element.noRenderOnPrefsChange = true;
- await element.updateComplete;
- });
-
- test('large render w/ context = 10', async () => {
- element.prefs = {...MINIMAL_PREFS, context: 10};
- element.renderDiffTable();
- await waitForEventOnce(element, 'render');
-
- assert.isTrue(renderStub.called);
- assert.isFalse(element.showWarning);
- });
-
- test('large render w/ whole file and bypass', async () => {
- element.prefs = {...MINIMAL_PREFS, context: -1};
- element.safetyBypass = 10;
- element.renderDiffTable();
- await waitForEventOnce(element, 'render');
-
- assert.isTrue(renderStub.called);
- assert.isFalse(element.showWarning);
- });
-
- test('large render w/ whole file and no bypass', async () => {
- element.prefs = {...MINIMAL_PREFS, context: -1};
- element.renderDiffTable();
- await waitForEventOnce(element, 'render');
-
- assert.isFalse(renderStub.called);
- assert.isTrue(element.showWarning);
- });
-
- test('toggles expand context using bypass', async () => {
- element.prefs = {...MINIMAL_PREFS, context: 3};
-
- element.toggleAllContext();
- element.renderDiffTable();
- await element.updateComplete;
-
- assert.equal(element.prefs.context, 3);
- assert.equal(element.safetyBypass, -1);
- assert.equal(element.getBypassPrefs().context, -1);
- });
-
- test('toggles collapse context from bypass', async () => {
- element.prefs = {...MINIMAL_PREFS, context: 3};
- element.safetyBypass = -1;
-
- element.toggleAllContext();
- element.renderDiffTable();
- await element.updateComplete;
-
- assert.equal(element.prefs.context, 3);
- assert.isNull(element.safetyBypass);
- assert.equal(element.getBypassPrefs().context, 3);
- });
-
- test('toggles collapse context from pref using default', async () => {
- element.prefs = {...MINIMAL_PREFS, context: -1};
-
- element.toggleAllContext();
- element.renderDiffTable();
- await element.updateComplete;
-
- assert.equal(element.prefs.context, -1);
- assert.equal(element.safetyBypass, 10);
- assert.equal(element.getBypassPrefs().context, 10);
- });
- });
-
suite('blame', () => {
test('unsetting', async () => {
element.blame = [];
