prettier diff (#30087)

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,18 +286,35 @@ 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);
 	}
 
 	/**
 	 * Computes the differences between the original and modified input
 	 * 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<DiffChange>();
+			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;
 		}
 	}
-
-}
+}

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

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');
 	});

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++) {

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) {

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);
+	});
 });