fix: Ensure immovable blocks are considered during workspace tidying

[BenHenning]

The basics

• [x] I validated my changes

The details

Resolves

Fixes #6889

Proposed Changes

This PR introduces a fix for #6889 (WorkspaceSvg.cleanUp) when a workspace contains an immovable block. Previously, immovable blocks were ignored in cleanUp which could lead to situations where movable blocks were stacked on top of the immovable blocks. The updated algorithm addresses this.

The old algorithm was essentially the following steps:

• Sort top-level blocks first by their y value then by their x (per getTopBlocks and Workspace's sorting logic).
• Position the first movable block at (0, 0).
• For each subsequent block:
  • If it's immovable, skip it.
  • If it's movable, position it at the previous block's y coordinate plus its height plus a minimum height spacing defined by the renderer's MIN_BLOCK_HEIGHT property. Also, left-align the block at an x value of 0.

The updated algorithm augments this by considering any immovable blocks that the new block's position might intersect with (by using Rect's intersects method), as so:

• Sort the top-level blocks in the same way before.
• Split the list of blocks into movable and immovable blocks.
• For each immovable block, compute its bounds Rect (to be used later).
• For each movable block, in order, compute its new position:
  • If it's the first block, assume a position of (0, 0). If not, assume it to be (0, y) where y is the previous block's position plus the previous block's height plus the minimum block spacing (as described above).
  • Enumerate all immovable block bounds and verify whether the block's new position would intersect with any of them. If it would, recompute y in the same way as the previous step except use the conflicting immovable block's position and size, rather than the previous movable block.
  • Repeat the last step until the new position no longer conflicts with any immovable blocks.

NOTE:

~This PR is also renaming WorkspaceSvg.cleanUp to WorkspaceSvg.tidyUp since 'clean up' is an overloaded term in the codebase and generally means "deinitialize." This PR does not rename the context menu item since cleaning up the workspace has a different context for the user and probably makes equal sense to 'tidy' (so there wasn't an obvious reason to change it other than to keep the two aligned).~

This has been reverted after a discussion with the team--the preference is to avoid unnecessary code changes and this rename has few benefits.

Reason for Changes

Trying to tidy up the workspace without considering immovable blocks leads to undesirable results. See previous behavior:

Screen recording 2024-08-21 1.42.28 PM.webm

With the changes, the new behavior is:

Screen recording 2024-08-21 1.40.37 PM.webm

This is a much better user experience. Note that the above was demoed by importing the following JSON into a local Blockly playground:

{
  "blocks": {
    "languageVersion": 0,
    "blocks": [
      {
        "type": "math_number",
        "id": "block1",
        "x": 1,
        "y": 2,
        "fields": {
          "NUM": 1
        }
      },
      {
        "type": "math_number",
        "id": "block2",
        "x": 10,
        "y": 20,
        "movable": false,
        "fields": {
          "NUM": 2
        }
      },
      {
        "type": "math_number",
        "id": "block3",
        "x": 2,
        "y": 30,
        "fields": {
          "NUM": 3
        }
      },
      {
        "type": "controls_repeat_ext",
        "id": "block4",
        "x": 3,
        "y": 40,
        "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"
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      },
      {
        "type": "math_number",
        "id": "block5",
        "x": 20,
        "y": 200,
        "movable": false,
        "fields": {
          "NUM": 5
        }
      }
    ]
  }
}

Test Coverage

cleanUp had no functional tests, so a bunch were added (including for verifying existing behaviors). All of this function's new tests passed without the algorithmic changes except for the test "multiple block types immovable blocks are not moved" (which was verified to fail, as expected, without the algorithmic fixes).

Separately, Rect had some additional functionality and documentation added, along with an entire new test suite meant to thoroughly test all aspects of the class (since it plays a pivotal role in the new functionality, plus a bunch of other existing functionality throughout Blockly core).

Documentation

It doesn't seem any documentation changes are needed at this time since the fix was entirely infrastructural and implementation-specific. Since this is a public API function, it's certainly possible some documentation work will be needed.

Additional Information

The needs of cleanUp functional changes and its tests led to needing a bounding box class. This was added before Rect was discovered, and its intersection algorithm was independently derived. Rect's own intersection code is actually replaced in this PR with the derived version (the same logic, just inverted) with some documentation to logically explain why it works to help any future readers who come across it.

Separately, the workspace tests prefer to use JSON for block initialization vs. direct API access. This seems to be the recommended way to approach such test condition arrangement after discussions with other Blockly team members.

[BenHenning]

Looks like this is now ready for review.

[BenHenning]

NB: Comments should be limited to 80 characters (ES lint doesn't catch this).

[BenHenning]

@gonfunko could you PTAL per latest changes and let me know if the PR still looks good to you?

[BenHenning]

Thanks @gonfunko!

[mikeharv]

Thanks so much for this!