Skip to content

Commit 2413a13

Browse files
authored
Fix sequence diff alignment (#493)
* Fix sequence diff alignment * Align sequence diff with Toolbox
1 parent 7a0c2fd commit 2413a13

File tree

3 files changed

+130
-9
lines changed

3 files changed

+130
-9
lines changed

src/model/check.ts

Lines changed: 87 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -581,13 +581,7 @@ function findSetChanges(prev: SetValue, state: SetValue): boolean {
581581
/**
582582
* Recursively finds and marks all the changes between two collections.
583583
*/
584-
export function findChanges(prev: CollectionValue, state: CollectionValue): boolean {
585-
// Special handling for sets - they are unordered collections
586-
if (prev instanceof SetValue && state instanceof SetValue) {
587-
return findSetChanges(prev, state);
588-
}
589-
590-
// For other collections (sequences, structures), use ordered comparison
584+
function findOrderedChanges(prev: CollectionValue, state: CollectionValue): boolean {
591585
let pi = 0;
592586
let si = 0;
593587
let modified = false;
@@ -630,6 +624,92 @@ export function findChanges(prev: CollectionValue, state: CollectionValue): bool
630624
return modified;
631625
}
632626

627+
function findSequenceChanges(prev: SequenceValue, state: SequenceValue): boolean {
628+
const prevItems = prev.items;
629+
const stateItems = state.items;
630+
if (prevItems.length === stateItems.length) {
631+
return findOrderedChanges(prev, state);
632+
}
633+
634+
let shorter = prevItems;
635+
let longer = stateItems;
636+
let firstShorter = true;
637+
if (prevItems.length > stateItems.length) {
638+
shorter = stateItems;
639+
longer = prevItems;
640+
firstShorter = false;
641+
}
642+
643+
let isPrefix = true;
644+
for (let i = 0; i < shorter.length; i++) {
645+
if (shorter[i].str !== longer[i].str) {
646+
isPrefix = false;
647+
break;
648+
}
649+
}
650+
651+
let isSuffix = true;
652+
for (let i = 0; i < shorter.length; i++) {
653+
if (shorter[i].str !== longer[i + longer.length - shorter.length].str) {
654+
isSuffix = false;
655+
break;
656+
}
657+
}
658+
659+
if (isPrefix && isSuffix) {
660+
if (firstShorter) {
661+
isSuffix = false;
662+
} else {
663+
isPrefix = false;
664+
}
665+
} else if (!(isPrefix || isSuffix)) {
666+
return findOrderedChanges(prev, state);
667+
}
668+
669+
let modified = false;
670+
const firstEltToMark = isPrefix ? shorter.length : 0;
671+
const diffCount = longer.length - shorter.length;
672+
if (firstShorter) {
673+
for (let i = 0; i < diffCount; i++) {
674+
stateItems[firstEltToMark + i].changeType = Change.ADDED;
675+
modified = true;
676+
}
677+
} else {
678+
const deletedItems: Value[] = [];
679+
for (let i = 0; i < diffCount; i++) {
680+
deletedItems.push(prevItems[firstEltToMark + i]);
681+
}
682+
if (deletedItems.length > 0) {
683+
state.addDeletedItems(deletedItems);
684+
modified = true;
685+
}
686+
}
687+
if (modified) {
688+
state.changeType = Change.MODIFIED;
689+
}
690+
return modified;
691+
}
692+
693+
/**
694+
* Recursively finds and marks all the changes between two collections.
695+
*/
696+
export function findChanges(prev: CollectionValue, state: CollectionValue): boolean {
697+
// Special handling for sets - they are unordered collections
698+
if (prev instanceof SetValue && state instanceof SetValue) {
699+
return findSetChanges(prev, state);
700+
}
701+
702+
if (prev instanceof SequenceValue && state instanceof SequenceValue) {
703+
if (prev.items.length === state.items.length) {
704+
return findOrderedChanges(prev, state);
705+
}
706+
return findSequenceChanges(prev, state);
707+
}
708+
709+
// For other collections (structures, functions), use ordered comparison
710+
return findOrderedChanges(prev, state);
711+
}
712+
633713
function dateTimeToStr(dateTime: Moment | undefined): string {
634714
if (!dateTime) {
635715
return 'not yet';

src/webview/checkResultView/errorTraceSection/errorTraceVariable.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,9 @@ export const ErrorTraceVariable = React.memo(({ value, stateId, settings }: Erro
5050
const changeHintKey = value.changeType as keyof typeof changeHints;
5151
const changeHint = changeHints[changeHintKey] ?? '';
5252
const changeTypeClass = 'value-' + value.changeType;
53+
const isSequence = value.prefix === '<<' && value.postfix === '>>';
5354
const hasValueChildren = hasVariableChildrenToDisplay(value);
54-
const hasDeletedChildren = Array.isArray(value.deletedItems) && value.deletedItems.length > 0;
55+
const hasDeletedChildren = !isSequence && Array.isArray(value.deletedItems) && value.deletedItems.length > 0;
5556
const hasChildren = hasValueChildren || hasDeletedChildren;
5657

5758
return (
@@ -97,7 +98,7 @@ export const ErrorTraceVariable = React.memo(({ value, stateId, settings }: Erro
9798
stateId={stateId}
9899
settings={settings} />)}
99100

100-
{value.deletedItems &&
101+
{!isSequence && value.deletedItems &&
101102
value.deletedItems.map(
102103
(childValue) =>
103104
<ErrorTraceVariable

tests/suite/model/check.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,46 @@ suite('Check Model Test Suite', () => {
9393
);
9494
});
9595

96+
test('Sequence diff treats head deletion as deletion only (value-based)', () => {
97+
const expected = seqX(ROOT, Change.MODIFIED,
98+
vX(1, Change.NOT_CHANGED, '2'),
99+
vX(2, Change.NOT_CHANGED, '3')
100+
);
101+
expected.addDeletedItems([v(1, '1')]);
102+
assertChanges(
103+
seq(ROOT, v(1, '1'), v(2, '2'), v(3, '3')),
104+
seq(ROOT, v(1, '2'), v(2, '3')),
105+
expected
106+
);
107+
});
108+
109+
test('Sequence diff treats middle deletion as function diff when not prefix/suffix', () => {
110+
const expected = seqX(ROOT, Change.MODIFIED,
111+
vX(1, Change.NOT_CHANGED, '1'),
112+
vX(2, Change.MODIFIED, '3'),
113+
vX(3, Change.MODIFIED, '4')
114+
);
115+
expected.addDeletedItems([v(4, '4')]);
116+
assertChanges(
117+
seq(ROOT, v(1, '1'), v(2, '2'), v(3, '3'), v(4, '4')),
118+
seq(ROOT, v(1, '1'), v(2, '3'), v(3, '4')),
119+
expected
120+
);
121+
});
122+
123+
test('Sequence diff treats head insertion as addition only (value-based)', () => {
124+
const expected = seqX(ROOT, Change.MODIFIED,
125+
vX(1, Change.ADDED, '1'),
126+
vX(2, Change.NOT_CHANGED, '2'),
127+
vX(3, Change.NOT_CHANGED, '3')
128+
);
129+
assertChanges(
130+
seq(ROOT, v(1, '2'), v(2, '3')),
131+
seq(ROOT, v(1, '1'), v(2, '2'), v(3, '3')),
132+
expected
133+
);
134+
});
135+
96136
test('Handles unchanged set', () => {
97137
assertChanges(
98138
set(ROOT, v(1, 'foo'), v(2, 'bar')),

0 commit comments

Comments
 (0)