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: fix block factory in manual mode #6533

Merged
merged 1 commit into from Oct 12, 2022
Merged

Conversation

maribethb
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

fixes #6175 (and then the other problems with block factory in manual mode)

Proposed Changes

  1. Updates reference to a moved function (Blockly.hueToRgb hasn't existed for a while)
  2. Updates the block factory to not allow you to create blocks that share a name with an existing block.
  3. Don't actually delete all blocks from Blockly.Blocks, because this breaks in manual mode. Instead, get the name of the block from either the json or js definition, and then only delete that new block if it was actually created to begin with.

Behavior Before Change

Block factory is broken in manual mode.

  1. Open block factory demo
  2. Change the format to Manual JSON
  3. Try to change anything about the block, such as the color
  4. Note the console error about no block definition for factory_base and the block disappears from the main workspace

Behavior After Change

  • Above steps work as expected in manual mode
  • If you try to create a block called, e.g. factory_base you get a console error and warning text on the block.

Reason for Changes

Manual mode broke in #5571
The reason is we're removing all the blocks from Blockly.Blocks during the execution of BlockFactory.updatePreview. In normal mode that's fine, but in manual mode the following happens:

  1. BlockFactory.updateBlocksFlag is true due to the event listener on the text area
  2. updatePreview deletes all the blocks from Blockly.Blocks so that we can detect the new one that's added.
  3. updatePreview calls BlockFactory.updateGenerator
  4. which calls FactoryUtils.getGeneratorStub
  5. because the BlockFactory.updateBlocksFlag is true, the main workspace of block factory is rebuilt. basically, this is the step that updates the block factory workspace to be in sync with the text area now that we're editing the text area directly. Why is that buried in this generator function, I don't know. but while rebuilding, we need to have the definition of factory_base and other blocks available. that definition is not available because we've just deleted everything from Blockly.Blocks, so that causes the error about missing definition for factory_base.

Even if you check out a commit from before #5571 such as 5f72a61b2dba14ec86c8a128060cfb38c29cde00 things didn't really work that well before if you used the same name as an existing block. (Note if you actually try this, you'll have to fix the color thing and then probably run npm run build:compressed and npm run checkin:built). Because we follow all the same steps above, but instead of factory_base not existing, you've overwritten the definition of factory_base so things get weird when we try to serialize that block into the main workspace. This is despite the comments in updatePreview that try to claim things will be fine if a user picks an existing name.

That's why I just decided to not let them use existing names. Is it the best experience? no. but it's good enough and it's better than it used to be.

This probably isn't the ideal solution but the existing code is a bit of a mess. The problem comes from there not being a clear source of truth for the block definition. Is it the main workspace, or the text area? it depends on if you are in manual mode or not, but it isn't handled cleanly in both cases. Ideally we'd know which one is the source of truth and be able to update the other one cleanly, but I didn't really feel like re-architecting all of block factory, so I slapped this bandaid on instead.

Test Coverage

Documentation

Additional Information

Me: sweet #6175 should take me 30 seconds to fix
Me 4 hours later: T_T

@maribethb maribethb requested a review from a team as a code owner October 12, 2022 02:59
@github-actions github-actions bot added the PR: fix Fixes a bug label Oct 12, 2022
@maribethb maribethb merged commit a64d6e9 into google:develop Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing function Blockly.hueToRgb
2 participants