Merge pull request #8550 from BenHenning/ensure-immovable-blocks-are-considered-during-workspace-tidying

fix: Ensure immovable blocks are considered during workspace tidying
This commit is contained in:
Ben Henning
2024-09-05 13:40:28 -07:00
committed by GitHub
6 changed files with 2330 additions and 16 deletions

View File

@@ -13,6 +13,8 @@
*/
// Former goog.module ID: Blockly.utils.Rect
import {Coordinate} from './coordinate.js';
/**
* Class for representing rectangular regions.
*/
@@ -30,10 +32,21 @@ export class Rect {
public right: number,
) {}
/**
* Creates a new copy of this rectangle.
*
* @returns A copy of this Rect.
*/
clone(): Rect {
return new Rect(this.top, this.bottom, this.left, this.right);
}
/** Returns the height of this rectangle. */
getHeight(): number {
return this.bottom - this.top;
}
/** Returns the width of this rectangle. */
getWidth(): number {
return this.right - this.left;
}
@@ -59,11 +72,56 @@ export class Rect {
* @returns Whether this rectangle intersects the provided rectangle.
*/
intersects(other: Rect): boolean {
return !(
this.left > other.right ||
this.right < other.left ||
this.top > other.bottom ||
this.bottom < other.top
// The following logic can be derived and then simplified from a longer form symmetrical check
// of verifying each rectangle's borders with the other rectangle by checking if either end of
// the border's line segment is contained within the other rectangle. The simplified version
// used here can be logically interpreted as ensuring that each line segment of 'this' rectangle
// is not outside the bounds of the 'other' rectangle (proving there's an intersection).
return (
this.left <= other.right &&
this.right >= other.left &&
this.bottom >= other.top &&
this.top <= other.bottom
);
}
/**
* Compares bounding rectangles for equality.
*
* @param a A Rect.
* @param b A Rect.
* @returns True iff the bounding rectangles are equal, or if both are null.
*/
static equals(a?: Rect | null, b?: Rect | null): boolean {
if (a === b) {
return true;
}
if (!a || !b) {
return false;
}
return (
a.top === b.top &&
a.bottom === b.bottom &&
a.left === b.left &&
a.right === b.right
);
}
/**
* Creates a new Rect using a position and supplied dimensions.
*
* @param position The upper left coordinate of the new rectangle.
* @param width The width of the rectangle, in pixels.
* @param height The height of the rectangle, in pixels.
* @returns A newly created Rect using the provided Coordinate and dimensions.
*/
static createFromPoint(
position: Coordinate,
width: number,
height: number,
): Rect {
const left = position.x;
const top = position.y;
return new Rect(top, top + height, left, left + width);
}
}

View File

@@ -1645,23 +1645,56 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
return boundary;
}
/** Clean up the workspace by ordering all the blocks in a column. */
/** Clean up the workspace by ordering all the blocks in a column such that none overlap. */
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 movableBlocks = topBlocks.filter((block) => block.isMovable());
const immovableBlocks = topBlocks.filter((block) => !block.isMovable());
const immovableBlockBounds = immovableBlocks.map((block) =>
block.getBoundingRectangle(),
);
const getNextIntersectingImmovableBlock = function (
rect: Rect,
): Rect | null {
for (const immovableRect of immovableBlockBounds) {
if (rect.intersects(immovableRect)) {
return immovableRect;
}
}
const xy = block.getRelativeToSurfaceXY();
block.moveBy(-xy.x, cursorY - xy.y, ['cleanup']);
return null;
};
let cursorY = 0;
const minBlockHeight = this.renderer.getConstants().MIN_BLOCK_HEIGHT;
for (const block of movableBlocks) {
// Make the initial movement of shifting the block to its best possible position.
let boundingRect = block.getBoundingRectangle();
block.moveBy(-boundingRect.left, cursorY - boundingRect.top, ['cleanup']);
block.snapToGrid();
boundingRect = block.getBoundingRectangle();
let conflictingRect = getNextIntersectingImmovableBlock(boundingRect);
while (conflictingRect != null) {
// If the block intersects with an immovable block, move it down past that immovable block.
cursorY =
conflictingRect.top + conflictingRect.getHeight() + minBlockHeight;
block.moveBy(0, cursorY - boundingRect.top, ['cleanup']);
block.snapToGrid();
boundingRect = block.getBoundingRectangle();
conflictingRect = getNextIntersectingImmovableBlock(boundingRect);
}
// Ensure all next blocks start past the most recent (which will also put them past all
// previously intersecting immovable blocks).
cursorY =
block.getRelativeToSurfaceXY().y +
block.getHeightWidth().height +
this.renderer.getConstants().MIN_BLOCK_HEIGHT;
minBlockHeight;
}
eventUtils.setGroup(false);
this.setResizesEnabled(true);

View File

@@ -123,7 +123,7 @@ suite('Context Menu Items', function () {
suite('Cleanup', function () {
setup(function () {
this.cleanupOption = this.registry.getItem('cleanWorkspace');
this.cleanupStub = sinon.stub(this.workspace, 'cleanUp');
this.cleanUpStub = sinon.stub(this.workspace, 'cleanUp');
});
test('Enabled when multiple blocks', function () {
@@ -153,9 +153,9 @@ suite('Context Menu Items', function () {
);
});
test('Calls workspace cleanup', function () {
test('Calls workspace cleanUp', function () {
this.cleanupOption.callback(this.scope);
sinon.assert.calledOnce(this.cleanupStub);
sinon.assert.calledOnce(this.cleanUpStub);
});
test('Has correct label', function () {

View File

@@ -110,6 +110,7 @@
import './old_workspace_comment_test.js';
import './procedure_map_test.js';
import './blocks/procedures_test.js';
import './rect_test.js';
import './registry_test.js';
import './render_management_test.js';
import './serializer_test.js';

1668
tests/mocha/rect_test.js Normal file

File diff suppressed because it is too large Load Diff

View File

@@ -406,6 +406,560 @@ suite('WorkspaceSvg', function () {
});
});
});
suite('cleanUp', function () {
test('empty workspace does not change', function () {
this.workspace.cleanUp();
const blocks = this.workspace.getTopBlocks(true);
assert.equal(blocks.length, 0, 'workspace is empty');
});
test('single block at (0, 0) does not change', function () {
const blockJson = {
'type': 'math_number',
'x': 0,
'y': 0,
'fields': {
'NUM': 123,
},
};
Blockly.serialization.blocks.append(blockJson, this.workspace);
this.workspace.cleanUp();
const blocks = this.workspace.getTopBlocks(true);
const origin = new Blockly.utils.Coordinate(0, 0);
assert.equal(blocks.length, 1, 'workspace has one top-level block');
assert.deepEqual(
blocks[0].getRelativeToSurfaceXY(),
origin,
'block is at origin',
);
});
test('single block at (10, 15) is moved to (0, 0)', function () {
const blockJson = {
'type': 'math_number',
'x': 10,
'y': 15,
'fields': {
'NUM': 123,
},
};
Blockly.serialization.blocks.append(blockJson, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const allBlocks = this.workspace.getAllBlocks(false);
const origin = new Blockly.utils.Coordinate(0, 0);
assert.equal(topBlocks.length, 1, 'workspace has one top-level block');
assert.equal(allBlocks.length, 1, 'workspace has one block overall');
assert.deepEqual(
topBlocks[0].getRelativeToSurfaceXY(),
origin,
'block is at origin',
);
});
test('single block at (10, 15) with child is moved as unit to (0, 0)', function () {
const blockJson = {
'type': 'logic_negate',
'id': 'parent',
'x': 10,
'y': 15,
'inputs': {
'BOOL': {
'block': {
'type': 'logic_boolean',
'id': 'child',
'fields': {
'BOOL': 'TRUE',
},
},
},
},
};
Blockly.serialization.blocks.append(blockJson, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const allBlocks = this.workspace.getAllBlocks(false);
const origin = new Blockly.utils.Coordinate(0, 0);
assert.equal(topBlocks.length, 1, 'workspace has one top-level block');
assert.equal(allBlocks.length, 2, 'workspace has two blocks overall');
assert.deepEqual(
topBlocks[0].getRelativeToSurfaceXY(),
origin,
'block is at origin',
);
assert.notDeepEqual(
allBlocks[1].getRelativeToSurfaceXY(),
origin,
'child is not at origin',
);
});
test('two blocks first at (10, 15) second at (0, 0) do not switch places', function () {
const blockJson1 = {
'type': 'math_number',
'id': 'block1',
'x': 10,
'y': 15,
'fields': {
'NUM': 123,
},
};
const blockJson2 = {...blockJson1, 'id': 'block2', 'x': 0, 'y': 0};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
this.workspace.cleanUp();
// block1 and block2 do not switch places since blocks are pre-sorted by their position before
// being tidied up, so the order they were added to the workspace doesn't matter.
const topBlocks = this.workspace.getTopBlocks(true);
const block1 = this.workspace.getBlockById('block1');
const block2 = this.workspace.getBlockById('block2');
const origin = new Blockly.utils.Coordinate(0, 0);
const belowBlock2 = new Blockly.utils.Coordinate(0, 50);
assert.equal(topBlocks.length, 2, 'workspace has two top-level blocks');
assert.deepEqual(
block2.getRelativeToSurfaceXY(),
origin,
'block2 is at origin',
);
assert.deepEqual(
block1.getRelativeToSurfaceXY(),
belowBlock2,
'block1 is below block2',
);
});
test('two overlapping blocks are moved to origin and below', function () {
const blockJson1 = {
'type': 'math_number',
'id': 'block1',
'x': 25,
'y': 15,
'fields': {
'NUM': 123,
},
};
const blockJson2 = {
...blockJson1,
'id': 'block2',
'x': 15.25,
'y': 20.25,
};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const block1 = this.workspace.getBlockById('block1');
const block2 = this.workspace.getBlockById('block2');
const origin = new Blockly.utils.Coordinate(0, 0);
const belowBlock1 = new Blockly.utils.Coordinate(0, 50);
assert.equal(topBlocks.length, 2, 'workspace has two top-level blocks');
assert.deepEqual(
block1.getRelativeToSurfaceXY(),
origin,
'block1 is at origin',
);
assert.deepEqual(
block2.getRelativeToSurfaceXY(),
belowBlock1,
'block2 is below block1',
);
});
test('two overlapping blocks with snapping are moved to grid-aligned positions', function () {
const blockJson1 = {
'type': 'math_number',
'id': 'block1',
'x': 25,
'y': 15,
'fields': {
'NUM': 123,
},
};
const blockJson2 = {
...blockJson1,
'id': 'block2',
'x': 15.25,
'y': 20.25,
};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
this.workspace.getGrid().setSpacing(20);
this.workspace.getGrid().setSnapToGrid(true);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const block1 = this.workspace.getBlockById('block1');
const block2 = this.workspace.getBlockById('block2');
const snappedOffOrigin = new Blockly.utils.Coordinate(10, 10);
const belowBlock1 = new Blockly.utils.Coordinate(10, 70);
assert.equal(topBlocks.length, 2, 'workspace has two top-level blocks');
assert.deepEqual(
block1.getRelativeToSurfaceXY(),
snappedOffOrigin,
'block1 is near origin',
);
assert.deepEqual(
block2.getRelativeToSurfaceXY(),
belowBlock1,
'block2 is below block1',
);
});
test('two overlapping blocks are moved to origin and below including children', function () {
const blockJson1 = {
'type': 'logic_negate',
'id': 'block1',
'x': 10,
'y': 15,
'inputs': {
'BOOL': {
'block': {
'type': 'logic_boolean',
'fields': {
'BOOL': 'TRUE',
},
},
},
},
};
const blockJson2 = {
...blockJson1,
'id': 'block2',
'x': 15.25,
'y': 20.25,
};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const allBlocks = this.workspace.getAllBlocks(false);
const block1 = this.workspace.getBlockById('block1');
const block2 = this.workspace.getBlockById('block2');
const origin = new Blockly.utils.Coordinate(0, 0);
const belowBlock1 = new Blockly.utils.Coordinate(0, 50);
const block1Pos = block1.getRelativeToSurfaceXY();
const block2Pos = block2.getRelativeToSurfaceXY();
const block1ChildPos = block1.getChildren()[0].getRelativeToSurfaceXY();
const block2ChildPos = block2.getChildren()[0].getRelativeToSurfaceXY();
assert.equal(topBlocks.length, 2, 'workspace has two top-level block2');
assert.equal(allBlocks.length, 4, 'workspace has four blocks overall');
assert.deepEqual(block1Pos, origin, 'block1 is at origin');
assert.deepEqual(block2Pos, belowBlock1, 'block2 is below block1');
assert.isAbove(
block1ChildPos.x,
block1Pos.x,
"block1's child is right of it",
);
assert.isBelow(
block1ChildPos.y,
block2Pos.y,
"block1's child is above block 2",
);
assert.isAbove(
block2ChildPos.x,
block2Pos.x,
"block2's child is right of it",
);
assert.isAbove(
block2ChildPos.y,
block1Pos.y,
"block2's child is below block 1",
);
});
test('two large overlapping blocks are moved to origin and below', function () {
const blockJson1 = {
'type': 'controls_repeat_ext',
'id': 'block1',
'x': 10,
'y': 20,
'inputs': {
'TIMES': {
'shadow': {
'type': 'math_number',
'fields': {
'NUM': 10,
},
},
},
'DO': {
'block': {
'type': 'controls_if',
'inputs': {
'IF0': {
'block': {
'type': 'logic_boolean',
'fields': {
'BOOL': 'TRUE',
},
},
},
'DO0': {
'block': {
'type': 'text_print',
'inputs': {
'TEXT': {
'shadow': {
'type': 'text',
'fields': {
'TEXT': 'abc',
},
},
},
},
},
},
},
},
},
},
};
const blockJson2 = {...blockJson1, 'id': 'block2', 'x': 20, 'y': 30};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const block1 = this.workspace.getBlockById('block1');
const block2 = this.workspace.getBlockById('block2');
const origin = new Blockly.utils.Coordinate(0, 0);
const belowBlock1 = new Blockly.utils.Coordinate(0, 144);
assert.equal(topBlocks.length, 2, 'workspace has two top-level blocks');
assert.deepEqual(
block1.getRelativeToSurfaceXY(),
origin,
'block1 is at origin',
);
assert.deepEqual(
block2.getRelativeToSurfaceXY(),
belowBlock1,
'block2 is below block1',
);
});
test('five overlapping blocks are moved in-order as one column', function () {
const blockJson1 = {
'type': 'math_number',
'id': 'block1',
'x': 1,
'y': 2,
'fields': {
'NUM': 123,
},
};
const blockJson2 = {...blockJson1, 'id': 'block2', 'x': 3, 'y': 4};
const blockJson3 = {...blockJson1, 'id': 'block3', 'x': 5, 'y': 6};
const blockJson4 = {...blockJson1, 'id': 'block4', 'x': 7, 'y': 8};
const blockJson5 = {...blockJson1, 'id': 'block5', 'x': 9, 'y': 10};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
Blockly.serialization.blocks.append(blockJson3, this.workspace);
Blockly.serialization.blocks.append(blockJson4, this.workspace);
Blockly.serialization.blocks.append(blockJson5, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const block1Pos = this.workspace
.getBlockById('block1')
.getRelativeToSurfaceXY();
const block2Pos = this.workspace
.getBlockById('block2')
.getRelativeToSurfaceXY();
const block3Pos = this.workspace
.getBlockById('block3')
.getRelativeToSurfaceXY();
const block4Pos = this.workspace
.getBlockById('block4')
.getRelativeToSurfaceXY();
const block5Pos = this.workspace
.getBlockById('block5')
.getRelativeToSurfaceXY();
const origin = new Blockly.utils.Coordinate(0, 0);
assert.equal(topBlocks.length, 5, 'workspace has five top-level blocks');
assert.deepEqual(block1Pos, origin, 'block1 is at origin');
assert.equal(block2Pos.x, 0, 'block2.x is at 0');
assert.equal(block3Pos.x, 0, 'block3.x is at 0');
assert.equal(block4Pos.x, 0, 'block4.x is at 0');
assert.equal(block5Pos.x, 0, 'block5.x is at 0');
assert.isAbove(block2Pos.y, block1Pos.y, 'block2 is below block1');
assert.isAbove(block3Pos.y, block2Pos.y, 'block3 is below block2');
assert.isAbove(block4Pos.y, block3Pos.y, 'block4 is below block3');
assert.isAbove(block5Pos.y, block4Pos.y, 'block5 is below block4');
});
test('single immovable block at (10, 15) is not moved', function () {
const blockJson = {
'type': 'math_number',
'x': 10,
'y': 15,
'movable': false,
'fields': {
'NUM': 123,
},
};
Blockly.serialization.blocks.append(blockJson, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const allBlocks = this.workspace.getAllBlocks(false);
const origPos = new Blockly.utils.Coordinate(10, 15);
assert.equal(topBlocks.length, 1, 'workspace has one top-level block');
assert.equal(allBlocks.length, 1, 'workspace has one block overall');
assert.deepEqual(
topBlocks[0].getRelativeToSurfaceXY(),
origPos,
'block is at (10, 15)',
);
});
test('multiple block types immovable blocks are not moved', function () {
const smallBlockJson = {
'type': 'math_number',
'fields': {
'NUM': 123,
},
};
const largeBlockJson = {
'type': 'controls_repeat_ext',
'inputs': {
'TIMES': {
'shadow': {
'type': 'math_number',
'fields': {
'NUM': 10,
},
},
},
'DO': {
'block': {
'type': 'controls_if',
'inputs': {
'IF0': {
'block': {
'type': 'logic_boolean',
'fields': {
'BOOL': 'TRUE',
},
},
},
'DO0': {
'block': {
'type': 'text_print',
'inputs': {
'TEXT': {
'shadow': {
'type': 'text',
'fields': {
'TEXT': 'abc',
},
},
},
},
},
},
},
},
},
},
};
// Block 1 overlaps block 2 (immovable) from above.
const blockJson1 = {...smallBlockJson, 'id': 'block1', 'x': 1, 'y': 2};
const blockJson2 = {
...smallBlockJson,
'id': 'block2',
'x': 10,
'y': 20,
'movable': false,
};
// Block 3 overlaps block 2 (immovable) from below.
const blockJson3 = {...smallBlockJson, 'id': 'block3', 'x': 2, 'y': 30};
const blockJson4 = {...largeBlockJson, 'id': 'block4', 'x': 3, 'y': 40};
// Block 5 (immovable) will end up overlapping with block 4 since it's large and will be
// moved.
const blockJson5 = {
...smallBlockJson,
'id': 'block5',
'x': 20,
'y': 200,
'movable': false,
};
Blockly.serialization.blocks.append(blockJson1, this.workspace);
Blockly.serialization.blocks.append(blockJson2, this.workspace);
Blockly.serialization.blocks.append(blockJson3, this.workspace);
Blockly.serialization.blocks.append(blockJson4, this.workspace);
Blockly.serialization.blocks.append(blockJson5, this.workspace);
this.workspace.cleanUp();
const topBlocks = this.workspace.getTopBlocks(true);
const block1Rect = this.workspace
.getBlockById('block1')
.getBoundingRectangle();
const block2Rect = this.workspace
.getBlockById('block2')
.getBoundingRectangle();
const block3Rect = this.workspace
.getBlockById('block3')
.getBoundingRectangle();
const block4Rect = this.workspace
.getBlockById('block4')
.getBoundingRectangle();
const block5Rect = this.workspace
.getBlockById('block5')
.getBoundingRectangle();
assert.equal(topBlocks.length, 5, 'workspace has five top-level blocks');
// Check that immovable blocks haven't moved.
assert.equal(block2Rect.left, 10, 'block2.x is at 10');
assert.equal(block2Rect.top, 20, 'block2.y is at 20');
assert.equal(block5Rect.left, 20, 'block5.x is at 20');
assert.equal(block5Rect.top, 200, 'block5.y is at 200');
// Check that movable positions have correctly been left-aligned.
assert.equal(block1Rect.left, 0, 'block1.x is at 0');
assert.equal(block3Rect.left, 0, 'block3.x is at 0');
assert.equal(block4Rect.left, 0, 'block4.x is at 0');
// Block order should be: 2, 1, 3, 5, 4 since 2 and 5 are immovable.
assert.isAbove(block1Rect.top, block2Rect.top, 'block1 is below block2');
assert.isAbove(block3Rect.top, block1Rect.top, 'block3 is below block1');
assert.isAbove(block5Rect.top, block3Rect.top, 'block5 is below block3');
assert.isAbove(block4Rect.top, block5Rect.top, 'block4 is below block5');
// Ensure no blocks intersect (can check in order due to the position verification above).
assert.isFalse(
block2Rect.intersects(block1Rect),
'block2/block1 do not intersect',
);
assert.isFalse(
block1Rect.intersects(block3Rect),
'block1/block3 do not intersect',
);
assert.isFalse(
block3Rect.intersects(block5Rect),
'block3/block5 do not intersect',
);
assert.isFalse(
block5Rect.intersects(block4Rect),
'block5/block4 do not intersect',
);
});
});
suite('Workspace Base class', function () {
testAWorkspace();
});