From 23ed1d51697e7a413f3fb0e1eb0168f9c870b64e Mon Sep 17 00:00:00 2001 From: Alan Smith Date: Tue, 10 Nov 2026 13:48:38 -0500 Subject: [PATCH 1/5] Fix conflict where highlight was being overridden --- frontend/index.css | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/frontend/index.css b/frontend/index.css index a505a68a..4c1cd7f1 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -73,36 +73,48 @@ code { --mrc-diff-moved: var(--mrc-diff-changed); } -.mrcDiffAdded > .blocklyPath, -.mrcDiffRemoved > .blocklyPath, -.mrcDiffMoved > .blocklyPath, -.mrcDiffChanged > .blocklyPath { +/* The block styles set the outline width with selectors like + * `.-renderer.-theme .blocklyPath`, so these selectors start with + * `.blocklySvg .blocklyBlockCanvas` to be more specific than those. */ +.blocklySvg .blocklyBlockCanvas .mrcDiffAdded > .blocklyPath, +.blocklySvg .blocklyBlockCanvas .mrcDiffRemoved > .blocklyPath, +.blocklySvg .blocklyBlockCanvas .mrcDiffMoved > .blocklyPath, +.blocklySvg .blocklyBlockCanvas .mrcDiffChanged > .blocklyPath { stroke-width: 4px; } -.mrcDiffAdded > .blocklyPath { +/* The FIRST blocks style draws a light line inside the top edge of each block, which would cover + * part of the outline. */ +.mrcDiffAdded > .firstBlocksStyleTopHighlight, +.mrcDiffRemoved > .firstBlocksStyleTopHighlight, +.mrcDiffMoved > .firstBlocksStyleTopHighlight, +.mrcDiffChanged > .firstBlocksStyleTopHighlight { + display: none; +} + +.blocklySvg .blocklyBlockCanvas .mrcDiffAdded > .blocklyPath { stroke: var(--mrc-diff-added); } -.mrcDiffRemoved > .blocklyPath { +.blocklySvg .blocklyBlockCanvas .mrcDiffRemoved > .blocklyPath { stroke: var(--mrc-diff-removed); } /* Moved blocks are dotted: zero-length dashes with round caps are drawn as dots. This is before * changed, so a block that was moved and changed is outlined as changed. */ -.mrcDiffMoved > .blocklyPath { +.blocklySvg .blocklyBlockCanvas .mrcDiffMoved > .blocklyPath { stroke: var(--mrc-diff-moved); stroke-dasharray: 0 8; stroke-linecap: round; } -.mrcDiffChanged > .blocklyPath { +.blocklySvg .blocklyBlockCanvas .mrcDiffChanged > .blocklyPath { stroke: var(--mrc-diff-changed); stroke-dasharray: none; stroke-linecap: butt; } /* The change that was last scrolled to with Previous Change or Next Change. */ -.mrcDiffCurrent > .blocklyPath { +.blocklySvg .blocklyBlockCanvas .mrcDiffCurrent > .blocklyPath { stroke-width: 7px; } From 62e5dfad8a233cce6857df072572562180a49963 Mon Sep 17 00:00:00 2001 From: Alan Smith Date: Tue, 10 Nov 2026 13:48:47 -0500 Subject: [PATCH 2/5] Change so next/previous don't wrap --- frontend/reactComponents/VersionsModal.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frontend/reactComponents/VersionsModal.tsx b/frontend/reactComponents/VersionsModal.tsx index 85e9db70..82966c09 100644 --- a/frontend/reactComponents/VersionsModal.tsx +++ b/frontend/reactComponents/VersionsModal.tsx @@ -598,13 +598,13 @@ export default function VersionsModal(props: VersionsModalProps): React.JSX.Elem (positions.get(a)!.y - positions.get(b)!.y) || (positions.get(a)!.x - positions.get(b)!.x)); }; - /** Scrolls the panes to the change at the given index, wrapping around at either end. */ + /** Scrolls the panes to the change at the given index. */ const goToChange = (index: number): void => { const changes = getChanges(); if (!changes.length) { return; } - const i = (index + changes.length) % changes.length; + const i = Math.min(Math.max(index, 0), changes.length - 1); const change = changes[i]; currentChangeBlocksRef.current.forEach(block => block.removeClass(CURRENT_CHANGE_CLASS)); currentChangeBlocksRef.current = []; @@ -683,7 +683,7 @@ export default function VersionsModal(props: VersionsModalProps): React.JSX.Elem } - disabled={changeCount === 0} + disabled={changeCount === 0 || changeIndex <= 0} onClick={() => goToChange(changeIndex - 1)} > {t('VERSIONS.PREVIOUS_CHANGE')} @@ -691,7 +691,7 @@ export default function VersionsModal(props: VersionsModalProps): React.JSX.Elem } - disabled={changeCount === 0} + disabled={changeCount === 0 || changeIndex >= changeCount - 1} onClick={() => goToChange(changeIndex + 1)} > {t('VERSIONS.NEXT_CHANGE')} From bac4fbee237fab5d98456a2a00f205637980f101 Mon Sep 17 00:00:00 2001 From: Alan Smith Date: Tue, 10 Nov 2026 14:34:23 -0500 Subject: [PATCH 3/5] Cleanup to highlight deleted and added groups instead of just outer block --- docs/project_versions_design.md | 3 +- frontend/index.css | 60 +++++++----- .../ReadOnlyBlocklyPreview.tsx | 95 ++++++++++++++++++- frontend/reactComponents/VersionsModal.tsx | 6 +- frontend/storage/version_diff.ts | 34 +++++-- tests/versions.test.ts | 19 ++-- 6 files changed, 173 insertions(+), 44 deletions(-) diff --git a/docs/project_versions_design.md b/docs/project_versions_design.md index b1257a8f..3db244ae 100644 --- a/docs/project_versions_design.md +++ b/docs/project_versions_design.md @@ -43,7 +43,8 @@ objects *first*, then reads every `versionId.json` (including incomplete ones), * All objects are local to a project. (ie, even if a module is identical across projects there is a copy per project) * Modules are compared by ID, so a renamed module is still compared with itself -* Only the outermost added or removed block is marked +* Every added or removed block is outlined, but a group of them (a block with the blocks inside + and after it that were added or removed too) is one change for next/previous change * Moved top-level stacks count as a change. (but not in the next/previous change) # Diffs diff --git a/frontend/index.css b/frontend/index.css index 4c1cd7f1..2ca067f1 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -65,7 +65,9 @@ code { scroll-behavior: smooth !important; } /* Blocks that differ between a version and the current project (see VersionsModal.tsx). The - * colors are used for the legend too. Only the block's own outline is styled, not its children. */ + * colors are used for the legend too. Changed and moved blocks have their own outline styled, not + * their children's. Added and removed blocks have one outline around each group of them (made by + * ReadOnlyBlocklyPreview.tsx), which is masked so only the outer half of its stroke shows. */ :root { --mrc-diff-added: #2e9d4c; --mrc-diff-removed: #d9363e; @@ -73,48 +75,62 @@ code { --mrc-diff-moved: var(--mrc-diff-changed); } -/* The block styles set the outline width with selectors like - * `.-renderer.-theme .blocklyPath`, so these selectors start with - * `.blocklySvg .blocklyBlockCanvas` to be more specific than those. */ -.blocklySvg .blocklyBlockCanvas .mrcDiffAdded > .blocklyPath, -.blocklySvg .blocklyBlockCanvas .mrcDiffRemoved > .blocklyPath, -.blocklySvg .blocklyBlockCanvas .mrcDiffMoved > .blocklyPath, -.blocklySvg .blocklyBlockCanvas .mrcDiffChanged > .blocklyPath { - stroke-width: 4px; +.mrcDiffGroupOutlineStrokes > path { + fill: none; + /* 2px shows outside the group: half of the stroke, less the 1px that the mask's stroke covers. */ + stroke-width: 6px; + stroke-linejoin: round; +} + +.mrcDiffAdded > .mrcDiffGroupOutline > .mrcDiffGroupOutlineStrokes > path { + stroke: var(--mrc-diff-added); +} + +.mrcDiffRemoved > .mrcDiffGroupOutline > .mrcDiffGroupOutlineStrokes > path { + stroke: var(--mrc-diff-removed); +} + +/* The paths of the outline's mask are filled, so that the mask hides the stroke inside the group. + * They are stroked too, to close the gaps that antialiasing leaves where two blocks meet. */ +.mrcDiffGroupOutline mask path { + fill: #000; + stroke: #000; + stroke-width: 2px; } /* The FIRST blocks style draws a light line inside the top edge of each block, which would cover * part of the outline. */ -.mrcDiffAdded > .firstBlocksStyleTopHighlight, -.mrcDiffRemoved > .firstBlocksStyleTopHighlight, .mrcDiffMoved > .firstBlocksStyleTopHighlight, .mrcDiffChanged > .firstBlocksStyleTopHighlight { display: none; } -.blocklySvg .blocklyBlockCanvas .mrcDiffAdded > .blocklyPath { - stroke: var(--mrc-diff-added); -} - -.blocklySvg .blocklyBlockCanvas .mrcDiffRemoved > .blocklyPath { - stroke: var(--mrc-diff-removed); -} - -/* Moved blocks are dotted: zero-length dashes with round caps are drawn as dots. This is before +/* The block styles set the outline width with selectors like + * `.-renderer.-theme .blocklyPath`, so these selectors start with + * `.blocklySvg .blocklyBlockCanvas` to be more specific than those. + * + * Moved blocks are dotted: zero-length dashes with round caps are drawn as dots. This is before * changed, so a block that was moved and changed is outlined as changed. */ .blocklySvg .blocklyBlockCanvas .mrcDiffMoved > .blocklyPath { stroke: var(--mrc-diff-moved); + /* Wider than the other outlines, since the dots aren't solid. */ + stroke-width: 4px; stroke-dasharray: 0 8; stroke-linecap: round; } .blocklySvg .blocklyBlockCanvas .mrcDiffChanged > .blocklyPath { stroke: var(--mrc-diff-changed); + stroke-width: 2px; stroke-dasharray: none; stroke-linecap: butt; } /* The change that was last scrolled to with Previous Change or Next Change. */ -.blocklySvg .blocklyBlockCanvas .mrcDiffCurrent > .blocklyPath { - stroke-width: 7px; +.blocklySvg .blocklyBlockCanvas .mrcDiffCurrent.mrcDiffChanged > .blocklyPath { + stroke-width: 4px; +} + +.mrcDiffCurrent > .mrcDiffGroupOutline > .mrcDiffGroupOutlineStrokes > path { + stroke-width: 10px; } diff --git a/frontend/reactComponents/ReadOnlyBlocklyPreview.tsx b/frontend/reactComponents/ReadOnlyBlocklyPreview.tsx index 8f847da1..ae7d9e46 100644 --- a/frontend/reactComponents/ReadOnlyBlocklyPreview.tsx +++ b/frontend/reactComponents/ReadOnlyBlocklyPreview.tsx @@ -47,13 +47,22 @@ export interface BlockHighlights { moved?: Set; } -/** The CSS class (defined in index.css) for each kind of highlight. */ +/** + * The CSS class (defined in index.css) for each kind of highlight. Added and removed blocks are + * outlined in groups (see addGroupOutline), and changed and moved blocks one at a time. + */ const HIGHLIGHT_CLASSES: [keyof BlockHighlights, string][] = [ ['added', 'mrcDiffAdded'], ['removed', 'mrcDiffRemoved'], ['changed', 'mrcDiffChanged'], ['moved', 'mrcDiffMoved'], ]; +const GROUP_HIGHLIGHT_KINDS: (keyof BlockHighlights)[] = ['added', 'removed']; +const GROUP_OUTLINE_CLASS = 'mrcDiffGroupOutline'; +const GROUP_OUTLINE_STROKES_CLASS = 'mrcDiffGroupOutlineStrokes'; + +/** Used to give each group outline's mask a unique id. */ +let nextGroupOutlineMaskId = 1; const FULL_SIZE_STYLE: React.CSSProperties = { width: '100%', @@ -238,19 +247,28 @@ export default function ReadOnlyBlocklyPreview(props: ReadOnlyBlocklyPreviewProp } /** - * Adds the highlight CSS classes to the given blocks. Returns the top-most highlighted block, or - * null if no blocks were highlighted. + * Highlights the given blocks: adds the highlight CSS classes to them, and outlines each group of + * added or removed blocks. Returns the top-most highlighted block, or null if no blocks were + * highlighted. */ function highlightBlocks( workspace: Blockly.WorkspaceSvg, highlights?: BlockHighlights): Blockly.BlockSvg | null { + // The outlines are made from the blocks' rendered shapes. + Blockly.renderManagement.triggerQueuedRenders(workspace); let first: Blockly.BlockSvg | null = null; for (const [kind, className] of HIGHLIGHT_CLASSES) { - for (const id of highlights?.[kind] ?? []) { + const ids = highlights?.[kind] ?? new Set(); + for (const id of ids) { const block = workspace.getBlockById(id); if (!block) { continue; } block.addClass(className); + const parent = block.getParent(); + const isFirstInGroup = !parent || !ids.has(parent.id); + if (GROUP_HIGHLIGHT_KINDS.includes(kind) && isFirstInGroup) { + addGroupOutline(block, ids); + } if (!first || block.getRelativeToSurfaceXY().y < first.getRelativeToSurfaceXY().y) { first = block; } @@ -258,3 +276,72 @@ function highlightBlocks( } return first; } + +/** + * Outlines the outside of the group of blocks that starts with the given block: the block, and the + * blocks inside it and after it that are in the given ids too. Each block's outer edge is + * stroked, and a mask of all of the shapes hides the strokes inside the group, so only the outside + * edge is left. The outline is added to the first block's SVG group, after the blocks in it, so that it + * moves with the block and is drawn over the blocks around it. + */ +function addGroupOutline(firstBlock: Blockly.BlockSvg, ids: Set): void { + const groupBlocks: Blockly.BlockSvg[] = []; + const addToGroup = (block: Blockly.BlockSvg): void => { + groupBlocks.push(block); + for (const child of block.getChildren(false)) { + if (ids.has(child.id)) { + addToGroup(child); + } + } + }; + addToGroup(firstBlock); + + const svg = Blockly.utils.Svg; + const dom = Blockly.utils.dom; + const outline = dom.createSvgElement(svg.G, { + 'class': GROUP_OUTLINE_CLASS, + 'pointer-events': 'none', + }); + const maskId = `mrcDiffGroupOutlineMask${nextGroupOutlineMaskId++}`; + const mask = dom.createSvgElement('mask', { + 'id': maskId, + 'maskUnits': 'userSpaceOnUse', + 'x': -1e6, 'y': -1e6, 'width': 2e6, 'height': 2e6, + }, dom.createSvgElement(svg.DEFS, {}, outline)); + dom.createSvgElement(svg.RECT, { + 'x': -1e6, 'y': -1e6, 'width': 2e6, 'height': 2e6, 'fill': '#fff', + }, mask); + const strokes = dom.createSvgElement(svg.G, { + 'class': GROUP_OUTLINE_STROKES_CLASS, + 'mask': `url(#${maskId})`, + }, outline); + + const origin = firstBlock.getRelativeToSurfaceXY(); + for (const block of groupBlocks) { + const d = getOuterPath(block.pathObject.svgPath.getAttribute('d') ?? ''); + if (!d) { + continue; + } + const xy = block.getRelativeToSurfaceXY(); + const transform = `translate(${xy.x - origin.x}, ${xy.y - origin.y})`; + dom.createSvgElement(svg.PATH, {'d': d, 'transform': transform}, mask); + dom.createSvgElement(svg.PATH, {'d': d, 'transform': transform}, strokes); + } + firstBlock.getSvgRoot().appendChild(outline); +} + +/** + * Returns the outer edge of a block from its path: everything before the first move that comes + * after something is drawn. The renderers draw the sockets of a block's inline inputs (the ones its + * fields and blocks are in) after the outer edge, each starting with a move. The outer edge itself + * can start with more than one move, such as ' m 0,0 m 0,8 a ...' for a statement block. + */ +function getOuterPath(d: string): string { + // Command letters other than moves (and the e of an exponent in a number). + const firstDraw = d.search(/[a-df-ln-zA-DF-LN-Z]/); + if (firstDraw === -1) { + return ''; + } + const nextMove = d.slice(firstDraw).search(/[mM]/); + return nextMove === -1 ? d : d.slice(0, firstDraw + nextMove); +} diff --git a/frontend/reactComponents/VersionsModal.tsx b/frontend/reactComponents/VersionsModal.tsx index 82966c09..93a0472c 100644 --- a/frontend/reactComponents/VersionsModal.tsx +++ b/frontend/reactComponents/VersionsModal.tsx @@ -568,7 +568,7 @@ export default function VersionsModal(props: VersionsModalProps): React.JSX.Elem // An unchanged module is shown in one preview, since both would show the same blocks. const isUnchanged = selectedDiff?.change === versionDiff.ModuleChange.UNCHANGED; const changeCount = selectedDiff - ? selectedDiff.blockDiff.added.size + selectedDiff.blockDiff.removed.size + + ? selectedDiff.blockDiff.addedGroups.size + selectedDiff.blockDiff.removedGroups.size + selectedDiff.blockDiff.changed.size : 0; @@ -585,10 +585,10 @@ export default function VersionsModal(props: VersionsModalProps): React.JSX.Elem } const blockDiff = selectedDiff.blockDiff; const changes: BlockChange[] = [ - ...[...blockDiff.removed].map(blockId => ({ blockId, panes: ['left'] as Pane[] })), + ...[...blockDiff.removedGroups].map(blockId => ({ blockId, panes: ['left'] as Pane[] })), ...[...blockDiff.changed].map( blockId => ({ blockId, panes: ['right', 'left'] as Pane[] })), - ...[...blockDiff.added].map(blockId => ({ blockId, panes: ['right'] as Pane[] })), + ...[...blockDiff.addedGroups].map(blockId => ({ blockId, panes: ['right'] as Pane[] })), ]; const getPosition = (change: BlockChange): Blockly.utils.Coordinate => workspacesRef.current[change.panes[0]]?.getBlockById(change.blockId) diff --git a/frontend/storage/version_diff.ts b/frontend/storage/version_diff.ts index 962a89d8..f52b4923 100644 --- a/frontend/storage/version_diff.ts +++ b/frontend/storage/version_diff.ts @@ -21,6 +21,10 @@ export enum ModuleChange { export type BlockDiff = { added: Set, // ids of blocks only in the right module removed: Set, // ids of blocks only in the left module + // ids of the first block of each group of added or removed blocks: the blocks inside it and + // after it in its stack that were added or removed too are in the same group + addedGroups: Set, + removedGroups: Set, changed: Set, // ids of blocks in both whose own content differs moved: Set, // ids of top-level blocks in both that are at a different position }; @@ -126,8 +130,9 @@ function indexModules(files: {[fileName: string]: string}): Map { return infos; } +/** + * Returns whether the given block, which is not in the other workspace, is the first block of a + * group of added or removed blocks: neither the block it is inside nor the block before it in its + * stack was also added or removed. + */ +function isFirstInGroup(info: BlockInfo, otherInfos: Map): boolean { + const isInOther = (id: string | null): boolean => id === null || otherInfos.has(id); + return isInOther(info.enclosingId) && + (info.indexInStack === 0 || isInOther(info.stack[info.indexInStack - 1])); +} + /** * Returns the id of the closest block before the given block in its stack that is also in the * other workspace, or '' if there is none. Blocks that were only in one workspace are skipped, so diff --git a/tests/versions.test.ts b/tests/versions.test.ts index aaa79325..5201bdb3 100644 --- a/tests/versions.test.ts +++ b/tests/versions.test.ts @@ -75,19 +75,22 @@ describe('diffBlocks', () => { expect([...diff.removed]).toEqual(['n1']); }); - test('deleting a block marks only that block, not the blocks inside it or its parent', () => { + test('deleting a block marks it and the blocks inside it, not its parent', () => { // if (p1 printing n1) { p2 printing n2 }, with p3 after p2. const statements = stack(printBlock('p2', numberBlock('n2', 2)), printBlock('p3', numberBlock('n3', 3))); const leftBlocks = makeBlocks(ifBlock('if1', stack(printBlock('p1', numberBlock('n1', 1)), statements))); const rightBlocks = makeBlocks(ifBlock('if1', stack(printBlock('p1', numberBlock('n1', 1))))); const diff = versionDiff.diffBlocks(leftBlocks, rightBlocks); - expect([...diff.removed].sort()).toEqual(['p2', 'p3']); + expect([...diff.removed].sort()).toEqual(['n2', 'n3', 'p2', 'p3']); + // p3 is after p2 in its stack, so they are one group. + expect([...diff.removedGroups]).toEqual(['p2']); expect([...diff.changed]).toEqual([]); expect([...diff.added]).toEqual([]); - // Deleting the if block marks only the if block. + // Deleting the if block makes one group that starts with the if block. const deletedIf = versionDiff.diffBlocks(leftBlocks, makeBlocks()); - expect([...deletedIf.removed]).toEqual(['if1']); + expect([...deletedIf.removed].sort()).toEqual(['if1', 'n1', 'n2', 'n3', 'p1', 'p2', 'p3']); + expect([...deletedIf.removedGroups]).toEqual(['if1']); }); test('deleting a block from the middle of a stack does not mark the blocks around it', () => { @@ -95,15 +98,17 @@ describe('diffBlocks', () => { makeBlocks(stack(printBlock('a', numberBlock('na', 1)), printBlock('b', numberBlock('nb', 2)), printBlock('c', numberBlock('nc', 3)))), makeBlocks(stack(printBlock('a', numberBlock('na', 1)), printBlock('c', numberBlock('nc', 3))))); - expect([...diff.removed]).toEqual(['b']); + expect([...diff.removed].sort()).toEqual(['b', 'nb']); + expect([...diff.removedGroups]).toEqual(['b']); expect([...diff.changed]).toEqual([]); }); - test('adding a block with blocks inside it marks only that block', () => { + test('adding a block with blocks inside it makes one group', () => { const diff = versionDiff.diffBlocks( makeBlocks(ifBlock('if1')), makeBlocks(ifBlock('if1', stack(printBlock('p1', numberBlock('n1', 1)))))); - expect([...diff.added]).toEqual(['p1']); + expect([...diff.added].sort()).toEqual(['n1', 'p1']); + expect([...diff.addedGroups]).toEqual(['p1']); expect([...diff.changed]).toEqual([]); }); From 2df0ec9be66f0ff69f93c81ec698dcfea3e7b2b2 Mon Sep 17 00:00:00 2001 From: Alan Smith Date: Tue, 10 Nov 2026 14:55:29 -0500 Subject: [PATCH 4/5] Fixed minor bug in diff If a component was moved to a mechanism, the port block would show as changed instead of added/deleted. --- docs/project_versions_design.md | 2 + frontend/storage/version_diff.ts | 70 ++++++++++++++++++++++++-------- tests/versions.test.ts | 21 ++++++++++ 3 files changed, 75 insertions(+), 18 deletions(-) diff --git a/docs/project_versions_design.md b/docs/project_versions_design.md index 3db244ae..3b8d6ee7 100644 --- a/docs/project_versions_design.md +++ b/docs/project_versions_design.md @@ -45,6 +45,8 @@ objects *first*, then reads every `versionId.json` (including incomplete ones), * Modules are compared by ID, so a renamed module is still compared with itself * Every added or removed block is outlined, but a group of them (a block with the blocks inside and after it that were added or removed too) is one change for next/previous change +* A block moved out of a removed block into an added block (like a component's port when the + component is moved into a mechanism) is part of both groups, not changed * Moved top-level stacks count as a change. (but not in the next/previous change) # Diffs diff --git a/frontend/storage/version_diff.ts b/frontend/storage/version_diff.ts index f52b4923..49da84db 100644 --- a/frontend/storage/version_diff.ts +++ b/frontend/storage/version_diff.ts @@ -19,8 +19,12 @@ export enum ModuleChange { } export type BlockDiff = { - added: Set, // ids of blocks only in the right module - removed: Set, // ids of blocks only in the left module + // ids of blocks only in the right module, and of blocks that were moved into them from blocks + // only in the left module + added: Set, + // ids of blocks only in the left module, and of blocks that were moved out of them into blocks + // only in the right module + removed: Set, // ids of the first block of each group of added or removed blocks: the blocks inside it and // after it in its stack that were added or removed too are in the same group addedGroups: Set, @@ -133,6 +137,10 @@ function indexModules(files: {[fileName: string]: string}): Map(); + let foundMore = true; + while (foundMore) { + foundMore = false; + for (const [id, rightInfo] of rightInfos) { + const leftInfo = leftInfos.get(id); + if (leftInfo && !wentAlong.has(id) && + leftInfo.enclosingId && diff.removed.has(leftInfo.enclosingId) && + rightInfo.enclosingId && diff.added.has(rightInfo.enclosingId)) { + wentAlong.add(id); + diff.added.add(id); + diff.removed.add(id); + foundMore = true; + } + } + } + for (const [id, rightInfo] of rightInfos) { const leftInfo = leftInfos.get(id); if (leftInfo?.position && rightInfo.position && leftInfo.position !== rightInfo.position) { diff.moved.add(id); } if (!leftInfo) { - diff.added.add(id); - if (isFirstInGroup(rightInfo, leftInfos)) { + if (isFirstInGroup(rightInfo, diff.added)) { diff.addedGroups.add(id); } } else if (leftInfo.fingerprint !== rightInfo.fingerprint || - leftInfo.location !== rightInfo.location || - getPreviousBlockIn(leftInfo, rightInfos) !== getPreviousBlockIn(rightInfo, leftInfos)) { + (!wentAlong.has(id) && (leftInfo.location !== rightInfo.location || + getPreviousBlockIn(leftInfo, rightInfos) !== + getPreviousBlockIn(rightInfo, leftInfos)))) { diff.changed.add(id); } } for (const [id, leftInfo] of leftInfos) { - if (!rightInfos.has(id)) { - diff.removed.add(id); - if (isFirstInGroup(leftInfo, rightInfos)) { - diff.removedGroups.add(id); - } + if (!rightInfos.has(id) && isFirstInGroup(leftInfo, diff.removed)) { + diff.removedGroups.add(id); } } return diff; @@ -222,14 +256,14 @@ function indexBlocks(blocks: {[key: string]: any}): Map { } /** - * Returns whether the given block, which is not in the other workspace, is the first block of a - * group of added or removed blocks: neither the block it is inside nor the block before it in its - * stack was also added or removed. + * Returns whether the given block, which is in the given added or removed blocks, is the first + * block of a group of them: neither the block it is inside nor the block before it in its stack is + * also in them. */ -function isFirstInGroup(info: BlockInfo, otherInfos: Map): boolean { - const isInOther = (id: string | null): boolean => id === null || otherInfos.has(id); - return isInOther(info.enclosingId) && - (info.indexInStack === 0 || isInOther(info.stack[info.indexInStack - 1])); +function isFirstInGroup(info: BlockInfo, groupIds: Set): boolean { + const isInGroup = (id: string | null): boolean => id !== null && groupIds.has(id); + return !isInGroup(info.enclosingId) && + (info.indexInStack === 0 || !isInGroup(info.stack[info.indexInStack - 1])); } /** diff --git a/tests/versions.test.ts b/tests/versions.test.ts index 5201bdb3..3465002c 100644 --- a/tests/versions.test.ts +++ b/tests/versions.test.ts @@ -103,6 +103,27 @@ describe('diffBlocks', () => { expect([...diff.changed]).toEqual([]); }); + test('a block moved from a removed block into an added block goes along with them', () => { + // Like moving a component into a mechanism, which moves the component's port shadow block. + const withShadow = (id: string, shadow: any): any => + ({ type: 'text_print', id, x: 10, y: 10, inputs: { TEXT: { shadow } } }); + const diff = versionDiff.diffBlocks( + makeBlocks(withShadow('p1', numberBlock('s1', 1))), + makeBlocks(withShadow('p2', numberBlock('s1', 1)))); + expect([...diff.removed].sort()).toEqual(['p1', 's1']); + expect([...diff.added].sort()).toEqual(['p2', 's1']); + expect([...diff.removedGroups]).toEqual(['p1']); + expect([...diff.addedGroups]).toEqual(['p2']); + expect([...diff.changed]).toEqual([]); + + // A block moved from a removed block into a block that is in both is still changed. + const intoExisting = versionDiff.diffBlocks( + makeBlocks(withShadow('p1', numberBlock('s1', 1)), printBlock('p3', null)), + makeBlocks(withShadow('p3', numberBlock('s1', 1)))); + expect([...intoExisting.removed]).toEqual(['p1']); + expect([...intoExisting.changed]).toEqual(['s1']); + }); + test('adding a block with blocks inside it makes one group', () => { const diff = versionDiff.diffBlocks( makeBlocks(ifBlock('if1')), From 2d95297615f6d2b86a4889ced6cc260b525757e0 Mon Sep 17 00:00:00 2001 From: Alan Smith Date: Wed, 11 Nov 2026 14:56:49 -0500 Subject: [PATCH 5/5] Make sure that all blocks start with an id This is what caused the bug while showing the tech team where it had unchanged items showing as added. --- frontend/storage/module_content.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/frontend/storage/module_content.ts b/frontend/storage/module_content.ts index dbe96d16..a5440bb8 100644 --- a/frontend/storage/module_content.ts +++ b/frontend/storage/module_content.ts @@ -56,9 +56,19 @@ function startingBlocksToModuleContentText( const privateComponents: Component[] = []; const events: Event[] = []; const methods: Method[] = []; + // The starting blocks don't have ids, so give each block one. Otherwise the blocks would only + // get ids when the module is loaded into the editor, and a version created before the module + // is saved would have blocks without ids, which can't be matched with the blocks in other + // versions. Copy the starting blocks first so the imported JSON isn't changed. + const blocks = JSON.parse(JSON.stringify(startingBlocks)); + visitAllBlockJson(blocks, blockJson => { + if (!blockJson.id) { + blockJson.id = Blockly.utils.idGenerator.genUid(); + } + }); return makeModuleContentText( module, - startingBlocks, + blocks, mechanisms, components, privateComponents,