From ea52207a48da24bb6a5483b1225735ca3778f366 Mon Sep 17 00:00:00 2001 From: Sergio Padrino Date: Wed, 31 Mar 2021 11:04:11 +0200 Subject: [PATCH] Refactor hunk expansion info computation --- app/src/lib/diff-parser.ts | 14 ++- app/src/models/diff/raw-diff.ts | 29 +++++- app/src/ui/diff/diff-helpers.tsx | 4 + app/src/ui/diff/side-by-side-diff.tsx | 6 +- app/src/ui/diff/text-diff-expansion.ts | 130 ++++++++++++++----------- app/src/ui/diff/text-diff.tsx | 23 ++--- 6 files changed, 129 insertions(+), 77 deletions(-) diff --git a/app/src/lib/diff-parser.ts b/app/src/lib/diff-parser.ts index 5cabc74355..4ef579fbbd 100644 --- a/app/src/lib/diff-parser.ts +++ b/app/src/lib/diff-parser.ts @@ -6,6 +6,7 @@ import { DiffLineType, } from '../models/diff' import { assertNever } from '../lib/fatal-error' +import { getHunkHeaderExpansionType } from '../ui/diff/text-diff-expansion' // https://en.wikipedia.org/wiki/Diff_utility // @@ -280,7 +281,11 @@ export class DiffParser { * have no real meaning in the context of a diff and * are only used to aid the app in line-selections. */ - private parseHunk(linesConsumed: number): DiffHunk { + private parseHunk( + linesConsumed: number, + hunkIndex: number, + previousHunk: DiffHunk | null + ): DiffHunk { const headerLine = this.readLine() if (!headerLine) { throw new Error('Expected hunk header but reached end of diff') @@ -366,7 +371,8 @@ export class DiffParser { header, lines, linesConsumed, - linesConsumed + lines.length - 1 + linesConsumed + lines.length - 1, + getHunkHeaderExpansionType(hunkIndex, header, previousHunk) ) } @@ -397,10 +403,12 @@ export class DiffParser { const hunks = new Array() let linesConsumed = 0 + let previousHunk: DiffHunk | null = null do { - const hunk = this.parseHunk(linesConsumed) + const hunk = this.parseHunk(linesConsumed, hunks.length, previousHunk) hunks.push(hunk) + previousHunk = hunk linesConsumed += hunk.lines.length } while (this.peek()) diff --git a/app/src/models/diff/raw-diff.ts b/app/src/models/diff/raw-diff.ts index 9067b9b559..be3c903211 100644 --- a/app/src/models/diff/raw-diff.ts +++ b/app/src/models/diff/raw-diff.ts @@ -1,5 +1,31 @@ import { DiffLine } from './diff-line' +export enum DiffHunkExpansionType { + /** The hunk header cannot be expanded at all. */ + None = 'None', + + /** + * The hunk header can be expanded up exclusively. Only the first hunk can be + * expanded up exclusively. + */ + Up = 'Up', + + /** + * The hunk header can be expanded down exclusively. Only the last hunk (if + * it's the dummy hunk with only one line) can be expanded down exclusively. + */ + Down = 'Down', + + /** The hunk header can be expanded both up and down. */ + Both = 'Both', + + /** + * The hunk header represents a short gap that, when expanded, will + * result in merging this hunk and the hunk above. + */ + Short = 'Short', +} + /** each diff is made up of a number of hunks */ export class DiffHunk { /** @@ -12,7 +38,8 @@ export class DiffHunk { public readonly header: DiffHunkHeader, public readonly lines: ReadonlyArray, public readonly unifiedDiffStart: number, - public readonly unifiedDiffEnd: number + public readonly unifiedDiffEnd: number, + public readonly expansionType: DiffHunkExpansionType ) {} } diff --git a/app/src/ui/diff/diff-helpers.tsx b/app/src/ui/diff/diff-helpers.tsx index 6ca334104a..dedb2f8bbc 100644 --- a/app/src/ui/diff/diff-helpers.tsx +++ b/app/src/ui/diff/diff-helpers.tsx @@ -8,6 +8,7 @@ import { WorkingDirectoryFileChange, CommittedFileChange, } from '../../models/status' +import { DiffHunkExpansionType } from '../../models/diff/raw-diff' /** * DiffRowType defines the different types of @@ -181,6 +182,9 @@ interface IDiffRowHunk { * The actual contents of the line. */ readonly content: string + + /** How the hunk can be expanded. */ + readonly expansionType: DiffHunkExpansionType } export type DiffRow = diff --git a/app/src/ui/diff/side-by-side-diff.tsx b/app/src/ui/diff/side-by-side-diff.tsx index c866fba3fb..0188191b74 100644 --- a/app/src/ui/diff/side-by-side-diff.tsx +++ b/app/src/ui/diff/side-by-side-diff.tsx @@ -864,7 +864,11 @@ function getDiffRowsFromHunk( } if (line.type === DiffLineType.Hunk) { - rows.push({ type: DiffRowType.Hunk, content: line.content }) + rows.push({ + type: DiffRowType.Hunk, + content: line.content, + expansionType: hunk.expansionType, + }) continue } diff --git a/app/src/ui/diff/text-diff-expansion.ts b/app/src/ui/diff/text-diff-expansion.ts index bd0018197e..f11fa3ae41 100644 --- a/app/src/ui/diff/text-diff-expansion.ts +++ b/app/src/ui/diff/text-diff-expansion.ts @@ -1,5 +1,7 @@ +import { enableTextDiffExpansion } from '../../lib/feature-flag' import { DiffHunk, + DiffHunkExpansionType, DiffHunkHeader, DiffLine, DiffLineType, @@ -12,24 +14,6 @@ export const DiffExpansionStep = 20 /** Type of expansion: could be up or down. */ export type ExpansionKind = 'up' | 'down' -/** Interface to represent the expansion capabilities of a hunk header. */ -interface IHunkHeaderExpansionInfo { - /** True if the hunk header can be expanded down. */ - readonly isExpandableDown: boolean - - /** True if the hunk header can be expanded up. */ - readonly isExpandableUp: boolean - - /** True if the hunk header can be expanded both up and down. */ - readonly isExpandableBoth: boolean - - /** - * True if the hunk header represents a short gap that, when expanded, will - * result in merging this hunk and the hunk above. - */ - readonly isExpandableShort: boolean -} - /** Builds the diff text string given a list of hunks. */ function getDiffTextFromHunks(hunks: ReadonlyArray) { // Grab all hunk lines and rebuild the diff text from it @@ -70,7 +54,18 @@ function mergeDiffHunks(hunk1: DiffHunk, hunk2: DiffHunk): DiffHunk { [newFirstHunkLine, ...allHunk1LinesButFirst, ...allHunk2LinesButFirst], hunk1.unifiedDiffStart, // This -1 represents the header line of the second hunk that we removed - hunk2.unifiedDiffEnd - 1 + hunk2.unifiedDiffEnd - 1, + // The expansion type of the resulting hunk will match the expansion type + // of the first hunk: + // - If the first hunk can be expanded up, it means it's the very first + // hunk, so the resulting hunk will be the first too. + // - If the first hunk can be expanded but short, that doesn't change after + // merging it with the second one. + // - If it can be expanded up and down (meaning it's a long gap), that + // doesn't change after merging it with the second one. + // - It can never be expanded down exclusively, because only the last dummy + // hunk can do that, and that will never be the first hunk in a merge. + hunk1.expansionType ) } @@ -79,24 +74,24 @@ function mergeDiffHunks(hunk1: DiffHunk, hunk2: DiffHunk): DiffHunk { * the space represented by the hunk header is short and expansion there would * mean merging with the hunk above. * - * @param hunks All hunks in the diff. - * @param hunk The specific hunk for which we want to know the expansion info. + * @param hunkIndex Index of the hunk to evaluate within the whole diff. + * @param hunkHeader Header of the hunk to evaluate. + * @param previousHunk Hunk previous to the one to evaluate. Null if the + * evaluated hunk is the first one. */ -export function getHunkHeaderExpansionInfo( - hunks: ReadonlyArray, - hunk: DiffHunk -): IHunkHeaderExpansionInfo { - let isExpandableDown = false - let isExpandableUp = false - let isExpandableBoth = false - let isExpandableShort = false +export function getHunkHeaderExpansionType( + hunkIndex: number, + hunkHeader: DiffHunkHeader, + previousHunk: DiffHunk | null +): DiffHunkExpansionType { + if (!enableTextDiffExpansion()) { + return DiffHunkExpansionType.None + } - const hunkIndex = hunks.indexOf(hunk) - const previousHunk = hunks[hunkIndex - 1] const distanceToPrevious = - previousHunk === undefined + previousHunk === null ? Infinity - : hunk.header.oldStartLine - + : hunkHeader.oldStartLine - previousHunk.header.oldStartLine - previousHunk.header.oldLineCount @@ -108,21 +103,15 @@ export function getHunkHeaderExpansionInfo( // short and therefore the direction of expansion doesn't matter. if (hunkIndex === 0) { // The top hunk can only be expanded if there is content above it - isExpandableUp = - hunk.header.oldStartLine > 1 && hunk.header.newStartLine > 1 - } else if (hunkIndex === hunks.length - 1 && hunk.lines.length === 1) { - isExpandableDown = true + if (hunkHeader.oldStartLine > 1 && hunkHeader.newStartLine > 1) { + return DiffHunkExpansionType.Up + } else { + return DiffHunkExpansionType.None + } } else if (distanceToPrevious <= DiffExpansionStep) { - isExpandableShort = true + return DiffHunkExpansionType.Short } else { - isExpandableBoth = true - } - - return { - isExpandableDown, - isExpandableUp, - isExpandableBoth, - isExpandableShort, + return DiffHunkExpansionType.Both } } @@ -270,12 +259,20 @@ export function expandTextDiffHunk( let numberOfNewDiffLines = updatedHunkLines.length - hunk.lines.length + const previousHunk = hunkIndex === 0 ? null : diff.hunks[hunkIndex - 1] + const expansionType = getHunkHeaderExpansionType( + hunkIndex, + newHunkHeader, + previousHunk + ) + // Update the hunk with all the new info (header, lines, start/end...) let updatedHunk = new DiffHunk( newHunkHeader, updatedHunkLines, hunk.unifiedDiffStart, - hunk.unifiedDiffEnd + numberOfNewDiffLines + hunk.unifiedDiffEnd + numberOfNewDiffLines, + expansionType ) let previousHunksEndIndex = 0 // Exclusive @@ -312,17 +309,33 @@ export function expandTextDiffHunk( const followingHunks = newHunkLastLine >= newContentLines.length ? [] - : diff.hunks - .slice(followingHunksStartIndex) - .map( - hunk => - new DiffHunk( - hunk.header, - hunk.lines, - hunk.unifiedDiffStart + numberOfNewDiffLines, - hunk.unifiedDiffEnd + numberOfNewDiffLines - ) + : diff.hunks.slice(followingHunksStartIndex).map((hunk, hunkIndex) => { + const isLastDummyHunk = + hunkIndex + followingHunksStartIndex === diff.hunks.length - 1 && + hunk.lines.length === 1 && + hunk.lines[0].type === DiffLineType.Hunk + + // Only compute the new expansion type if the hunk is the first one + // (of the remaining hunks) and it's not the last dummy hunk. + const shouldComputeNewExpansionType = + hunkIndex === 0 && !isLastDummyHunk + + return new DiffHunk( + hunk.header, + hunk.lines, + hunk.unifiedDiffStart + numberOfNewDiffLines, + hunk.unifiedDiffEnd + numberOfNewDiffLines, + // If it's the first hunk after the one we expanded, recalculate + // its expansion type. + shouldComputeNewExpansionType + ? getHunkHeaderExpansionType( + followingHunksStartIndex, + hunk.header, + updatedHunk + ) + : hunk.expansionType ) + }) // Create the new list of hunks of the diff, and the new diff text const newHunks = [...previousHunks, updatedHunk, ...followingHunks] @@ -386,7 +399,8 @@ export function getTextDiffWithBottomDummyHunk( dummyHeader, [dummyLine], lastHunk.unifiedDiffEnd + 1, - lastHunk.unifiedDiffEnd + 1 + lastHunk.unifiedDiffEnd + 1, + DiffHunkExpansionType.Down ) const newHunks = [...hunks, dummyHunk] diff --git a/app/src/ui/diff/text-diff.tsx b/app/src/ui/diff/text-diff.tsx index 084c9ed2fe..709502dc9f 100644 --- a/app/src/ui/diff/text-diff.tsx +++ b/app/src/ui/diff/text-diff.tsx @@ -8,6 +8,7 @@ import { DiffSelection, DiffLine, ITextDiff, + DiffHunkExpansionType, } from '../../models/diff' import { WorkingDirectoryFileChange, @@ -50,7 +51,6 @@ import { canSelect } from './diff-helpers' import { expandTextDiffHunk, ExpansionKind, - getHunkHeaderExpansionInfo, getTextDiffWithBottomDummyHunk, } from './text-diff-expansion' import { createOcticonElement } from '../octicons/octicon' @@ -338,6 +338,7 @@ export class TextDiff extends React.Component { /** The current, active, diff gutter selection if any */ private selection: ISelection | null = null + /** The content lines of the "new" file */ private newContentLines: ReadonlyArray | null = null /** Whether a particular range should be highlighted due to hover */ @@ -909,7 +910,7 @@ export class TextDiff extends React.Component { ) { const marker = lineInfo.gutterMarkers[diffGutterName] if (marker instanceof HTMLElement) { - this.updateGutterMarker(marker, hunks, hunk, diffLine) + this.updateGutterMarker(marker, hunk, diffLine) } } else { batchedOps.push(() => { @@ -949,7 +950,6 @@ export class TextDiff extends React.Component { } private getGutterLineClassNameInfo( - hunks: ReadonlyArray, hunk: DiffHunk, diffLine: DiffLine ): { [className: string]: boolean } { @@ -962,10 +962,6 @@ export class TextDiff extends React.Component { isIncludeable && diffLine.originalLineNumber !== null && inSelection(this.hunkHighlightRange, diffLine.originalLineNumber) - const hunkExpansionInfo = - enableTextDiffExpansion() && diffLine.type === DiffLineType.Hunk - ? getHunkHeaderExpansionInfo(hunks, hunk) - : undefined return { 'diff-line-gutter': true, @@ -976,10 +972,10 @@ export class TextDiff extends React.Component { 'read-only': this.props.readOnly, 'diff-line-selected': isIncluded, 'diff-line-hover': hover, - 'expandable-down': hunkExpansionInfo?.isExpandableDown === true, - 'expandable-up': hunkExpansionInfo?.isExpandableUp === true, - 'expandable-both': hunkExpansionInfo?.isExpandableBoth === true, - 'expandable-short': hunkExpansionInfo?.isExpandableShort === true, + 'expandable-down': hunk.expansionType === DiffHunkExpansionType.Down, + 'expandable-up': hunk.expansionType === DiffHunkExpansionType.Up, + 'expandable-both': hunk.expansionType === DiffHunkExpansionType.Both, + 'expandable-short': hunk.expansionType === DiffHunkExpansionType.Short, includeable: isIncludeable && !this.props.readOnly, } } @@ -1074,7 +1070,7 @@ export class TextDiff extends React.Component { ) } - this.updateGutterMarker(marker, hunks, hunk, diffLine) + this.updateGutterMarker(marker, hunk, diffLine) return marker } @@ -1157,11 +1153,10 @@ export class TextDiff extends React.Component { private updateGutterMarker( marker: HTMLElement, - hunks: ReadonlyArray, hunk: DiffHunk, diffLine: DiffLine ) { - const classNameInfo = this.getGutterLineClassNameInfo(hunks, hunk, diffLine) + const classNameInfo = this.getGutterLineClassNameInfo(hunk, diffLine) for (const [className, include] of Object.entries(classNameInfo)) { if (include) { marker.classList.add(className)