-
Notifications
You must be signed in to change notification settings - Fork 13
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
Plugin for Selecting & Dragging & Doing Actions on Multiple Blocks #1
Conversation
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is quite large, so I'm going to have to review it in stages =) This is the first pass.
So far, looks really good! Thank you so much for your hard work on this :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second Pass!
src/dragger.js
Outdated
}); | ||
if (this.blockDraggers_.size > 1) { | ||
// Restore the highlighting around connection for multiple blocks. | ||
Blockly.RenderedConnection.prototype.highlight = origHighlight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to find a way around this that doesn't involve monkey patching the prototype, but I'll have to think on it haha. @BeksOmega for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can luckily fix the bug, then that monkey patch is no longer needed.
The bug is that, when you finished connecting multiple blocks at the same time, only one of the highlighted connection path that appear during the multiple drag can get deleted and others will still remain:
So the patch here is just a work around, it disables showing the highlight in a multiple drag.
Currently I have no idea on how to fix that, may be some upstream patches will be necessary I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that that is an upstream bug that needs to be fixed. Will file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed: google/blockly#6273
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I don't see a way around these monkey patches without fixes in core. @cpcallen , will module sealing break these patches? or will they be fine and we can leave them for now?
Signed-off-by: Hollow Man <[email protected]>
Hi @BeksOmega ! Thanks so much for reviewing, I have committed all the resolved code changes for the Pass 1 & 2 in a85fc79 In addition, I just disabled bumping neighbours and fixed a Bug 92234e7 The reason for disabling bumping is that, I noticed that sometimes, for multiple selection, bumping neighbours will lead some selected blocks' surrounding highlight disappear (while still selected) after moving, and I also dislike bumping neighbours in multiple selection mode as those unpredictable movements can make the workspace really messy. The fixed random bug is that, when you drag the selected children blocks from their unselected parent, the children blocks of the selected ones can be out of the position while still connected after the disconnection (the selected blocks' children do should remain connected, but they shouldn't be out of the position). |
…n when dragging Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass 3! And good catch on that bumping bug =) Haven't had time to investigate ways around all of the prototype
assignments yet.
src/index.js
Outdated
this.justDeletedBlock_.pathObject.updateSelected(false); | ||
this.justDeletedBlock_ = null; | ||
} | ||
// Update the selection highlight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to update the selection highlight in bulk if you're updating individual blocks individual blocks as they're selected/unselected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may still want to do this here, otherwise such as in the multiple selection shortcut copy and paste, it's not easy to keep all those selected. Maybe it can be optimized later, but currently we just keep the original as it's an easy way to avoid bugs and it seems like that it won't impact the performance.
Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass 4!
src/dragger.js
Outdated
}); | ||
if (this.blockDraggers_.size > 1) { | ||
// Restore the highlighting around connection for multiple blocks. | ||
Blockly.RenderedConnection.prototype.highlight = origHighlight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that that is an upstream bug that needs to be fixed. Will file.
We should verify this. App Inventor uses a different rendering algorithm than base Blockly. We haven't tested whether the new render pipeline performs similarly without the optimizations we've added.
…Sent from my iPhone
On Jul 14, 2022, at 20:12, Hollow Man ***@***.***> wrote:
@HollowMan6 commented on this pull request.
In src/patch.js:
> +};
+
+/**
+ * Fix when you drag the selected children blocks from their unselected parent
+ * the children blocks of the selected ones can be out of the position while
+ * still connected (do should remain connected).
+ * @OverRide
+ */
+Blockly.BlockDragger.prototype.updateBlockAfterMove_ = (function(func) {
+ if (func.isWrapped) {
+ return func;
+ } else {
+ const wrappedFunc = function(delta) {
+ func.call(this, delta);
+ this.draggingBlock_.render();
+ this.draggingBlock_.getDescendants(false).forEach(function(block) {
Okay, didn't aware that each time you render a block it rerenders all of that block's parents as well. So will check if the blocks have children, only re-render when they don't. Also will add this to the comment.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
Okay, then will add a |
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
General JavaScript style note: Usually if a file only contains a single class, the name of the class and the name of the file are the same. E.g. if a file exports a single class |
Signed-off-by: Hollow Man <[email protected]>
Okay, just reorganized the code accordingly. |
Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think with this I've been about as helpful as I can be =)
The monkey patches included will definitely require maintenance (the core team does not guarantee they will continue to work). But aside from that, I think this is about as stable as one can make it.
@ewpatton you might want to have someone on your team take a look as well before merging.
Signed-off-by: Hollow Man <[email protected]>
Thanks so much for helping me review my code all the way around! |
Absolutely! Reviewing PRs is one of my favorite things =) I can't wait to see this go up on NPM! |
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good @HollowMan6. I've left some comments for you to think about.
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
Signed-off-by: Hollow Man <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HollowMan6 I think this is good for a first pass. We're going to merge this and deploy this first version to npm so people can start trying it out. @ellelili2025 will coordinate next steps for integration into the latest version of App Inventor with Blockly 8.
User behavior
bumpNeighbours: true
.Duplicate
menu will duplicate the selected most top block in the block stack and all the children blocks of that most top block. The selection will be changed to all newly created duplicate blocks' most top blocks.Add Comment
/Remove Comment
will instead read asAdd Comment (X)
/Remove Comment (X)
, and will add / remove comment buttons to all the selected blocks.Inline Inputs
/External Inputs
will instead read asInline Inputs (X)
/External Inputs (X)
, and will convert the input format with all the selected blocks.Collapse Block
/Expand Block
will instead read asCollapse Block (X)
/Expand Block (X)
, and will only apply to the selected most top block in the block stack.Disable Block
/Enable Block
will instead read asDisable Block (X)
/Enable Block (X)
, and will only apply to the selected most top block in the block stack. All the children blocks of that most top block will also get disabled.X
means the currently applicable number of selected blocks by the user, and(X)
will only be shown when X is greater than 1.Delete [X] Blocks
represents the count of the selected most top block in the block stack as well as all children of those selected most top block, and delete the blocks mentioned.Select all Blocks
in that workspace.Ctrl/Alt + A
, you can select all the blocks in the current workspace.Ctrl/Alt + C
to copy the selected blocks,Ctrl/Alt + X
to cut the selected blocks to the clipboard, andCtrl/Alt + V
to paste all the blocks currently in the clipboard and get all the newly pasted blocks selected, these will only apply to the selected most top block in the block stack.useDoubleClick: true
.Note: This plugin is part of the achievement by Songlin Jiang(@HollowMan6) participating the Google Summer of Code 2022 at MIT App Inventor.