Skip to content
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.

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

fix: Call blocks handle both manual disabling and disabled defs #2334

Merged
merged 2 commits into from
May 3, 2024

Conversation

johnnesky
Copy link
Member

@johnnesky johnnesky commented Apr 20, 2024

The basics

The details

Resolves

Fixes #2035

Proposed Changes

google/blockly#7958 makes the core Blockly system for disabling blocks more advanced, allowing a block to be simultaneously disabled for multiple independent reasons (such as manually disabling from the context menu, or being disabled because the block is not valid). Removing one of these reasons doesn't affect the others, and the block as a whole is only considered to be enabled if it doesn't have any disabled reasons. This PR takes advantage of that system to allow shared procedure call blocks to have a unique reason for being disabled when the corresponding procedure model is disabled.

Also, I updated all plugins to depend on the latest blockly beta: 11.0.0-beta.9.

Reason for Changes

Previously, if the user manually disabled a procedure call block, then edited the name of the procedure, the call block would be re-enabled (because the call block was updated to match the procedure model). Also, a call block that had been disabled by the model could be manually re-enabled.

Test Coverage

I tested manually. (with npm run start in the plugin directory.) All bugs I'm aware of have been fixed.

There are existing tests to ensure that call blocks get disabled/enabled when the corresponding def is changed, and these still pass. I don't think there are any tests related to disabling from the context menu though.

Documentation

N/A

Additional Information

In procedureDefUpdateShapeMixin.doProcedureUpdate(), there's the following line:

    this.setEnabled(this.getProcedureModel().getEnabled());

This is using the deprecated Block method setEnabled which only updates the MANUALLY_DISABLED reason. If the def block is disabled for other reasons, then the procedure model can't fully enable the def block. This sounds like an unlikely scenario (what else would be disabling a def block? what would cause the model to become enabled when the def block is disabled?) but if we want to robustly handle this situation, then the procedure model should probably fully represent all of the procedure's disabled reasons, instead of just a boolean indicating whether there are any disabled reasons, so that it can properly sync the def with the model.

@johnnesky johnnesky requested a review from a team as a code owner April 20, 2024 00:36
@johnnesky johnnesky requested review from cpcallen and removed request for a team April 20, 2024 00:36
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re:

In procedureDefUpdateShapeMixin.doProcedureUpdate(), there's the following line:

    this.setEnabled(this.getProcedureModel().getEnabled());

This is using the deprecated Block method setEnabled which only updates the MANUALLY_DISABLED reason. If the def block is disabled for other reasons, then the procedure model can't fully enable the def block…

This seems reasonable enough to me but I don't think I have enough knowledge of this part of the codebase to be able to say for sure. Perhaps @BeksOmega could weigh in?

@BeksOmega BeksOmega self-requested a review April 29, 2024 15:27
@BeksOmega BeksOmega self-assigned this Apr 29, 2024
Comment on lines +1097 to +1100
this.setDisabledReason(
!this.getProcedureModel().getEnabled(),
PROCEDURE_MODEL_DISABLED_REASON,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesss this code is so much more elegant, and it actually works!

@johnnesky johnnesky merged commit 5eade55 into google:rc/v11.0.0 May 3, 2024
11 checks passed
@johnnesky johnnesky deleted the nesky_invalid_procedures branch May 3, 2024 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants