diff --git a/src/vs/base/common/diff/diff.ts b/src/vs/base/common/diff/diff.ts index 3b402e44a1d..c86a872c16b 100644 --- a/src/vs/base/common/diff/diff.ts +++ b/src/vs/base/common/diff/diff.ts @@ -15,8 +15,8 @@ function createStringSequence(a: string): ISequence { }; } -export function stringDiff(original: string, modified: string): IDiffChange[] { - return new LcsDiff(createStringSequence(original), createStringSequence(modified)).ComputeDiff(); +export function stringDiff(original: string, modified: string, pretty: boolean): IDiffChange[] { + return new LcsDiff(createStringSequence(original), createStringSequence(modified)).ComputeDiff(pretty); } @@ -286,8 +286,16 @@ export class LcsDiff { return this.m_originalIds[originalIndex] === this.m_modifiedIds[newIndex]; } - public ComputeDiff(): IDiffChange[] { - return this._ComputeDiff(0, this.OriginalSequence.getLength() - 1, 0, this.ModifiedSequence.getLength() - 1); + private OriginalElementsAreEqual(index1: number, index2: number): boolean { + return this.m_originalIds[index1] === this.m_originalIds[index2]; + } + + private ModifiedElementsAreEqual(index1: number, index2: number): boolean { + return this.m_modifiedIds[index1] === this.m_modifiedIds[index2]; + } + + public ComputeDiff(pretty: boolean): IDiffChange[] { + return this._ComputeDiff(0, this.OriginalSequence.getLength() - 1, 0, this.ModifiedSequence.getLength() - 1, pretty); } /** @@ -295,9 +303,18 @@ export class LcsDiff { * sequences on the bounded range. * @returns An array of the differences between the two input sequences. */ - private _ComputeDiff(originalStart: number, originalEnd: number, modifiedStart: number, modifiedEnd: number): DiffChange[] { + private _ComputeDiff(originalStart: number, originalEnd: number, modifiedStart: number, modifiedEnd: number, pretty: boolean): DiffChange[] { let quitEarlyArr = [false]; - return this.ComputeDiffRecursive(originalStart, originalEnd, modifiedStart, modifiedEnd, quitEarlyArr); + let changes = this.ComputeDiffRecursive(originalStart, originalEnd, modifiedStart, modifiedEnd, quitEarlyArr); + + if (pretty) { + // We have to clean up the computed diff to be more intuitive + // but it turns out this cannot be done correctly until the entire set + // of diffs have been computed + return this.ShiftChanges(changes); + } + + return changes; } /** @@ -769,6 +786,57 @@ export class LcsDiff { ); } + /** + * Shifts the given changes to provide a more intuitive diff. + * While the first element in a diff matches the first element after the diff, + * we shift the diff down. + * + * @param changes The list of changes to shift + * @returns The shifted changes + */ + private ShiftChanges(changes: DiffChange[]): DiffChange[] { + let mergedDiffs: boolean; + do { + mergedDiffs = false; + + // Shift all the changes first + for (let i = 0; i < changes.length; i++) { + const change = changes[i]; + const originalStop = (i < changes.length - 1) ? changes[i + 1].originalStart : this.OriginalSequence.getLength(); + const modifiedStop = (i < changes.length - 1) ? changes[i + 1].modifiedStart : this.ModifiedSequence.getLength(); + const checkOriginal = change.originalLength > 0; + const checkModified = change.modifiedLength > 0; + + while (change.originalStart + change.originalLength < originalStop && + change.modifiedStart + change.modifiedLength < modifiedStop && + (!checkOriginal || this.OriginalElementsAreEqual(change.originalStart, change.originalStart + change.originalLength)) && + (!checkModified || this.ModifiedElementsAreEqual(change.modifiedStart, change.modifiedStart + change.modifiedLength))) { + change.originalStart++; + change.modifiedStart++; + } + } + + // Build up the new list (we have to build a new list because we + // might have changes we can merge together now) + let result = new Array(); + let mergedChangeArr: DiffChange[] = [null]; + for (let i = 0; i < changes.length; i++) { + if (i < changes.length - 1 && this.ChangesOverlap(changes[i], changes[i + 1], mergedChangeArr)) { + mergedDiffs = true; + result.push(mergedChangeArr[0]); + i++; + } + else { + result.push(changes[i]); + } + } + + changes = result; + } while (mergedDiffs); + + return changes; + } + /** * Concatenates the two input DiffChange lists and returns the resulting * list. @@ -847,7 +915,6 @@ export class LcsDiff { * @param numDiagonals The total number of diagonals. * @returns The clipped diagonal index. */ - private ClipDiagonalBound(diagonal: number, numDifferences: number, diagonalBaseIndex: number, numDiagonals: number): number { if (diagonal >= 0 && diagonal < numDiagonals) { // Nothing to clip, its in range @@ -868,5 +935,4 @@ export class LcsDiff { return (diffEven === upperBoundEven) ? numDiagonals - 1 : numDiagonals - 2; } } - -} \ No newline at end of file +} diff --git a/src/vs/base/parts/tree/browser/treeView.ts b/src/vs/base/parts/tree/browser/treeView.ts index 02a5ce62343..115c07c566c 100644 --- a/src/vs/base/parts/tree/browser/treeView.ts +++ b/src/vs/base/parts/tree/browser/treeView.ts @@ -955,7 +955,7 @@ export class TreeView extends HeightMap { getElementHash: (i: number) => afterModelItems[i].id }, null); - diff = lcs.ComputeDiff(); + diff = lcs.ComputeDiff(false); // this means that the result of the diff algorithm would result // in inserting items that were already registered. this can only diff --git a/src/vs/base/test/common/diff/diff.test.ts b/src/vs/base/test/common/diff/diff.test.ts index b5467da27fc..a822586e7c4 100644 --- a/src/vs/base/test/common/diff/diff.test.ts +++ b/src/vs/base/test/common/diff/diff.test.ts @@ -143,7 +143,7 @@ suite('Diff - Ported from VS', () => { // cancel processing return false; }); - var changes = diff.ComputeDiff(); + var changes = diff.ComputeDiff(true); assert.equal(predicateCallCount, 1); @@ -159,7 +159,7 @@ suite('Diff - Ported from VS', () => { // Continue processing as long as there hasn't been a match made. return longestMatchSoFar < 1; }); - changes = diff.ComputeDiff(); + changes = diff.ComputeDiff(true); assertAnswer(left, right, changes, 'abcf'); @@ -172,7 +172,7 @@ suite('Diff - Ported from VS', () => { // Continue processing as long as there hasn't been a match made. return longestMatchSoFar < 2; }); - changes = diff.ComputeDiff(); + changes = diff.ComputeDiff(true); assertAnswer(left, right, changes, 'abcdf'); @@ -188,7 +188,7 @@ suite('Diff - Ported from VS', () => { // Continue processing as long as there hasn't been a match made. return !hitYet; }); - changes = diff.ComputeDiff(); + changes = diff.ComputeDiff(true); assertAnswer(left, right, changes, 'abcdf'); @@ -201,7 +201,7 @@ suite('Diff - Ported from VS', () => { // Continue processing as long as there hasn't been a match made. return longestMatchSoFar < 3; }); - changes = diff.ComputeDiff(); + changes = diff.ComputeDiff(true); assertAnswer(left, right, changes, 'abcdef'); }); diff --git a/src/vs/editor/common/diff/diffComputer.ts b/src/vs/editor/common/diff/diffComputer.ts index 4ddccc4eaa2..9ee7f6b8153 100644 --- a/src/vs/editor/common/diff/diffComputer.ts +++ b/src/vs/editor/common/diff/diffComputer.ts @@ -17,9 +17,9 @@ interface IMarker { offset: number; } -function computeDiff(originalSequence: ISequence, modifiedSequence: ISequence, continueProcessingPredicate: () => boolean): IDiffChange[] { +function computeDiff(originalSequence: ISequence, modifiedSequence: ISequence, continueProcessingPredicate: () => boolean, pretty: boolean): IDiffChange[] { var diffAlgo = new LcsDiff(originalSequence, modifiedSequence, continueProcessingPredicate); - return diffAlgo.ComputeDiff(); + return diffAlgo.ComputeDiff(pretty); } class MarkerSequence implements ISequence { @@ -252,7 +252,7 @@ class LineChange implements ILineChange { var originalCharSequence = originalLineSequence.getCharSequence(diffChange.originalStart, diffChange.originalStart + diffChange.originalLength - 1); var modifiedCharSequence = modifiedLineSequence.getCharSequence(diffChange.modifiedStart, diffChange.modifiedStart + diffChange.modifiedLength - 1); - var rawChanges = computeDiff(originalCharSequence, modifiedCharSequence, continueProcessingPredicate); + var rawChanges = computeDiff(originalCharSequence, modifiedCharSequence, continueProcessingPredicate, false); if (shouldPostProcessCharChanges) { rawChanges = postProcessCharChanges(rawChanges); @@ -271,12 +271,14 @@ export interface IDiffComputerOpts { shouldPostProcessCharChanges: boolean; shouldIgnoreTrimWhitespace: boolean; shouldConsiderTrimWhitespaceInEmptyCase: boolean; + shouldMakePrettyDiff: boolean; } export class DiffComputer { private shouldPostProcessCharChanges: boolean; private shouldIgnoreTrimWhitespace: boolean; + private shouldMakePrettyDiff: boolean; private maximumRunTimeMs: number; private original: LineMarkerSequence; private modified: LineMarkerSequence; @@ -286,6 +288,7 @@ export class DiffComputer { constructor(originalLines: string[], modifiedLines: string[], opts: IDiffComputerOpts) { this.shouldPostProcessCharChanges = opts.shouldPostProcessCharChanges; this.shouldIgnoreTrimWhitespace = opts.shouldIgnoreTrimWhitespace; + this.shouldMakePrettyDiff = opts.shouldMakePrettyDiff; this.maximumRunTimeMs = MAXIMUM_RUN_TIME; this.original = new LineMarkerSequence(originalLines, this.shouldIgnoreTrimWhitespace); this.modified = new LineMarkerSequence(modifiedLines, this.shouldIgnoreTrimWhitespace); @@ -341,7 +344,7 @@ export class DiffComputer { this.computationStartTime = (new Date()).getTime(); - var rawChanges = computeDiff(this.original, this.modified, this._continueProcessingPredicate.bind(this)); + var rawChanges = computeDiff(this.original, this.modified, this._continueProcessingPredicate.bind(this), this.shouldMakePrettyDiff); var lineChanges: ILineChange[] = []; for (var i = 0, length = rawChanges.length; i < length; i++) { diff --git a/src/vs/editor/common/services/editorSimpleWorker.ts b/src/vs/editor/common/services/editorSimpleWorker.ts index 5d36b8f6c4b..5654bd38526 100644 --- a/src/vs/editor/common/services/editorSimpleWorker.ts +++ b/src/vs/editor/common/services/editorSimpleWorker.ts @@ -313,7 +313,8 @@ export abstract class BaseEditorSimpleWorker { let diffComputer = new DiffComputer(originalLines, modifiedLines, { shouldPostProcessCharChanges: true, shouldIgnoreTrimWhitespace: ignoreTrimWhitespace, - shouldConsiderTrimWhitespaceInEmptyCase: true + shouldConsiderTrimWhitespaceInEmptyCase: true, + shouldMakePrettyDiff: true }); return TPromise.as(diffComputer.computeDiff()); } @@ -330,7 +331,8 @@ export abstract class BaseEditorSimpleWorker { let diffComputer = new DiffComputer(originalLines, modifiedLines, { shouldPostProcessCharChanges: false, shouldIgnoreTrimWhitespace: ignoreTrimWhitespace, - shouldConsiderTrimWhitespaceInEmptyCase: false + shouldConsiderTrimWhitespaceInEmptyCase: false, + shouldMakePrettyDiff: true }); return TPromise.as(diffComputer.computeDiff()); } @@ -377,7 +379,7 @@ export abstract class BaseEditorSimpleWorker { } // compute diff between original and edit.text - const changes = stringDiff(original, text); + const changes = stringDiff(original, text, false); const editOffset = model.offsetAt(Range.lift(range).getStartPosition()); for (const change of changes) { diff --git a/src/vs/editor/test/common/diff/diffComputer.test.ts b/src/vs/editor/test/common/diff/diffComputer.test.ts index 3bb149dcf53..412442db7f3 100644 --- a/src/vs/editor/test/common/diff/diffComputer.test.ts +++ b/src/vs/editor/test/common/diff/diffComputer.test.ts @@ -55,7 +55,8 @@ function assertDiff(originalLines: string[], modifiedLines: string[], expectedCh var diffComputer = new DiffComputer(originalLines, modifiedLines, { shouldPostProcessCharChanges: shouldPostProcessCharChanges || false, shouldIgnoreTrimWhitespace: shouldIgnoreTrimWhitespace || false, - shouldConsiderTrimWhitespaceInEmptyCase: true + shouldConsiderTrimWhitespaceInEmptyCase: true, + shouldMakePrettyDiff: true }); var changes = diffComputer.computeDiff(); @@ -458,4 +459,84 @@ suite('Editor Diff - DiffComputer', () => { ]; assertDiff(original, modified, expected, false, true); }); + + test('pretty diff 1', () => { + var original = [ + 'suite(function () {', + ' test1() {', + ' assert.ok(true);', + ' }', + '', + ' test2() {', + ' assert.ok(true);', + ' }', + '});', + '', + ]; + var modified = [ + '// An insertion', + 'suite(function () {', + ' test1() {', + ' assert.ok(true);', + ' }', + '', + ' test2() {', + ' assert.ok(true);', + ' }', + '', + ' test3() {', + ' assert.ok(true);', + ' }', + '});', + '', + ]; + var expected = [ + createLineInsertion(1, 1, 0), + createLineInsertion(10, 13, 8) + ]; + assertDiff(original, modified, expected, false, true); + }); + + test('pretty diff 2', () => { + var original = [ + '// Just a comment', + '', + 'function compute(a, b, c, d) {', + ' if (a) {', + ' if (b) {', + ' if (c) {', + ' return 5;', + ' }', + ' }', + ' // These next lines will be deleted', + ' if (d) {', + ' return -1;', + ' }', + ' return 0;', + ' }', + '}', + ]; + var modified = [ + '// Here is an inserted line', + '// and another inserted line', + '// and another one', + '// Just a comment', + '', + 'function compute(a, b, c, d) {', + ' if (a) {', + ' if (b) {', + ' if (c) {', + ' return 5;', + ' }', + ' }', + ' return 0;', + ' }', + '}', + ]; + var expected = [ + createLineInsertion(1, 3, 0), + createLineDeletion(10, 13, 12), + ]; + assertDiff(original, modified, expected, false, true); + }); });