Enforce connection preconditions for setParent #4999
Conversation
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Error now thrown when calling `a.setParent(null)` if a is connected to a superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fixed lint errors.
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fixed lint errors. * Adjusted comment.
|
Hi jschanker, thanks for posting this! I'm just about to head out for today, so I'll get a review back to you on Monday =) Also sorry about creating all of those linked issues when I was testing some automations stuff (whoops) I'll see if I can get those cleaned up. |
There was a problem hiding this comment.
Thanks for putting up this fix :D I think it's good to enforce these changes programmatically in addition to fixing the visibility.
I think the only thing this change really needs is some tests. You can either choose to add them, or file an issue so that someone can add them later. I think there are 5 cases that need to be covered:
- Properly setting the parent to null
- Properly setting the parent to a new not-null parent
- Cases for each of your error situations
But if you think of others that's rad.
The best place to add these tests is probably in the block_test.js mocha file. I can't remember if you've written tests before, so here are the mocha docs, and the chai docs just in case. You will probably want to assert equal, throws, and maybe doesNotThrow.
Thank you again for sending this in! The code looks great :D
* One is failing (commented out), will investigate later
Happy to help!
Absolutely! Added the tests.
I have for #4876, and I hope it's okay that I cleaned up the ones I wrote in this pull request (creating the workspace in the setup and tearing it down upon completion).
My pleasure again and thanks for reviewing! |
There was a problem hiding this comment.
Thanks for adding tests, they look great! Just a few formatting changes, then should be gtg :D I think there should be some UI for you to commit them automagically.
I hope it's okay that I cleaned up the ones I wrote in this pull request (creating the workspace in the setup and tearing it down upon completion).
Oh yeah definitely! In my opinion it's way better to clean up code than to worry about being sticklery on PRs.
Capitalize first letter, period at end Co-authored-by: Beka Westberg <bekawestberg@gmail.com>
|
Great, made the formatting changes. Thanks again! |
The basics
The details
Resolves #4989
Proposed Changes
Throws error when attempting to set a block's parent to a non-null block
bifbisnotproperly connected to it without changing state (i.e., if its previous/output connection is not connected to one ofb's input/next connections or when it doesn't have such connections). Also throws error when attempting to set a block's parent tonullif it has a connection to a superior block.Behavior Before Change
Setting a parent of a block already connected to some other superior block would remove the block from that old parent's child list before throwing, leading to potential rendering issues. Setting the parent of a disconnected block could cause rendering issues and errors when attempting to delete that parent.
Behavior After Change
Errors are thrown in these cases without changing state so none of these problems ensue.
Reason for Changes
To enforce the desired precondition for
setParentthat a non-nullinputted parent should already be a superior block via the proper connections and that a block should not be connected to a superior one when attempting to set its new parent tonull.Test Coverage
Tested on Blockly Playground to make sure that it resolved the desired bugs.
Documentation
No additional documentation required.