fix: bump neighbours performance regression

[BeksOmega]

The basics

• I validated my changes

The details

Resolves

N/A

Proposed Changes

Makes it so that bumpNeighbours is called as part of the rendering cycle.

Reason for Changes

1. You no longer have to remember to call bumpNeighbours because it just happens whenever the block changes shape.
2. Alleviates a performance regression caused by fix!: rendered meaning #7747 . The regression was that bumpNeighbours was being called on every single block initialization action during deserialization, which meant a ton of setTimeout calls, which were slowing everything down. Making bumpNeighbours part of serialization gets rid of these setTimeout calls.

Test Coverage

Manually tested.
Manually profiled performance.
Also covered by some existing unit tests (caught the issue with event grouping!)

Documentation

N/A

Additional Information

Dependent on #7747

[maribethb]

this feels suspicious to me... if we're telling external users they don't need to call bumpNeighbors directly, we shouldn't either when we're creating a new block in this manner.

I don't think we need it anyway. createBlock uses json serialization to append new block to the workspace. append will eventually queue a render on the block, which will now call these two things. so calling them here should be redundant, I think.

[BeksOmega]

Ah you're right, this is actually a good case for someone calling this externally, so I've undeprecated it.

The problem here is that createBlock forces an immediate render (because we always force an immediate render when deserializing). But at the time the render is triggered, the blockj isn't guaranteed to be in the correct place, so the bump isn't guaranteed to be correct.

This would be fixed if createBlock just deserialized at the correct spot, but since it's overridable, that's not possible to guarantee.

It would also be possible to trigger a bumping after every block move, but this could easily break external developers' functions for "cleaning up" the workspace. I believe it would even break our built-in context menu option for cleanup, because we space blocks based on the minimum block height, not the bump distance.

So our only option is to snap to the grid after deserializing, and then trigger the bump manually.

[maribethb]

createBlock is currently internal, which... is one of the ones that is incorrect because it's also part of the IFlyout interface. But I think it might be fair to fix this in our implementation of createBlock and then add it as a requirement that anyone overriding createBlock needs to take into account? Because this is going into a major I feel comfortable adding that new constraint. We could even fix it in placeNewBlock and/or positionNewBlock and then make those methods protected instead of private, so that subclasses of Flyout could use them and use our fixed logic, making it pretty easy on the developer to get this right.

By "just deserialized at the correct spot" do you mean we'd calculate the position of where the block should go before deserializing (so add the correct coordinates to the json instead of calling moveTo afterwards)? If so I think the approach above could make sense.

Otherwise or if that actually doesn't make sense, then leaving it undeprecated makes sense as well.

[BeksOmega]

So the specific case for createBlock could be worked around by making a breaking change, but I still don't think it's fixable in general.

Folks could still want to move blocks programmatically, and then trigger bumpin neighbours on them, and that requires the bumpNeighbours method to be public.

I don't think making the break in createBlock makes sense given that, but will await your opinions.

[maribethb]

Okay, that makes sense too. If we can't trigger it on every block move then it would need to be public regardless. I'm fine with leaving createBlock alone then.

[BeksOmega]

Found an issue with insertion markers causing bumping that I have a few solutions for, but no clear winner. I'm going to let it sit in my brain over the long weekend and then come back to this tuesday.

[maribethb]

Unfortunately we use this in a plugin :/ This also seems like a likely function that people would be using even though it's marked internal. How would you feel about deprecating it instead and removing it at a future date?

[BeksOmega]

How do you feel about making scheduleSnapAndBump public instead? Since we already established people may want to bump neighbours after programatically moving their blocks (makes sense they'd want to snap too).

[maribethb]

That sounds great!

[maribethb]

But I think the comment may be inaccurate now, there's not a delay inherent to this other than just the rendering queue. right?