WorkspaceSvg.cleanUp() method doesn't account for immovable blocks

[mikeharv]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

If blocks are immovable when cleanUp() is called, they are skipped. Other blocks are moved into a column but ignoring any immovable blocks that might already be there:

blockly/core/workspace_svg.ts

Lines 1812 to 1830 in 4125fd3

	cleanUp() {
	this.setResizesEnabled(false);
	eventUtils.setGroup(true);
	const topBlocks = this.getTopBlocks(true);
	let cursorY = 0;
	for (let i = 0, block; block = topBlocks[i]; i++) {
	if (!block.isMovable()) {
	continue;
	}
	const xy = block.getRelativeToSurfaceXY();
	block.moveBy(-xy.x, cursorY - xy.y);
	block.snapToGrid();
	cursorY = block.getRelativeToSurfaceXY().y +
	block.getHeightWidth().height +
	this.renderer.getConstants().MIN_BLOCK_HEIGHT;
	}
	eventUtils.setGroup(false);
	this.setResizesEnabled(true);
	}

This creates a situation where cleanUp() can actually make things look worse, covering up immovable blocks with movable ones.

Reproduction steps

1. Add several blocks to the workspace.
2. Set one or more blocks to be immovable (e.g. Blockly.selected.setMovable(false))
3. Select "Clean up blocks" from the workspace context menu.

Stack trace

No response

Screenshots

2023-03-09.08-16-55.2023-03-09.08_18_05.mp4

Browsers

No response

[rachel-fenichel]

Desired behaviour: when moving blocks during cleanup, check whether they will overlap with an immovable block, and move them somewhere else to avoid the conflict.

cleanUp is a fairly simple function on WorkspaceSvg. The change here is to build a list of immovable blocks and then check for conflicts. @mikeharv do you want to take a crack at this?

[maribethb]

For more background discussion, see #1734. Not necessarily related but has information about a smarter algorithm for cleaning up.