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.

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?

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.

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?

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.

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!

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?

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.

@johnnesky Stop working while you're OOO :P