From d8e0a9907074ca06ef21220d840bae55e35fa1f1 Mon Sep 17 00:00:00 2001 From: Rodrigo Fernandes Date: Tue, 26 Nov 2019 16:07:53 +0000 Subject: [PATCH] refactor: Separate matching in line-by-line algorithm --- src/__tests__/diff2html-tests.ts | 26 +-- src/__tests__/line-by-line-tests.ts | 76 ++------- src/line-by-line-renderer.ts | 251 +++++++++++++--------------- src/render-utils.ts | 8 +- src/side-by-side-renderer.ts | 6 +- 5 files changed, 141 insertions(+), 226 deletions(-) diff --git a/src/__tests__/diff2html-tests.ts b/src/__tests__/diff2html-tests.ts index 6cdc6e7..1466537 100644 --- a/src/__tests__/diff2html-tests.ts +++ b/src/__tests__/diff2html-tests.ts @@ -89,23 +89,23 @@ const htmlLineExample1 = '
@@ -1 +1 @@
\n' + " \n" + "\n" + - ' \n' + + ' \n' + '
1
\n' + '
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' -\n' + ' test\n' + "
\n" + " \n" + "\n" + - ' \n' + + ' \n' + '
\n' + '
1
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' +\n' + ' test1\n' + "
\n" + @@ -446,23 +446,23 @@ describe("Diff2Html", () => { "
\n" + " \n" + "\n" + - ' \n' + + ' \n' + '
14
\n' + '
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' -\n' + ' - Fix HEAD branch order when redraw [#858](https://github.com/FredrikNoren/ungit/issues/858)\n' + "
\n" + " \n" + "\n" + - ' \n' + + ' \n' + '
\n' + '
13
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' +\n' + ' 4 - Fix HEAD branch order when redraw [#858](https://github.com/FredrikNoren/ungit/issues/858)\n' + "
\n" + @@ -519,7 +519,7 @@ describe("Diff2Html", () => { "
"; const result = html(diffExample2, { drawFileList: false }); - expect(htmlExample2).toEqual(result); + expect(result).toEqual(htmlExample2); }); }); }); diff --git a/src/__tests__/line-by-line-tests.ts b/src/__tests__/line-by-line-tests.ts index 6260306..c641eb4 100644 --- a/src/__tests__/line-by-line-tests.ts +++ b/src/__tests__/line-by-line-tests.ts @@ -1,6 +1,6 @@ import LineByLineRenderer from "../line-by-line-renderer"; import HoganJsUtils from "../hoganjs-utils"; -import { LineType, DiffLine, DiffFile, LineMatchingType } from "../types"; +import { LineType, DiffFile, LineMatchingType } from "../types"; import { CSSLineClass } from "../render-utils"; describe("LineByLineRenderer", () => { @@ -26,7 +26,7 @@ describe("LineByLineRenderer", () => { it("should work for insertions", () => { const hoganUtils = new HoganJsUtils({}); const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - let fileHtml = lineByLineRenderer.makeLineHtml(false, CSSLineClass.INSERTS, "test", undefined, 30, "+"); + let fileHtml = lineByLineRenderer.makeLineHtml(CSSLineClass.INSERTS, "+", "test", undefined, 30); fileHtml = fileHtml.replace(/\n\n+/g, "\n"); const expected = "\n" + @@ -48,7 +48,7 @@ describe("LineByLineRenderer", () => { it("should work for deletions", () => { const hoganUtils = new HoganJsUtils({}); const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - let fileHtml = lineByLineRenderer.makeLineHtml(false, CSSLineClass.DELETES, "test", 30, undefined, "-"); + let fileHtml = lineByLineRenderer.makeLineHtml(CSSLineClass.DELETES, "-", "test", 30, undefined); fileHtml = fileHtml.replace(/\n\n+/g, "\n"); const expected = "\n" + @@ -70,7 +70,7 @@ describe("LineByLineRenderer", () => { it("should convert indents into non breakin spaces (2 white spaces)", () => { const hoganUtils = new HoganJsUtils({}); const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - let fileHtml = lineByLineRenderer.makeLineHtml(false, CSSLineClass.INSERTS, " test", undefined, 30, "+"); + let fileHtml = lineByLineRenderer.makeLineHtml(CSSLineClass.INSERTS, "+", " test", undefined, 30); fileHtml = fileHtml.replace(/\n\n+/g, "\n"); const expected = "\n" + @@ -92,7 +92,7 @@ describe("LineByLineRenderer", () => { it("should convert indents into non breakin spaces (4 white spaces)", () => { const hoganUtils = new HoganJsUtils({}); const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - let fileHtml = lineByLineRenderer.makeLineHtml(false, CSSLineClass.INSERTS, " test", undefined, 30, "+"); + let fileHtml = lineByLineRenderer.makeLineHtml(CSSLineClass.INSERTS, "+", " test", undefined, 30); fileHtml = fileHtml.replace(/\n\n+/g, "\n"); const expected = "\n" + @@ -114,7 +114,7 @@ describe("LineByLineRenderer", () => { it("should preserve tabs", () => { const hoganUtils = new HoganJsUtils({}); const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - let fileHtml = lineByLineRenderer.makeLineHtml(false, CSSLineClass.INSERTS, "\ttest", undefined, 30, "+"); + let fileHtml = lineByLineRenderer.makeLineHtml(CSSLineClass.INSERTS, "+", "\ttest", undefined, 30); fileHtml = fileHtml.replace(/\n\n+/g, "\n"); const expected = "\n" + @@ -473,58 +473,6 @@ describe("LineByLineRenderer", () => { }); }); - describe("_processLines", () => { - it("should work for simple block header", () => { - const hoganUtils = new HoganJsUtils({}); - const lineByLineRenderer = new LineByLineRenderer(hoganUtils, {}); - const oldLines: DiffLine[] = [ - { - content: "-test", - type: LineType.DELETE, - oldNumber: 1, - newNumber: undefined - } - ]; - const newLines: DiffLine[] = [ - { - content: "+test1r", - type: LineType.INSERT, - oldNumber: undefined, - newNumber: 1 - } - ]; - - const html = lineByLineRenderer.processLines(false, oldLines, newLines); - - const expected = - "\n" + - ' \n' + - '
1
\n' + - '
\n' + - " \n" + - ' \n' + - '
\n' + - ' -\n' + - ' test\n' + - "
\n" + - " \n" + - "\n" + - ' \n' + - '
\n' + - '
1
\n' + - " \n" + - ' \n' + - '
\n' + - ' +\n' + - ' test1r\n' + - "
\n" + - " \n" + - ""; - - expect(html).toEqual(expected); - }); - }); - describe("_generateFileHtml", () => { it("should work for simple file", () => { const hoganUtils = new HoganJsUtils({}); @@ -595,23 +543,23 @@ describe("LineByLineRenderer", () => { "
\n" + " \n" + "\n" + - ' \n' + + ' \n' + '
2
\n' + '
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' -\n' + ' test\n' + "
\n" + " \n" + "\n" + - ' \n' + + ' \n' + '
\n' + '
2
\n' + " \n" + - ' \n' + - '
\n' + + ' \n' + + '
\n' + ' +\n' + ' test1r\n' + "
\n" + diff --git a/src/line-by-line-renderer.ts b/src/line-by-line-renderer.ts index 2e641b6..8a011b4 100644 --- a/src/line-by-line-renderer.ts +++ b/src/line-by-line-renderer.ts @@ -1,7 +1,7 @@ import HoganJsUtils from "./hoganjs-utils"; import * as Rematch from "./rematch"; import * as renderUtils from "./render-utils"; -import { DiffFile, DiffLine, LineType } from "./types"; +import { DiffFile, DiffLine, LineType, DiffBlock } from "./types"; export interface LineByLineRendererConfig extends renderUtils.RenderConfig { renderNothingWhenEmpty?: boolean; @@ -79,7 +79,6 @@ export default class LineByLineRenderer { }); } - // TODO: Make this private after improving tests generateFileHtml(file: DiffFile): string { const matcher = Rematch.newMatcherFn( Rematch.newDistanceFn((e: DiffLine) => renderUtils.deconstructLine(e.content, file.isCombined).content) @@ -93,58 +92,79 @@ export default class LineByLineRenderer { lineClass: "d2h-code-linenumber", contentClass: "d2h-code-line" }); - let oldLines: DiffLine[] = []; - let newLines: DiffLine[] = []; - for (let i = 0; i < block.lines.length; i++) { - const diffLine = block.lines[i]; - const { prefix, content } = renderUtils.deconstructLine(diffLine.content, file.isCombined); - - if ( - diffLine.type !== LineType.INSERT && - (newLines.length > 0 || (diffLine.type !== LineType.DELETE && oldLines.length > 0)) - ) { - lines += this.processChangeBlock(file, oldLines, newLines, matcher); - oldLines = []; - newLines = []; - } - - if (diffLine.type === LineType.CONTEXT || (diffLine.type === LineType.INSERT && !oldLines.length)) { - lines += this.makeLineHtml( - file.isCombined, - renderUtils.toCSSClass(diffLine.type), - content, - diffLine.oldNumber, - diffLine.newNumber, - prefix - ); - } else if (diffLine.type === LineType.DELETE) { - oldLines.push(diffLine); - } else if (diffLine.type === LineType.INSERT && Boolean(oldLines.length)) { - newLines.push(diffLine); + this.applyLineGroupping(block).forEach(([contextLines, oldLines, newLines]) => { + if (oldLines.length && newLines.length && !contextLines.length) { + lines += this.applyRematchMatching(oldLines, newLines, matcher) + .map(([oldLines, newLines]) => this.applyLineDiff(file, oldLines, newLines)) + .join(""); + } else if (oldLines.length || newLines.length || contextLines.length) { + lines += (contextLines || []) + .concat((oldLines || []).concat(newLines || [])) + .map(line => { + const { prefix, content } = renderUtils.deconstructLine(line.content, file.isCombined); + return this.makeLineHtml( + renderUtils.toCSSClass(line.type), + prefix, + content, + line.oldNumber, + line.newNumber + ); + }) + .join(""); } else { - console.error("Unknown state in html line-by-line generator"); - lines += this.processChangeBlock(file, oldLines, newLines, matcher); - oldLines = []; - newLines = []; + console.error("Unknown state reached while processing groups of lines", contextLines, oldLines, newLines); } - } - - lines += this.processChangeBlock(file, oldLines, newLines, matcher); - oldLines = []; - newLines = []; + }); return lines; }) .join("\n"); } - processChangeBlock( - file: DiffFile, + applyLineGroupping(block: DiffBlock): DiffLine[][][] { + const blockLinesGroups: DiffLine[][][] = []; + + let oldLines: DiffLine[] = []; + let newLines: DiffLine[] = []; + + for (let i = 0; i < block.lines.length; i++) { + const diffLine = block.lines[i]; + + if ( + (diffLine.type !== LineType.INSERT && newLines.length) || + (diffLine.type === LineType.CONTEXT && oldLines.length > 0) + ) { + blockLinesGroups.push([[], oldLines, newLines]); + oldLines = []; + newLines = []; + } + + if (diffLine.type === LineType.CONTEXT) { + blockLinesGroups.push([[diffLine], [], []]); + } else if (diffLine.type === LineType.INSERT && oldLines.length === 0) { + blockLinesGroups.push([[], [], [diffLine]]); + } else if (diffLine.type === LineType.INSERT && oldLines.length > 0) { + newLines.push(diffLine); + } else if (diffLine.type === LineType.DELETE) { + oldLines.push(diffLine); + } + } + + if (oldLines.length || newLines.length) { + blockLinesGroups.push([[], oldLines, newLines]); + oldLines = []; + newLines = []; + } + + return blockLinesGroups; + } + + applyRematchMatching( oldLines: DiffLine[], newLines: DiffLine[], matcher: Rematch.MatcherFn - ): string { + ): DiffLine[][][] { const comparisons = oldLines.length * newLines.length; const maxLineSizeInBlock = Math.max.apply( null, @@ -155,115 +175,72 @@ export default class LineByLineRenderer { maxLineSizeInBlock < this.config.maxLineSizeInBlockForComparison && (this.config.matching === "lines" || this.config.matching === "words"); - const [matches, insertType, deleteType] = doMatching - ? [matcher(oldLines, newLines), renderUtils.CSSLineClass.INSERT_CHANGES, renderUtils.CSSLineClass.DELETE_CHANGES] - : [[[oldLines, newLines]], renderUtils.CSSLineClass.INSERTS, renderUtils.CSSLineClass.DELETES]; + const matches = doMatching ? matcher(oldLines, newLines) : [[oldLines, newLines]]; - let lines = ""; - matches.forEach(match => { - oldLines = match[0]; - newLines = match[1]; - - let processedOldLines = ""; - let processedNewLines = ""; - - const common = Math.min(oldLines.length, newLines.length); - - let oldLine, newLine; - for (let j = 0; j < common; j++) { - oldLine = oldLines[j]; - newLine = newLines[j]; - - const diff = renderUtils.diffHighlight(oldLine.content, newLine.content, file.isCombined, this.config); - - processedOldLines += this.makeLineHtml( - file.isCombined, - deleteType, - diff.oldLine.content, - oldLine.oldNumber, - oldLine.newNumber, - diff.oldLine.prefix - ); - processedNewLines += this.makeLineHtml( - file.isCombined, - insertType, - diff.newLine.content, - newLine.oldNumber, - newLine.newNumber, - diff.newLine.prefix - ); - } - - lines += processedOldLines + processedNewLines; - lines += this.processLines(file.isCombined, oldLines.slice(common), newLines.slice(common)); - }); - - return lines; + return matches; } - // TODO: Make this private after improving tests - makeLineHtml( - isCombined: boolean, - type: renderUtils.CSSLineClass, - content: string, - oldNumber?: number, - newNumber?: number, - possiblePrefix?: string - ): string { - const lineNumberTemplate = this.hoganUtils.render(baseTemplatesPath, "numbers", { - oldNumber: oldNumber || "", - newNumber: newNumber || "" - }); + applyLineDiff(file: DiffFile, oldLines: DiffLine[], newLines: DiffLine[]): string { + let oldLinesHtml = ""; + let newLinesHtml = ""; - let lineWithoutPrefix = content; - let prefix = possiblePrefix; + const common = Math.min(oldLines.length, newLines.length); - if (!prefix) { - const lineWithPrefix = renderUtils.deconstructLine(content, isCombined); - prefix = lineWithPrefix.prefix; - lineWithoutPrefix = lineWithPrefix.content; - } + let oldLine, newLine; + for (let j = 0; j < common; j++) { + oldLine = oldLines[j]; + newLine = newLines[j]; - if (prefix === " ") { - prefix = " "; - } + const diff = renderUtils.diffHighlight(oldLine.content, newLine.content, file.isCombined, this.config); - return this.hoganUtils.render(genericTemplatesPath, "line", { - type: type, - lineClass: "d2h-code-linenumber", - contentClass: "d2h-code-line", - prefix: prefix, - content: lineWithoutPrefix, - lineNumber: lineNumberTemplate - }); - } - - // TODO: Make this private after improving tests - processLines(isCombined: boolean, oldLines: DiffLine[], newLines: DiffLine[]): string { - let lines = ""; - - for (let i = 0; i < oldLines.length; i++) { - const oldLine = oldLines[i]; - lines += this.makeLineHtml( - isCombined, - renderUtils.toCSSClass(oldLine.type), - oldLine.content, + oldLinesHtml += this.makeLineHtml( + renderUtils.CSSLineClass.DELETE_CHANGES, + diff.oldLine.prefix, + diff.oldLine.content, oldLine.oldNumber, oldLine.newNumber ); - } - - for (let j = 0; j < newLines.length; j++) { - const newLine = newLines[j]; - lines += this.makeLineHtml( - isCombined, - renderUtils.toCSSClass(newLine.type), - newLine.content, + newLinesHtml += this.makeLineHtml( + renderUtils.CSSLineClass.INSERT_CHANGES, + diff.newLine.prefix, + diff.newLine.content, newLine.oldNumber, newLine.newNumber ); } - return lines; + const remainingLines = oldLines + .slice(common) + .concat(newLines.slice(common)) + .map(line => { + const { prefix, content } = renderUtils.deconstructLine(line.content, file.isCombined); + return this.makeLineHtml(renderUtils.toCSSClass(line.type), prefix, content, line.oldNumber, line.newNumber); + }) + .join(""); + + return oldLinesHtml + newLinesHtml + remainingLines; + } + + // TODO: Make this private after improving tests + makeLineHtml( + type: renderUtils.CSSLineClass, + prefix: string, + content: string, + oldNumber?: number, + newNumber?: number + ): string { + const lineNumberHtml = this.hoganUtils.render(baseTemplatesPath, "numbers", { + oldNumber: oldNumber || "", + newNumber: newNumber || "" + }); + + return this.hoganUtils.render(genericTemplatesPath, "line", { + type: type, + lineClass: "d2h-code-linenumber", + contentClass: "d2h-code-line", + prefix: prefix === " " ? " " : prefix, + content: content, + lineNumber: lineNumberHtml + }); } } diff --git a/src/render-utils.ts b/src/render-utils.ts index 740f7b2..b32317b 100644 --- a/src/render-utils.ts +++ b/src/render-utils.ts @@ -240,19 +240,13 @@ export function diffHighlight( const changedWords: jsDiff.Change[] = []; if (diffStyle === "word" && matching === "words") { - let treshold = 0.25; - - if (typeof matchWordsThreshold !== "undefined") { - treshold = matchWordsThreshold; - } - const removed = diff.filter(element => element.removed); const added = diff.filter(element => element.added); const chunks = matcher(added, removed); chunks.forEach(chunk => { if (chunk[0].length === 1 && chunk[1].length === 1) { const dist = distance(chunk[0][0], chunk[1][0]); - if (dist < treshold) { + if (dist < matchWordsThreshold) { changedWords.push(chunk[0][0]); changedWords.push(chunk[1][0]); } diff --git a/src/side-by-side-renderer.ts b/src/side-by-side-renderer.ts index cd561e5..4f7ee63 100644 --- a/src/side-by-side-renderer.ts +++ b/src/side-by-side-renderer.ts @@ -331,15 +331,11 @@ export default class SideBySideRenderer { lineWithoutPrefix = lineWithPrefix.content; } - if (prefix === " ") { - prefix = " "; - } - return this.hoganUtils.render(genericTemplatesPath, "line", { type: preparedType, lineClass: lineClass, contentClass: contentClass, - prefix: prefix, + prefix: prefix === " " ? " " : prefix, content: lineWithoutPrefix, lineNumber: number });