Upgrade multiselect plugin to use an IDragger (for Blockly 11)

[HollowMan6]

Currently the multiselect plugin uses a custom block dragger to handle dragging multiple blocks at once. This is bad because it doesn’t allow you to select workspace comments as part of a multiselection. In Blockly 11, the upstream will make some changes to how the dragging in Blockly works. This would require some breaking changes, which would require fixes to the multi-select plugin. But after it's implemented we shouldn't have to monkey patch anymore! Which should make the plugin easier to maintain in the future.

Implementation

Custom Draggable

The multiselect plugin can implement a custom IDraggable that wraps all of the selected elements. It can delegate to drag operations to the individual elements.

class MultiselectDraggable implements IDraggable {
  private subDraggables: IDraggable[];

  addSubDraggable() { /* Implementation */ }

  startDrag(e: PointerEvent) {
    for (const draggable of this.subDraggables) {
      draggable.startDrag(e);
    }
  }

  drag(newLoc: Coordinate, target: IDragTarget, e?: PointerEvent) {
    for (const draggable of this.subDraggables) {
      draggable.drag(Blockly.Coordinate.sum(newLoc, draggable.dragOffset), target, e);
    }
  }

  endDrag(e: PointerEvent) {
    for (const draggable of this.subDraggables) {
      draggable.endDrag(e);
    }
  }
}

This implementation also means that the multiselect plugin won’t have to do any special work to be compatible with the scroll options plugin, since the scroll options will override the IDragger while this creates a custom IDraggable.

Selection

Currently, the multiselect plugin handles selection by passing a random one of the selected blocks to Blockly.common.setSelected. Instead, it will need to pass the MultiselectDraggable to setSelected so the Gesture can properly pass it to the IDragger.

Related issue: google/blockly-samples#2317

[HollowMan6]

The beta version of v11 (v11.0.0-beta.8) has included the IDragger feature. Some docs to help with migration:

• Draggables: https://developers.google.com/blockly/guides/configure/web/dragging/draggable
  This is the kind of object to implement to represent a multiselection.
• Draggers: https://developers.google.com/blockly/guides/configure/web/dragging/dragger
  These handle calculating the workspace location where draggables should move to. We shouldn't need to implement one, but it might be useful to read this for context.
• Copy paste: https://developers.google.com/blockly/guides/configure/web/copy-paste
  Since we'll be using a custom draggable, we'll need to handle copy-paste ourselves. We should be able to delegate to the pasters, so it shouldn't be too much lift.

[changminbark]

Hi everyone,

My name is Chang Min Bark and I will be the GSoC contributor in charge of upgrading the multiselect plugin. I am also in charge of cross-testing the mutliselect plugin with other Blockly plugins. I am looking forward to contributing to this community!

Thanks,
Chang Min

[changminbark]

Hello,

I made a few changes and wanted to test them out but when I run "npm start" it does not show anything. I do not even see the error messages. I ran the script without my changes and it also showed a blank white page (so I don't think it is a result of my changes). Is there something I can do to see the error messages?

Thanks,
Chang Min

[HollowMan6]

Copied from our last email, the bold and italic part in the first entry answers your question. Also, check the screenshots, you need to use IDraggable to replace that so we can fix and get the plugin working on Blockly v11.

• Now the only difference between the blockly-v11 and the main is this commit 11b76b8, which is just a version change done by me to get you started. I haven’t done anything related to actual code for IDraggable, so that branch is actually still broken and you will see a blank page with errors in the developer tools console (F12) once you do npm start. It will be your job to implement IDraggable so that we can fix the Blockly v11 support. You can start your implementation / fix guided by those errors as well as the documentation I mentioned in my first email/issue on GitHub Upgrade multiselect plugin to use an IDragger (for Blockly 11) #39.
• I don’t expect “@blockly/dev-tools” and any other possible conflict dependencies to explicitly indicate support for Blockly v11 now, as v11 is still in beta. But it looks fine to forcibly override that at this stage. Feel free to upgrade the version at any time later once you find they have the support.
• You are allowed to do whatever you want at this stage in the codebase without the need to ask for permission (be bold and creative), as long as it doesn’t cause any regression. Once you submit the PR, I will review your code and let you know the best practice/alternative way if I find any problems, so relax and explore!
• As another first step during the community bonding period, I would encourage you to try out the main branch as well to learn how things are supposed to work (since the blockly-v11 branch is broken, you won’t learn anything there). It will benefit you a lot after you finish the IDraggable implementation to make sure no regression/bug is introduced, as well as for your second cross-testing task. A possible checklist at https://github.com/mit-cml/workspace-multiselect?tab=readme-ov-file#user-behavior or you might want to experience the features I mentioned in my talk last year https://github.com/HollowMan6/My-Google-Summer-of-Code/blob/main/Slide_Songlin_Multi-Select_Plugin_2023_Blockly_Summit.pptx

[changminbark]

For the IDragger component, we would just need to import one without reimplementing it as the default one should work with the MultiselectDraggable that implements the IDraggable interface. Am I correct?

[HollowMan6]

Ah yes, sorry for the confusion I caused in my previous emails. I should call them IDraggable. Just correct them in my last comment.

[changminbark]

Hello,

I just had a question regarding the implementation of the IDraggable interface. Since the previous code was written in JavaScript and implementing an interface is a feature of TypeScript, should I write the code for the IDraggable implementation of the multi-select draggable in TypeScript? If so, do I have to install and configure the Webpack bundler for TypeScript?

[HollowMan6]

Good question! Yes, and if you feel like doing so, you can help convert the whole plugin into Typescript. I don't think you need to configure the Webpack bundler for TypeScript as the bundler is configured by @blockly/dev-scripts and it should already have the ability to handle Typescript, you can check one of the Typescript based plugin for reference https://github.com/google/blockly-samples/tree/master/plugins/block-dynamic-connection

[changminbark]

The main reason I asked is because when I tried importing the new class I created that implemented the IDraggable interface, I was encountering an error related to the code being in Typescript.

[HollowMan6]

I haven't investigated how to mix Javascript with Typescript in the Blockly plugin, you can check block-dynamic-connection to see how to use Typescript in the plugin. We can discuss it today later during the meeting if you can't get it fixed.

My guess is that you may need a tsconfig.json file https://github.com/google/blockly-samples/blob/master/plugins/block-dynamic-connection/tsconfig.json

[changminbark]

After looking at the main branch (original one that you worked on), I am thinking of creating a typescript declared class called MultiDraggable that implements the IDraggable interface. Then, I was planning to create the MultiselectDraggable class that extends the MultiDraggable declared class that I created. The reason for this is that I cannot directly implement the IDraggable interface (typescript) in the MultiselectDraggable class (javascript). What do you think about this approach?

[HollowMan6]

Sounds good to me as long as it works. It's fine to use your approach if you find something that is not doable in Typescript, but I guess it's better to directly have MultiselectDraggable class implements the IDraggable interface in Typescript (so that we can skip MultiDraggable).

[changminbark]

I guess that if I want to skip the MultiDraggable, I would have to use some sort of tsc to compile the typescript into javascript. However, if I want to avoid that, I would have to go with my method. I will decide whether it would be better to use tsc or just use javascript as I start to develop this more.

I also have another question about the skeleton code that you provided in the original implementation description. In the "drag" method, you have a target parameter. However, the IDraggable interface does not have this, so the IDE is giving me an incompatible override message. What was your intention with this parameter? Was it to check whether the blocks being dragged can be connected to an existing block in the workspace?

[HollowMan6]

That code snippet was taken from the Blockly team's original design document, maybe later they found that unnecessary and removed that parameter in the beta version. Anyway, you can safely ignore that target parameter as now they don't have that.

[mark-friedman]

I cannot directly implement the IDraggable interface (typescript) in the MultiselectDraggable class (javascript).

AFAIK, you should be able to just create a JavaScript class which satisfies the TypeScript interface and avoid using any TypeScript. See, for example, this section of the Blockly docs, which Songlin previously mentioned. What issues were you seeing?

[mark-friedman]

The key, I think, is that you want your class to extend the Blockly interface.

[changminbark]

@mark-friedman @HollowMan6
I have tried extending the Blockly interface but it does not allow me to extend an interface. That's why I thought that I needed to implement the interface to a MultiDraggable class (TypeScript) and then extend from that class (JavaScript).

I also don't know why I cannot directly extend the IDraggable interface in the JavaScript file for MultiselectDraggable.

I guess one way to "extend" the IDraggable interface would be to manually create the methods/properties in the interface.

[HollowMan6]

No worries, I think for the first step, we should get this working. Later we can consider refactoring and I will also see if there’s any way to avoid Typescript. At this stage I’m also not sure if there’s a better way.

[HollowMan6]

Or, you might want to try upgrading Blockly version https://www.npmjs.com/package/blockly/v/11.0.0-beta.12 Not sure if this can help improve things or not.

[HollowMan6]

@changminbark So for MultiselectDraggable, we don't need to implement the IRenderedElement interface, which means no need for having the getSvgRoot() method.

[changminbark]

Okay, thank you!

[HollowMan6]

Blockly v11 is officially released! https://github.com/google/blockly/releases/tag/blockly-v11.0.0 Now we should work on the release version instead of the beta one, and I have done an upgrade of the dev dependencies that actually support v11 and forced pushed the commit to the blockly-v11 branch: https://github.com/mit-cml/workspace-multiselect/blob/blockly-v11/package.json#L45-L48 @changminbark

[changminbark]

Should I just manually update the dependencies? Currently, my branch is 4 commits ahead of the remote branch and 1 commit behind it.

[changminbark]

I am also encountering an issue where I cannot seem to import the Coordinate class from Blockly after updating it. I am getting the following error message when I run the npm start script.

I have checked the package.json file in the Blockly directory under node_modules and the export statement seems to be correct.

Is there something wrong I am doing?

[changminbark]

I think I fixed it by using Blockly.utils.Coordinate. I guess that I cannot directly import Coordinate but rather have to use it indirectly.

[HollowMan6]

Should I just manually update the dependencies? Currently, my branch is 4 commits ahead of the remote branch and 1 commit behind it.

Sorry, I wasn't aware that you already have local commits. It can be a little complicated since I force-pushed. I guess you can drop these 2 commits related to package*.json changes to avoid conflict:

• changminbark@fb2c835
• changminbark@11b76b8

Then rebase on top of the upstream blockly-v11 branch.

I think I fixed it by using Blockly.utils.Coordinate. I guess that I cannot directly import Coordinate but rather have to use it indirectly.

Great to know you fixed this by yourself!

[ewpatton]

Blockly provides a migration tool that helps with situations where they reorganize classes. You may want to run that on the plugin code to see if there is anything else that needs to be corrected. Sent from my iPhoneOn May 22, 2024, at 01:50, Hollow Man ***@***.***> wrote: Should I just manually update the dependencies? Currently, my branch is 4 commits ahead of the remote branch and 1 commit behind it. Sorry, I wasn't aware that you already have local commits. It can be a little complicated since I force-pushed. I guess you can drop these 2 commits to avoid conflict: ***@***.*** ***@***.*** Then rebase on top of the upstream blockly-v11 branch. I think I fixed it by using Blockly.utils.Coordinate. I guess that I cannot directly import Coordinate but rather have to use it indirectly. Great to know you fixed this by yourself! —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[changminbark]

Hello @ewpatton ,

Thank you for the reply. How do I access this migration tool for Blockly? What I did was just copy over the updated dev-dependencies that Songlin made in the package.json file into my package.json file.

[mark-friedman]

I think I fixed it by using Blockly.utils.Coordinate. I guess that I cannot directly import Coordinate but rather have to use it indirectly.

It's worth noting the following from the Blockly 11 release notes:

If you install Blockly through npm, we now provide an exports clause to explicitly declare what you can import from our package.

• Your build tooling may now throw an error if you deep import from our package such as import 'blockly/core/some/file/name.js', which has never been supported by Blockly but may have been made possible by your build tools.

[mark-friedman]

Hello @ewpatton ,

Thank you for the reply. How do I access this migration tool for Blockly? What I did was just copy over the updated dev-dependencies that Songlin made in the package.json file into my package.json file.

Its here, though I'm not sure that it has been updated (yet) for Blockly 11. It's worth trying, though.

[changminbark]

On another note, I think I found a bug in the Multiselect plugin (original). I multi-deselect a group of blocks and then click at empty space, which seems to re-select the previously-selected group of blocks. This is footage of me testing it out on the original version of the plugin:

Multiselect_bug.mp4

[changminbark]

I am also encountering an issue where I select a group of blocks and then disable multiselect mode and then encounter an error where it is trying to run updateBlocks_ on the MultiselectDraggable object, which does not have a pathObject property, resulting in an error. However, the weird thing is that the code that calls the updateBlocks_ on the Blockly.getSelected should only run under these conditions:

// When not in multiple selection mode and Blockly selects a block not
// in currently selected set or unselects, clear the selected set.

However, I am not satisfying those conditions as I am merely disabling multiselect mode without unselecting the group of blocks.

MultiselectDraggable_bug.mp4

updateBlocks_ is called here:

[HollowMan6]

On another note, I think I found a bug in the Multiselect plugin (original). I multi-deselect a group of blocks and then click at empty space, which seems to re-select the previously-selected group of blocks. This is footage of me testing it out on the original version of the plugin:

That's interesting, I don't remember clearly if this was done intentionally or not, but I think this can be a feature/behavior to document XD (Revert back to the initial selection state of entering the multi-selection mode when you click the empty workspace).

I am also encountering an issue where I select a group of blocks and then disable multiselect mode and then encounter an error where it is trying to run updateBlocks_ on the MultiselectDraggable object, which does not have a pathObject property, resulting in an error.

You can safely remove all the pathObject updateSelected thing, that was initially intended to make the block look like it's "selected", now all the selection is handled by the Dragger, and we don't need that anymore.

However, the weird thing is that the code that calls the updateBlocks_ on the Blockly.getSelected should only run under these conditions:
// When not in multiple selection mode and Blockly selects a block not
// in currently selected set or unselects, clear the selected set.
However, I am not satisfying those conditions as I am merely disabling multiselect mode without unselecting the group of blocks.

updateBlocks_ is also called by the DragSelect library element (un)selection event, so it might also be called when disabling multiselect mode without unselecting the group of blocks.
I think this should be fine previously, let me know if this is blocking you from doing something.

workspace-multiselect/src/multiselect_controls.js

Lines 402 to 417 in af7eeb4

	this.dragSelect_.subscribe('elementselect', (info) => {
	const element = info.item.parentElement;
	if (inMultipleSelectionModeWeakMap.get(this.workspace_) &&
	element && element.dataset && element.dataset.id) {
	this.updateBlocks_(
	this.workspace_.getBlockById(element.dataset.id));
	}
	});
	this.dragSelect_.subscribe('elementunselect', (info) => {
	const element = info.item.parentElement;
	if (inMultipleSelectionModeWeakMap.get(this.workspace_) &&
	element && element.dataset && element.dataset.id) {
	this.updateBlocks_(
	this.workspace_.getBlockById(element.dataset.id));
	}
	});

[changminbark]

That's interesting, I don't remember clearly if this was done intentionally or not, but I think this can be a feature/behavior to document XD (Revert back to the initial selection state of entering the multi-selection mode when you click the empty workspace).

The weird thing about this is that it does not happen all the time. It only occurs in this scenario:

1. Turn on multiselect mode
2. Select a group of blocks
3. Turn off multiselect mode
4. Turn on multiselect mode
5. Unselect a group of blocks
6. Click on blank space to replicate weird behavior.

You can safely remove all the pathObject updateSelected thing, that was initially intended to make the block look like it's "selected", now all the selection is handled by the Dragger, and we don't need that anymore.

I tried commenting these portions of the code out and it leads to the blocks not being highlighted during the multi-selection (though they are highlighted after I let go of the mouseclick) and another error, which I think is because the multiselectDraggable class I made does not have a bringToFront() method:

[changminbark]

So I traced the error to the disableMultiselect() method. When I change the value of the Blockly.common.setSelected() in the pic to one of the random blocks in the selection, the error does not appear.

This is probably because updateBlocks_(Blockly.getSelected()) is being called in the updateMultiselect() method right after. The thing that confuses me is why is updateMultiselect() being called when I am merely turning off the multiselect mode? Is it because multiselect makes it so that updateMultiselect() is called for any workspace event?

[HollowMan6]

About that bug, I guess we just miss some condition checks, feel free to fix that in your code

I tried commenting these portions of the code out and it leads to the blocks not being highlighted during the multi-selection (though they are highlighted after I let go of the mouseclick)

You might want to update the selection on the go during the multi-selction as well in this case.

and another error, which I think is because the multiselectDraggable class I made does not have a bringToFront() method:

Then feel free to do something, either comment out the code and handle the issue at a later stage to find a solution, or do type checks and only call that when it’s an actual block.

Is it because multiselect makes it so that updateMultiselect() is called for any workspace event?

Ah yes, I do have some fix for fixing bugs in multi-workspace situations. Feel free to check old issues and PRs, such as https://github.com/mit-cml/workspace-multiselect/pull/24/files

[changminbark]

@HollowMan6 @mark-friedman
Hello, so I finished implementing the multiple-workspace helper functions. However, I'm stuck with the drag functions for the multiselectDraggable. I think I have an idea about why the drag functions are not being called. Basically, there is no clickable interface/SVG for the multiselectDraggable, as I explained in my email to Beka. What suggestions do you have to fix this? Or should I just wait for Beka to respond?

Thank you!

[HollowMan6]

Basically, there is no clickable interface/SVG for the multiselectDraggable, as I explained in my email to Beka. What suggestions do you have to fix this? Or should I just wait for Beka to respond?

We have already talked about this during our Saturday meeting, I'm not sure about the possible cause and I doubt if this is something really related to the SVG, since Beka has already mentioned that we don't need to implement the IRenderedElement interface in the previous email, but anyway it would be good to wait for Beka to respond.

@changminbark So for MultiselectDraggable, we don't need to implement the IRenderedElement interface, which means no need for having the getSvgRoot() method.

[changminbark]

I am uploading here the email thread between Songlin, Beka, and me:

This is for anyone who wants to learn more about the problem and what was proposed for the solution.

[HollowMan6]

This is for anyone who wants to learn more about the problem and what was proposed for the solution.

Yes, you should also include this comment in the code, suggesting that we should remove this hacky workaround once the Blockly team has fixed this with their new flexible gesture handling system.

[HollowMan6]

On another note, I think I found a bug in the Multiselect plugin (original). I multi-deselect a group of blocks and then click at empty space, which seems to re-select the previously-selected group of blocks. This is footage of me testing it out on the original version of the plugin:

Multiselect_bug.mp4

Now I fixed this bug by simulating the SHIFT keyboard event when we enter multi-select mode by icon, as indicated in the email I wrote to Chang Min:

About that bug, the correct behavior should be that we ignore the click on the workspace, and I just found that it’s actually the correct behavior when I hold the shift key to do the selection (instead of clicking on the icon), so it seems to be more clear for me what might be the cause for this bug, and it’s unlikely to be caused by eventListenerAll_ because of this.

[changminbark]

Hi everyone,

I finally got my laptop fixed and was able to resume implementing the multidraggable class. I am working on the final details of the implementation. More specifically, I am trying to fix this issue where the multidraggable object that contains the multiple blocks randomly jumps after starting a drag (after it has been moved before).

When I print out the newLoc parameter whenever I drag the multidraggable after unselecting and then reselecting them, it seems to jump to a very different number. How does the newLoc part of the drag function work? I think this bug has something to do with the gesture and specifically how it calculates the dragDelta when a drag is initiated:

I am not sure if I should reach out to Beka as I am not too familiar with the gesture code and whether they are going to update this.

I also know there is a bug where the block is disconnecting after being placed is related to this bug because when I reinitiate the drag, the blocks reconnect in the multidraggable object.

There are also a few bugs that I need to further explore, such as one where I turn on multiselect mode, select some blocks, turn off multiselect mode, move the cursor around, and then turn on multiselect mode and start the multiselect on top of a block, but it starts dragging the multidraggable object. I think this can be mitigated if we disable dragging entirely when multiselect mode is enabled, but I am not sure how to achieve this. We may have to ask Beka about this.

However, the bugs I stated in the beginning are the ones that I am most concerned with.

For now, I will continue to explore different options and look into the drag strategy class.

[changminbark]

Hi everyone,

I wanted to write an update regarding the bug where the multidraggable object would jump. It was resolved by setting the location of the multidraggable object to be (0, 0). This is because regardless of the reference frame of the workshop changing, the multidraggable object's location would be the same.

Currently, I am trying to fix the last major bug that I have encountered, which is that multidraggable objects that contain connected blocks seem to have its subdraggables/blocks disconnect upon ending a drag. I have printed some console.log statements and found that the blocks are being disconnected upon the end of the drag.

I am thinking that this has something to do with the fact that both blocks are being dragged and the dragstrategy that is used to aid with their dragging has updateConnectionPreview() and getConnectionCandidate() methods that are not functioning properly. However, I may be wrong as endDrag() is being called sequentially for all the subdraggables in the multidraggable object.

https://github.com/google/blockly/blob/24fe13f0c9ec6ddd06e1e70cd7b5ba4d6a3f9977/core/dragging/block_drag_strategy.ts

If anyone has any suggestions on what might be causing this bug, please let me know by commenting below! For now, I will continue to search for the cause of this bug and will try to post an update by Friday (US time).

Thank you!

[changminbark]

Hi everyone,

I would like to write an update and say that I am 95% done with updating the plugin. The main issue where the blocks were disconnecting on the drag was that I made it so that every selected block was being dragged. After talking to Beka and Songlin, I made the necessary changes to make sure the drag methods were only being called in the topmost blocks in the selection. If anyone would like me to post the email thread, please comment below.

Now, I only have to test two test cases, which are:

For the first test case, I think there may be an issue related to new updates in Blockly-v11. Correct me if I'm wrong, but there were updates to events in Blockly-v11 so that may not be compatible with the previous implementation of the multifield feature. In the code snippet below, my console.log is not firing, so I am guessing the new updates are not compatible with the e.element and e.recordUndo fields.

After implementing this and testing across two workspaces, I should be finished. All I would need to do is clean up the code and add comments/documentation, which should not take long.

Thank you