Migrate binary diff from legacy diff to lit diff
The support of binary diffs is extremely poor: We are just showing a
diff for the FILE line, so that you can comment on the file as a whole.
Otherwise we are just saying: "There is a difference in binary files."
So there was no point in migrating the diff pixel perfect. Here are
the changes: https://imgur.com/a/b7MxTtn
The pattern for how the image diff builder is implemented and called
is not great, but we use it anyway, because now is not the time to
think about it. And these two builders being consistent is of value.
The primary goal here is to remove the (last) dependency on the
legacy diff builder. Once this change is submitted, we can start
removing it. Yay!
Release-Notes: skip
Google-Bug-Id: b/280018838
Change-Id: I5961a3cd47696077baeffa1698bd34b23588bca6
(cherry picked from commit df87880519ab685d0496031dbd81fe73cf8f0cfc)
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
index 5267f307..502c6cf 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
@@ -3,16 +3,17 @@
* Copyright 2017 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
+import {GrDiffBuilderLit} from './gr-diff-builder-lit';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {createElementDiff} from '../gr-diff/gr-diff-utils';
import {GrDiffGroup} from '../gr-diff/gr-diff-group';
-import {GrDiffLine} from '../gr-diff/gr-diff-line';
-import {GrDiffLineType} from '../../../api/diff';
-import {queryAndAssert} from '../../../utils/common-util';
import {html, render} from 'lit';
+import {BinaryDiffBuilder} from './gr-diff-builder';
-export class GrDiffBuilderBinary extends GrDiffBuilderUnified {
+export class GrDiffBuilderBinary
+ extends GrDiffBuilderLit
+ implements BinaryDiffBuilder
+{
constructor(
diff: DiffInfo,
prefs: DiffPreferencesInfo,
@@ -25,17 +26,21 @@
const section = createElementDiff('tbody', 'binary-diff');
// Do not create a diff row for 'LOST'.
if (group.lines[0].beforeNumber !== 'FILE') return section;
+ return super.buildSectionElement(group);
+ }
- const line = new GrDiffLine(GrDiffLineType.BOTH, 'FILE', 'FILE');
- const fileRow = this.createRow(line);
- const contentTd = queryAndAssert<HTMLTableCellElement>(
- fileRow,
- 'td.both.file'
- )!;
- const div = document.createElement('div');
- render(html`<span>Difference in binary files</span>`, div);
- contentTd.insertBefore(div, contentTd.firstChild);
- section.appendChild(fileRow);
- return section;
+ public renderBinaryDiff() {
+ render(
+ html`
+ <tbody class="gr-diff binary-diff">
+ <tr class="gr-diff">
+ <td colspan="5" class="gr-diff">
+ <span>Difference in binary files</span>
+ </td>
+ </tr>
+ </tbody>
+ `,
+ this.outputEl
+ );
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 7fa6d72..8149d0e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -12,6 +12,7 @@
ImageDiffBuilder,
DiffContextExpandedEventDetail,
isImageDiffBuilder,
+ isBinaryDiffBuilder,
} from './gr-diff-builder';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
import {GrDiffBuilderBinary} from './gr-diff-builder-binary';
@@ -211,6 +212,8 @@
.then(async () => {
if (isImageDiffBuilder(this.builder)) {
this.builder.renderImageDiff();
+ } else if (isBinaryDiffBuilder(this.builder)) {
+ this.builder.renderBinaryDiff();
}
await this.untilGroupsRendered();
fire(this.diffElement, 'render-content', {});
@@ -437,7 +440,6 @@
this.useNewImageDiffUi
);
} else if (this.diff.binary) {
- // If the diff is binary, but not an image.
return new GrDiffBuilderBinary(this.diff, localPrefs, this.diffElement);
} else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
this.renderPrefs = {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-lit.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-lit.ts
index 2a61ef2..c834afc 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-lit.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-lit.ts
@@ -136,7 +136,7 @@
};
}
- protected override buildSectionElement(group: GrDiffGroup) {
+ protected override buildSectionElement(group: GrDiffGroup): HTMLElement {
const leftCl = `left-${group.startLine(Side.LEFT)}`;
const rightCl = `right-${group.startLine(Side.RIGHT)}`;
const section = html`
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
index 5ca5197..2581c03 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
@@ -74,6 +74,16 @@
return !!x && !!(x as ImageDiffBuilder).renderImageDiff;
}
+export interface BinaryDiffBuilder extends DiffBuilder {
+ renderBinaryDiff(): void;
+}
+
+export function isBinaryDiffBuilder(
+ x: DiffBuilder | BinaryDiffBuilder | undefined
+): x is BinaryDiffBuilder {
+ return !!x && !!(x as BinaryDiffBuilder).renderBinaryDiff;
+}
+
/**
* Base class for different diff builders, like side-by-side, unified etc.
*
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 1c5d594..3d54690 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -413,11 +413,16 @@
.image-diff .right.lineNumButton {
border-left: 1px solid var(--border-color);
}
- .image-diff label,
- .binary-diff label {
+ .image-diff label {
font-family: var(--font-family);
font-style: italic;
}
+ tbody.binary-diff td {
+ font-family: var(--font-family);
+ font-style: italic;
+ text-align: center;
+ padding: var(--spacing-s) 0;
+ }
.diff-row {
outline: none;
user-select: none;
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 227ac2d..f0826ad 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
@@ -3142,18 +3142,25 @@
element,
/* HTML */ `
<div class="diffContainer sideBySide">
+ <gr-diff-section class="left-FILE right-FILE"> </gr-diff-section>
+ <gr-diff-row class="left-FILE right-FILE"> </gr-diff-row>
<table class="selected-right" id="diffTable">
<colgroup>
<col class="blame gr-diff" />
- <col class="gr-diff" width="48" />
- <col class="gr-diff" width="48" />
- <col class="gr-diff" />
+ <col class="gr-diff left" width="48" />
+ <col class="gr-diff left sign" />
+ <col class="gr-diff left" />
+ <col class="gr-diff right" width="48" />
+ <col class="gr-diff right sign" />
+ <col class="gr-diff right" />
</colgroup>
<tbody class="binary-diff gr-diff"></tbody>
- <tbody class="binary-diff gr-diff">
+ <tbody class="both gr-diff section">
<tr
- aria-labelledby="left-button-FILE right-button-FILE right-content-FILE"
- class="both diff-row gr-diff unified"
+ aria-labelledby="left-button-FILE left-content-FILE right-button-FILE right-content-FILE"
+ class="diff-row gr-diff side-by-side"
+ left-type="both"
+ right-type="both"
tabindex="-1"
>
<td class="blame gr-diff" data-line-number="FILE"></td>
@@ -3168,6 +3175,14 @@
File
</button>
</td>
+ <td class="gr-diff left no-intraline-info sign"></td>
+ <td
+ class="both content file gr-diff left no-intraline-info"
+ >
+ <div class="thread-group" data-side="left">
+ <slot name="left-FILE"> </slot>
+ </div>
+ </td>
<td class="gr-diff lineNum right" data-value="FILE">
<button
aria-label="Add file comment"
@@ -3179,19 +3194,23 @@
File
</button>
</td>
+ <td class="gr-diff no-intraline-info right sign"></td>
<td
class="both content file gr-diff no-intraline-info right"
>
- <div>
- <span> Difference in binary files </span>
- </div>
<div class="thread-group" data-side="right">
<slot name="right-FILE"> </slot>
- <slot name="left-FILE"> </slot>
</div>
</td>
</tr>
</tbody>
+ <tbody class="binary-diff gr-diff">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="5">
+ <span> Difference in binary files </span>
+ </td>
+ </tr>
+ </tbody>
</table>
</div>
`
