8000 WorkspaceSvg.cleanUp() method doesn't account for immovable blocks · Issue #6889 · google/blockly · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
1 task done
mikeharv opened this issue Mar 9, 2023 · 2 comments · Fixed by #8550
Closed
1 task done

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

mikeharv opened this issue Mar 9, 2023 · 2 comments · Fixed by #8550
Assignees
@BenHenning
Labels
component: coordinates help wanted External contributions actively solicited issue: bug Describes why the code or behaviour is wrong

Comments

@mikeharv
Copy link
Contributor
mikeharv commented Mar 9, 2023 •
edited
Loading

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 act 8000 ually 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

image

Browsers

No response
@mikeharv mikeharv added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels Mar 9, 2023
@mikeharv mikeharv mentioned this issue Mar 9, 2023
Merged
8 tasks
@rachel-fenichel rachel-fenichel added component: coordinates help wanted External contributions actively solicited and removed issue: triage Issues awaiting triage by a Blockly team member labels Mar 15, 2023
@rachel-fenichel
Copy link
Collaborator

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
Copy link
Contributor

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

@BenHenning BenHenning mentioned this issue Aug 21, 2024
Merged
1 task
@BenHenning BenHenning self-assigned this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BenHenning BenHenning

Labels
component: coordinates help wanted External contributions actively solicited issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Ensure immovable blocks are considered during workspace tidying BenHenning/blockly
4 participants
@maribethb @BenHenning @rachel-fenichel @mikeharv
0