Update TokenHighlightLayer using class toggle. Existing implementation works by looking up all the lines that contain either previously highlighted or newly highlighted tokens and requests gr-diff to fully re-render those lines. Which: - Causes all layers (not just highligh) to re-annotate - Potentially loses selection/focus - Slower than proposed solution. Proposed solution instead stores and looks up all annotated elements corresponding to highlighted token and toggles the 'token-highlight' class. Using a dedicated <style> tag and editing it was considered as alternative, but it proved to have worse performance. We are also getting rid of TOKEN_HIGHLIGHT_LIMIT (100). With TOKEN_OCCURENCE_LIMIT (1000) still in place and the new approach the benefit from sorting and only highlighting 100 closest tokens doesn't seem worth added complexity. Release-Notes: skip Google-Bug-Id: b/236688880 Change-Id: Ieddd4b186638d3afdbb45e0b9385becc5ef91d33
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer.ts index b96b156..19e0e22 100644 --- a/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer.ts +++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer.ts
@@ -3,7 +3,7 @@ * Copyright 2021 Google LLC * SPDX-License-Identifier: Apache-2.0 */ -import {DiffLayer, DiffLayerListener} from '../../../types/types'; +import {DiffLayer} from '../../../types/types'; import {GrDiffLine, Side, TokenHighlightListener} from '../../../api/diff'; import {assertIsDefined} from '../../../utils/common-util'; import {GrAnnotation} from '../gr-diff-highlight/gr-annotation'; @@ -44,12 +44,6 @@ const TOKEN_OCCURRENCES_LIMIT = 1000; /** - * Token highlighting is only useful for code on-screen, so we only highlight - * the nearest set of tokens up to this limit. - */ -const TOKEN_HIGHLIGHT_LIMIT = 100; - -/** * When a user hovers over a token in the diff, then this layer makes sure that * all occurrences of this token are annotated with the 'token-highlight' css * class. And removes that class when the user moves the mouse away from the @@ -61,9 +55,6 @@ * And when that re-rendering happens the appropriate css class is added. */ export class TokenHighlightLayer implements DiffLayer { - /** The only listener is typically the renderer of gr-diff. */ - private listeners: DiffLayerListener[] = []; - /** The currently highlighted token. */ private currentHighlight?: string; @@ -92,9 +83,7 @@ * Keeps track of where tokens occur in a file during rendering, so that it is * easy to look up when processing mouse events. */ - private tokenToLinesLeft = new Map<string, Set<number>>(); - - private tokenToLinesRight = new Map<string, Set<number>>(); + private tokenToElements = new Map<string, Set<HTMLElement>>(); private hoveredElement?: Element; @@ -110,12 +99,7 @@ }); } - annotate( - el: HTMLElement, - _: HTMLElement, - line: GrDiffLine, - side: Side - ): void { + annotate(el: HTMLElement, _1: HTMLElement, _2: GrDiffLine, _3: Side): void { const text = el.textContent; if (!text) return; // Binary files encoded as text for example can have super long lines @@ -132,7 +116,7 @@ if (length > TOKEN_LENGTH_LIMIT) continue; atLeastOneTokenMatched = true; const highlightTypeClass = - token === this.currentHighlight ? CSS_HIGHLIGHT : CSS_TOKEN; + token === this.currentHighlight ? CSS_HIGHLIGHT : ''; const textClass = `${TOKEN_TEXT_PREFIX}${token}`; const indexClass = `${TOKEN_INDEX_PREFIX}${index}`; // We add the TOKEN_TEXT_PREFIX class so that we can look up the token later easily @@ -142,12 +126,12 @@ el, index, length, - `${textClass} ${indexClass} ${highlightTypeClass}` + `${textClass} ${indexClass} ${CSS_TOKEN} ${highlightTypeClass}` ); // We could try to detect whether we are re-rendering instead of initially // rendering the line. Then we would not have to call storeLineForToken() // again. But since the Set swallows the duplicates we don't care. - this.storeLineForToken(token, line, side); + this.storeElementsForToken(token, el, textClass); } if (atLeastOneTokenMatched) { // These listeners do not have to be cleaned, because listeners are @@ -162,21 +146,23 @@ } } - private storeLineForToken(token: string, line: GrDiffLine, side: Side) { - const tokenToLines = - side === Side.LEFT ? this.tokenToLinesLeft : this.tokenToLinesRight; - // Just to make sure that we don't break down on large files. - if (tokenToLines.size > TOKEN_COUNT_LIMIT) return; - let numbers = tokenToLines.get(token); - if (!numbers) { - numbers = new Set<number>(); - tokenToLines.set(token, numbers); + private storeElementsForToken( + token: string, + lineEl: HTMLElement, + cssClass: string + ) { + for (const el of lineEl.querySelectorAll(`.${cssClass}`)) { + let tokenEls = this.tokenToElements.get(token); + if (!tokenEls) { + // Just to make sure that we don't break down on large files. + if (this.tokenToElements.size > TOKEN_COUNT_LIMIT) return; + tokenEls = new Set<HTMLElement>(); + this.tokenToElements.set(token, tokenEls); + } + // Just to make sure that we don't break down on large files. + if (tokenEls.size > TOKEN_OCCURRENCES_LIMIT) return; + tokenEls.add(el as HTMLElement); } - // Just to make sure that we don't break down on large files. - if (numbers.size > TOKEN_OCCURRENCES_LIMIT) return; - const lineNumber = - side === Side.LEFT ? line.beforeNumber : line.afterNumber; - numbers.add(Number(lineNumber)); } private handleTokenMouseOut(e: MouseEvent) { @@ -234,10 +220,7 @@ } { if (!(el instanceof Element)) return {line: 0, token: undefined, element: undefined}; - if ( - el.classList.contains(CSS_TOKEN) || - el.classList.contains(CSS_HIGHLIGHT) - ) { + if (el.classList.contains(CSS_TOKEN)) { const tkTextClass = [...el.classList].find(c => c.startsWith(TOKEN_TEXT_PREFIX) ); @@ -265,8 +248,8 @@ this.currentHighlightLineNumber === newLineNumber ) return; + const oldHighlight = this.currentHighlight; - const oldLineNumber = this.currentHighlightLineNumber; this.currentHighlight = newHighlight; this.currentHighlightLineNumber = newLineNumber; this.triggerTokenHighlightEvent( @@ -274,8 +257,21 @@ newLineNumber, newHoveredElement ); - this.notifyForToken(oldHighlight, oldLineNumber); - this.notifyForToken(newHighlight, newLineNumber); + this.toggleTokenHighlight(oldHighlight, CSS_HIGHLIGHT); + this.toggleTokenHighlight(newHighlight, CSS_HIGHLIGHT); + } + + private toggleTokenHighlight(token: string | undefined, cssClass: string) { + if (!token) { + return; + } + const tokenEls = this.tokenToElements.get(token); + if (!tokenEls) { + return; + } + for (const el of tokenEls) { + el.classList.toggle(cssClass); + } } triggerTokenHighlightEvent( @@ -306,58 +302,4 @@ }; this.tokenHighlightListener({token, element, side, range}); } - - getSortedLinesForSide( - lineMapping: Map<string, Set<number>>, - token: string | undefined, - lineNumber: number - ): Array<number> { - if (!token) return []; - const lineSet = lineMapping.get(token); - if (!lineSet) return []; - const lines = [...lineSet]; - lines.sort((a, b) => { - const da = Math.abs(a - lineNumber); - const db = Math.abs(b - lineNumber); - // For equal distance, prefer lines later in the file over earlier in the - // file. This ensures total ordering. - if (da === db) return b - a; - // Compare the distance to lineNumber. - return da - db; - }); - return lines.slice(0, TOKEN_HIGHLIGHT_LIMIT); - } - - notifyForToken(token: string | undefined, lineNumber: number) { - const leftLines = this.getSortedLinesForSide( - this.tokenToLinesLeft, - token, - lineNumber - ); - for (const line of leftLines) { - this.notifyListeners(line, Side.LEFT); - } - const rightLines = this.getSortedLinesForSide( - this.tokenToLinesRight, - token, - lineNumber - ); - for (const line of rightLines) { - this.notifyListeners(line, Side.RIGHT); - } - } - - addListener(listener: DiffLayerListener) { - this.listeners.push(listener); - } - - removeListener(listener: DiffLayerListener) { - this.listeners = this.listeners.filter(f => f !== listener); - } - - notifyListeners(line: number, side: Side) { - for (const listener of this.listeners) { - listener(line, line, side); - } - } }
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer_test.ts index ef2e3d9..c0812b7 100644 --- a/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer_test.ts +++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/token-highlight-layer_test.ts
@@ -55,6 +55,7 @@ highlightDetails?: TokenHighlightEventDetails ) { tokenHighlightingCalls.push({details: highlightDetails}); + listener.notify({details: highlightDetails}); } setup(async () => { @@ -63,7 +64,6 @@ container = document.createElement('div'); document.body.appendChild(container); highlighter = new TokenHighlightLayer(container, tokenHighlightListener); - highlighter.addListener((...args) => listener.notify(...args)); }); teardown(() => { @@ -126,21 +126,21 @@ el, 0, 5, - 'tk-text-these tk-index-0 token' + 'tk-text-these tk-index-0 token ' ); assertAnnotation( annotateElementStub.args[1], el, 6, 3, - 'tk-text-are tk-index-6 token' + 'tk-text-are tk-index-6 token ' ); assertAnnotation( annotateElementStub.args[2], el, 10, 5, - 'tk-text-words tk-index-10 token' + 'tk-text-words tk-index-10 token ' ); }); @@ -196,10 +196,8 @@ // After a total of HOVER_DELAY_MS ms the hover behavior should trigger. clock.tick(HOVER_DELAY_MS - 100); - assert.equal(listener.pending, 2); + assert.equal(listener.pending, 1); assert.equal(_testOnly_allTasks.size, 0); - assert.deepEqual(listener.shift(), [1, 1, Side.LEFT]); - assert.deepEqual(listener.shift(), [2, 2, Side.RIGHT]); }); test('highlighting spans many lines', async () => { @@ -216,10 +214,14 @@ // After a total of HOVER_DELAY_MS ms the hover behavior should trigger. clock.tick(HOVER_DELAY_MS); - assert.equal(listener.pending, 2); + assert.equal(listener.pending, 1); assert.equal(_testOnly_allTasks.size, 0); - assert.deepEqual(listener.shift(), [1, 1, Side.LEFT]); - assert.deepEqual(listener.shift(), [1000, 1000, Side.RIGHT]); + assert.deepEqual(tokenHighlightingCalls[0].details, { + token: 'words', + side: Side.RIGHT, + element: words1, + range: {start_line: 1, start_column: 5, end_line: 1, end_column: 9}, + }); }); test('highlighting mouse out before delay', async () => { @@ -259,10 +261,12 @@ element: words1, range: {start_line: 1, start_column: 5, end_line: 1, end_column: 9}, }); + assert.isTrue(words1.classList.contains('token-highlight')); container.click(); assert.equal(tokenHighlightingCalls.length, 2); assert.deepEqual(tokenHighlightingCalls[1].details, undefined); + assert.isFalse(words1.classList.contains('token-highlight')); }); test('triggers listener on token with single occurrence', async () => { @@ -303,13 +307,13 @@ dispatchMouseEvent('mouseover', words1); assert.equal(listener.pending, 0); clock.tick(HOVER_DELAY_MS); - assert.equal(listener.pending, 2); + assert.equal(listener.pending, 1); listener.flush(); assert.equal(listener.pending, 0); + assert.isTrue(words1.classList.contains('token-highlight')); container.click(); - assert.equal(listener.pending, 2); - assert.deepEqual(listener.shift(), [1, 1, Side.LEFT]); - assert.deepEqual(listener.shift(), [2, 2, Side.RIGHT]); + assert.equal(listener.pending, 1); + assert.isFalse(words1.classList.contains('token-highlight')); }); test('clicking on word does not clear highlight', async () => { @@ -323,11 +327,13 @@ dispatchMouseEvent('mouseover', words1); assert.equal(listener.pending, 0); clock.tick(HOVER_DELAY_MS); - assert.equal(listener.pending, 2); + assert.equal(listener.pending, 1); listener.flush(); assert.equal(listener.pending, 0); + assert.isTrue(words1.classList.contains('token-highlight')); words1.click(); assert.equal(listener.pending, 0); + assert.isTrue(words1.classList.contains('token-highlight')); }); }); });