feat!: Invalid Blocks

[johnnesky]

The basics

• I validated my changes

The details

Resolves

Fixes #1392 and fixes #1390

It also helps address the blockly-samples issue google/blockly-samples#2035 for core procedures. It'll need to be addressed separately for blockly-samples procedures after this change is published.

Proposed Changes

This adds new methods to blocks similar to isEnabled and setEnabled, called isValid and setInvalidReason. The former are now intended exclusively for user manipulation via the UI, and the latter are intended for automatic processes. The latter supports adding multiple independent reasons why a block should be considered invalid, and the block is only valid if no reasons are present. A block is only included in code generators if it is both enabled and valid.

Reason for Changes

This allows mixing features like having a continue block outside of a loop, an orphan block not attached to a root block, and a user intentionally disabling a block, without any of these features interfering the others. Previously, the disableOrphans could not be combined with the user block disabling feature or with built-in blocks that disable themselves, and the user block disabling feature did not always work well with these built-in blocks because a user could re-enable a block that had been automatically disabled even though the conditions that disabled it haven't changed.

Test Coverage

I updated tests related to automatic disabling of blocks and added similar tests for the control flow block.

Documentation

We should update the codelab at: https://blocklycodelabs.dev/codelabs/validation-and-warnings/index.html

I don't think we have much other documentation of the setEnabled method but maybe we could add some as well as documentation of the new feature at: https://developers.google.com/blockly/guides/create-custom-blocks/define-blocks#per-block_configuration

Additional Information

In the process of testing this, I ran across a couple existing bugs that are not addressed by this PR:
#7950
#7951

Breaking changes / To fix

The block.setEnabled(...) method is now deprecated in favor of block.setDisabledReason(...) which allows you to indicate a reason why the block should or should not be disabled. The block.setEnabled(...) still exists, but only affects the "MANUALLY_DISABLED" reason (which is the same one that the right-click context menu option affects). If a block is disabled for other reasons, such as being detached from a required parent block, then calling the block.setEnabled(...) method cannot fully re-enable the block anymore. You can call getDisabledReasons() to get a list of all of the reasons why a block is currently disabled.

The XML and JSON serialization formats also have deprecations. The XML attribute disabled is now deprecated in favor of disabled-reasons and the JSON property enabled is now deprecated in favor of disabledReasons.

[johnnesky]

There are a bunch of places in the code that look like:

!block.isEnabled() || !block.isValid()

or even:

!block.isEnabled() || !block.isValid() || block.getInheritedDisabledOrInvalid()

It might be worth adding another helper method or two that establishes a simpler pattern for these, but I'm not sure what to name to use, or whether to try combining these methods somehow.

[BeksOmega]

High-level question before I review the rest of this: Instead of having the isEnabled boolean be separate from the isValid reasons, could we just have whether the user has disabled something be one of the reasons?

[johnnesky]

Yes! We would just have to decide where we want to draw line on backward compatibility. Do we still keep the isEnabled and setEnabled methods (in which case they would be internally implemented as setting an invalid reason)? Do we keep the XML and JSON attributes related to disabling blocks? If we completely remove the old isEnabled system, should the new system continue to be called isValid or should it be renamed to take the place of isEnabled?

Of course, the least disruptive thing to do would be to keep the old isEnabled API even if we implement it differently.

[BeksOmega]

Of course, the least disruptive thing to do would be to keep the old isEnabled API even if we implement it differently.

I think it's actually disruptive either way, which is good for us! We're changing the semantics of setEnabled, such that it's not the only thing that controls whether a block is enabled or not. And that's going to be disruptive no matter what.

Do we keep the XML and JSON attributes related to disabling blocks?

Mmm the serialization is something else I'm a bit concerned about with this proposal. I don't want to make our saves larger if we don't have to. E.g. the behavior of the disable orphans change listener is completely deterministic based on other state of the workspace, and shouldn't need to be saved separately.

If we used an overridable function instead of the invalid reasons map, we wouldn't have to worry about serialization.

I know we talked about using an overridable function, but can't remember what the problem was with that. Do you have context?

[johnnesky]

The main reason why I ended up not going with an overridable function is because I couldn't figure out a system to make sure that the function actually gets called whenever you need it to be called. I guess the simplest thing to do would be to call it on all blocks whenever there's any change, but that felt like it might be too heavy. Aside from that, how do you know whether something changed that affects whether a block should be invalid, especially for plugins which may have implied dependencies we couldn't predict?

In the core library, procedure call blocks are an example of an indirect dependency: they become valid or invalid depending on changes to a different block with no direct connection between the two. How would we make sure that the validity status of call blocks gets updated when the procedure definition block is modified? I mean, we could add a special case for that, similar to our existing event listeners, but then plugin authors would have to deduce whether their dependency is one that's already covered by the default behavior or something that they too will have to special-case and possibly getting confused when they expect one but get the other. I figured it's better to just keep the existing paradigm of writing your own event listeners that explicitly encode the dependencies.

On a side note, there is a way to automatically update things based on implied dependencies that works reasonably well: data binding aka signals: https://dev.to/this-is-learning/the-evolution-of-signals-in-javascript-8ob
But it would be messy to migrate Blockly to something like this.

[BeksOmega]

The main reason why I ended up not going with an overridable function is because I couldn't figure out a system to make sure that the function actually gets called whenever you need it to be called.

Sweet sweet, makes sense :D Thank you for the context!

[BeksOmega]

Ok how about this: I think it makes sense to just have the one set of Enabled methods instead of also having Valid methods.

I don't think unifying them causes any more behavioral changes than keeping them separate. Either way, the "disabledness" of a block is dependent on more things than just a single boolean, so someone's behavior could change if they were previously using setEnabled to override an invalidation that had a different reason. E.g. if they were using setEnabled to enable a break-continue block outside of a loop, that wouldn't work anymore.

In this proposal, setEnabled would have the same logic as your current setInvalidReason. We can make the reason optional if we want so that for the majority of cases, this wouldn't affect external devs. (excepting things like the break-continue example from earlier)

Wrt serialization, I think it makes sense to continue deserializing the enabled attributes. But we can stop serializing them and use an array of reasons instead.

Does that make sense? And how does that sound to you?

[johnnesky]

I've attempted to merge the old and new systems in a way that's not too disruptive. The isEnabled method returns true if there aren't any disabled reasons. The setEnabled method is now deprecated, but is still operational and internally manipulates the MANUALLY_DISABLED reason. The replacement is the new setDisabledReason method, along with some helper methods hasDisabledReason and getDisabledReasons.

There was a private "disabled" property that was nevertheless in use in various places. I updated the references I found in the core library, demos, and tests. I also replaced the property with private getter and setter methods to manipulate the MANUALLY_DISABLED reason, which are also marked as deprecated.

[BeksOmega]

@johnnesky Stop working while you're OOO :P

[BeksOmega]

This looks great John =) Thank you for your work on this! And for adding tests :DD Just a few nits.

[BeksOmega]

@maribethb We're planning to continue supporting cloud storage for the code demo right?

[BeksOmega]

LGTM! Make whatever decision you want wrt the event param and then you can go ahead and merge! (assuming you still have perms, if not lmk)