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

refactor!: Update and modernize the continuous toolbox plugin. #2468

Open
wants to merge 49 commits into
base: rc/v12.0.0
Choose a base branch
from

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Nov 8, 2024

The basics

The details

Resolves

Proposed Changes

This PR updates the continuous toolbox plugin for compatibility with the revised flyout APIs introduced in Blockly v12. It also converts the plugin to Typescript.

Additionally, it fixes several existing issues:

  • Importing the plugin is free of side effects; the plugin must be registered and injected to take effect. Previously, the CSS and some classes were unconditionally registered.
  • The continuous flyout now supports autoclosing; users can simply call Blockly.getMainWorkspace().getFlyout().setAutoClose(true) to enable this behavior.
  • Mutations to procedure blocks in the main workspace will be immediately reflected in the flyout.
  • Extremely poor performance when adding/deleting large numbers of blocks to the workspace has been resolved by debouncing toolbox refreshes in response to block add/create/change events.
  • The implementation has been slightly adjusted to make it more extensible in response to shortcomings identified by the Scratch modernization effort.

This PR should not be merged until Blockly v12 is released, as it depends upon APIs added in that version.

@gonfunko gonfunko requested a review from a team as a code owner November 8, 2024 21:43
@gonfunko gonfunko requested review from cpcallen and removed request for a team November 8, 2024 21:43
@cpcallen
Copy link
Contributor

This PR should not be merged until Blockly v12 is released, as it depends upon APIs added in that version.

Might it be worth creating a v12 branch in this repo (if there is not already one) and retargeting this PR to it?

@gonfunko gonfunko changed the base branch from master to rc/v12.0.0 November 12, 2024 21:54
@gonfunko
Copy link
Contributor Author

This PR should not be merged until Blockly v12 is released, as it depends upon APIs added in that version.

Might it be worth creating a v12 branch in this repo (if there is not already one) and retargeting this PR to it?

Done!

@gonfunko
Copy link
Contributor Author

gonfunko commented Dec 5, 2024

This also fixes #2471.

@gonfunko
Copy link
Contributor Author

gonfunko commented Dec 5, 2024

And also fixes google/blockly#8543

@cwillisf
Copy link

Hello! Any chance this could be built into a beta release or something like that? I'm attempting to use [email protected] but there's no corresponding compatible [email protected]. I can work around that for local dev, but I'm not sure I want to introduce npm link into our CI/CD pipeline even for staging :)

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.

Overall this looks good and the preponderance of the comments/suggestions below are 'for your consideration' rather than requests for change.

Nevertheless, I find myself slightly confused about the recycling business. I think it is worth disambiguationg "recycling bin" from "trash", and maybe including a bit more of a high-level overview of purpose and intended behaviour in relevant places (README, class-level JSDocs, etc.) to explain what's going on, because this was definitely the part of the code that I was least confident about my understanding of.

plugins/continuous-toolbox/src/ContinuousMetrics.ts Outdated Show resolved Hide resolved
plugins/continuous-toolbox/src/ContinuousMetrics.ts Outdated Show resolved Hide resolved
plugins/field-bitmap/src/field-bitmap.ts Show resolved Hide resolved
plugins/content-highlight/src/index.ts Show resolved Hide resolved
plugins/continuous-toolbox/src/ContinuousFlyout.ts Outdated Show resolved Hide resolved
return false;
}
if (field instanceof Blockly.FieldDropdown) {
if (field.isOptionListDynamic()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm mildly concerned about this, since it's possible a FieldDropdown could have dynamically-updated options without actually using a menu generating function. Hopefully it would save/load extra state and be caught that way—or the developer could override isOptionListDynamic to always return true, but I think perhaps this deserves:

  • A general warning in the README (and release notes) about issues with dynamically-modified blocks and fields when using this plugin, and/or
  • A general-purpose way to opt specific block types out of being recycled.

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.

I still think it would be a good idea to provide a bit more internal documentation about the recycling business, and perhaps also make sure the external documentation mentions the issue with non-obviously mutable blocks, but otherwise this LGTM.

Comment on lines +116 to +119
if (
(block.mutationToDom && block.domToMutation) ||
(block.saveExtraState && block.loadExtraState)
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't imagine why a block would get just one of mutationToDom and domToMutation (or saveExtraState and loadExtraState) but not the matching pair, but perhaps safer to just check if any of them exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants