-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cross-testing the Multi-select Plugin with other Blockly Plugins #50
Comments
The new changes in Blockly 11 may cause the compatibility issue with the backpack plugin again, so we should re-evaluate again after #39 is done |
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, |
Quick update: The newly updated multiselect plugin works with the scroll-options plugin as well as multiple workspaces. |
Checklist: Original
Workspace-related
Blocks-related
|
Keyboard Navigation Plugin:https://developers.google.com/blockly/guides/configure/web/keyboard-nav Tested with the backpack plugin and multiselect plugin
Note: When editing a field (e.g. true/false block), I cannot use "w" or "s" to navigate through the field options. However, I can use the arrow keys to navigate through the field options. I don't know if this is intended behavior for the keyboard navigation plugin. I tested this with and without the multiselect plugin. Note: I am not sure if there is a bug with the keyboard navigation's copy/paste function. The following gif was tested without the multiselect plugin. I cannot seem to reproduce it, so I am also not sure about this bug. However, with the multiselect plugin, it seems to be holding two different copyData storages (so two different copy-pastes are running simultaneously with the shortcut). What should be the intended behavior here? Should we incorporate the copy functionality of the keyboard navigation plugin into the multiselect plugin? I don't think this would be a good idea as not everyone will use both plugins at the same time. Do you have any other suggestions? Note: When installing the multiselect plugin first, there is no error regarding the delete shortcut. However, when installing the keyboard navigation plugin first, we encounter this error: I am not sure why we don't see this issue when we install the multiselect plugin first. Multiselect Plugin: Note: The multiselect mode is activated when holding shift for moving around the keyboard navigation cursor, but this does not cause any unwanted behavior as the multiselect mode affects cursor/mouse behavior. Backpack Plugin: |
The Keyboard Navigation Plugin is highly experimental, so I would expect this to happen. You can report it to the Blockly team.
That's one thing to think about. I guess the Blockly team doesn't have a good solution for this either (e.g. cross tab copy-paste and the keyboard navigation also have two set of copyData storages). Of course, we shouldn't incorporate the copy functionality of the keyboard navigation plugin into the multiselect plugin this time, but it would be good to work out some design so that people can connect the multi-select copyData storages with possibly other plugins /for other uses (extensibility).
This is exactly #41 Let me know if you have any insights on this one. |
This may be related to this issue: google/blockly-samples#2380
I think that a possible solution to this would be to implement what the multiselect plugin is doing in Blockly core. Instead of having the base copy/cut/paste function directly using the copyData of that one block/element, it should define a set (can be called copyData) that contains a set of each element's copy data (whether it be just one element or multiple). This can be used across different plugins that have their own copy/cut/paste shortcut functions defined.
I found out the reason why this occurs. Currently, the keyboard navigation plugin allows for collisions when they register the key mappings for the shortcut registry while the multiselect plugin does not. This should be a simple fix by also allowing the shortcuts in the multiselect plugin to allow for collisions. However, I am not sure if this would be a good idea as other plugins may have copy/cut/paste shortcuts that behave differently. Having multiple copy/cut/paste functions would lead to unexpected behaviors. It would also be impossible to make an all-in-one copy/cut/paste functions for all the plugins. So I am not sure how to approach this issue. If there is nothing else that I can investigate in, I will mark this cross-checking as done and move onto the next plugin. |
You can discuss that with the Blockly team to see if this is feasible.
Yeah, I mean that and that's why I mentioned this here: https://groups.google.com/g/blockly/c/MuL2Ln8SwDU/m/9WihibDsAgAJ
You can try and see if it works when you allow for collisions. One of the solutions I proposed (guess, not verified) is to temporarily unregister the multi-select plugin key mappings when keyboard navigation is activated, and register that back when deactivated. You can also verify if this works/is possible or not. if you have any better ideas/comments, feel free to continue commenting under that thread. |
I will send an email to them soon.
I don't have access to comment on the thread.
I was able to do the temporary unregister and it seems to be working. Since it works, should I implement it into the multi-select plugin and push the change into the blockly-v11 branch? |
Really? It's a public Google group.
You mean implement the "allow for collisions"? Sure, you can test that and once you think it's fine, it's always preferred to have that allowed. |
The only problem I have encountered is that when we unregister the multiselect shortcuts when we are in the keyboard navigation mode, we cannot use the cursor to select/copy/cut/paste. We can only copy/cut/paste the blocks selected in the keyboard navigation mode. I have tried registering the original Blockly core shortcuts, but they do not allow for collisions and will run into an error with the keyboard navigation plugin. If you think this behavior is still okay, I will push it into the blockly-v11 branch of the multiselect plugin. |
Have you actually joined the group?
I don’t quite understand, are you combining the things I mentioned with allowing collisions, or simply the methods I mentioned? If later, what are you going to commit in our code base? We can discuss this more during our meeting. BTW, since we are touching something new, there is no regression involved, do you think this is Okay or not? It’s always good to develop a “design taste” for everyone, not just for me. For me, my principle is that, if it introduces another higher priority bug that doesn’t have a possible workaround, it’s better not to introduce it. |
Oh, I guess that was the issue.
So what I did was I allowed collisions from the multiselect plugin side and then made it so that the multiselect plugin's shortcuts are unregistered when we turn on the keyboard navigation mode and re-register it when we turn off the keyboard navigation mode. I can just commit the portion of the code that allows for collisions from the multiselect plugin side, or I can commit the whole code that also unregisters/re-registers the shortcuts (this is based on the conditional shown below).
No, this is not possible. That is why I am a little unsure about whether we should commit just the collision allowance or also the shortcut unregistration/re-registration. In my opinion, it does not make sense for the keyboard navigation to rely on the multiselect plugin to select multiple blocks. That should be feature on its own in the keyboard navigation plugin. As for the behavior shown in the gif, I think it should be fine as long as the user knows that you are not able to copy/paste using the cursor at all (as we unregister all cursor related copy/paste functions including the original Blockly core one) while in the keyboard navigation mode. |
Sure, you can commit that. BTW, are you going to expose "unbindMultiselectCopyPaste_" to the developer, or is this going to happen automatically? If the former, there's a naming issue here in the method (public ones should not have an underscore as the suffix).
It looks like the Blockly team wants to integrate the keyboard navigation into core in the future, so in the end, we should still be able to do that google/blockly-samples#2380 (comment)
How will the user know? It sounds like only the developer knows that. |
This will happen automatically, as it is a 'keydown' event listener. It only fires if it detects that the workspace is in keyboard accessibility mode.
Then the Blockly team would have to make the original core shortcuts to allow for collisions. Because right now, we cannot use both (core and keyboard nav) shortcuts as the core does not allow for collisions (if we are to re-register the core shortcuts). This would be the solution to allowing users to use the core shortcuts while in keyboard navigation mode (with the multiselect plugin). So, currently, after I push the latest commit, we will be able to use both plugins, but we will not be able to copy/paste regularly using a cursor when the keyboard navigation mode is on unless we make a change to the core shortcuts OR change the keyboard navigation plugin to unregister/re-register their shortcuts.
I guess there is no way the user knows unless there is some kind of written documentation/tutorial for them. I just thought it did not make sense for a user to turn on the keyboard navigation mode and then proceed to use the cursor to copy/paste. Isn't the purpose of the keyboard navigation mode to completely remove the need for a mouse/cursor? |
Sounds good if we document this on our side.
Maybe you also want to make the Blockly team aware of this (by opening an issue there or something) |
Okay, I have pushed the commit in the PR and created an issue: google/blockly-samples#2424 I will now start cross-checking with the next plugin (Shadow-block-converter). |
Shadowblock Converter Plugin:https://github.com/google/blockly-samples/tree/master/plugins/shadow-block-converter Tested with the backpack plugin, keyboard navigation plugin, and multiselect plugin
Note: I think that I've come across a bug where editing a shadow block will select the parent block (at least visually). However, when trying to select something else (like a block or the workspace), the former shadow block's parent block does not get unselected (visually). This was tested without any of the other plugins (except for backpack). I will report this on the Blockly plugins github page (google/blockly-samples#2426). Note: When a shadow block gets converted into a regular block, it is not in the multiselect draggable's subdraggables, but this is not an issue for modifying/moving the multiselection as the parent block is still in the selection. Multiselect Plugin: Note: The multiselect mode is activated when holding shift for moving around the keyboard navigation cursor, but this does not cause any unwanted behavior as the multiselect mode affects cursor/mouse behavior. Backpack Plugin: Keyboard Navigation Plugin: |
If there are no concerns or questions, I will move on to the next plugin (Workspace-content-highlight) soon. |
(Workspace) Content Highlight Plugin:https://github.com/google/blockly-samples/tree/master/plugins/content-highlight Tested with the backpack plugin, keyboard navigation plugin, shadow block converter plugin and multiselect plugin
Multiselect Plugin: Backpack Plugin: Keyboard Navigation Plugin: Shadow Block Converter Plugin: |
Disable Top Blocks Plugin:https://github.com/google/blockly-samples/tree/master/plugins/disable-top-blocks Tested with the backpack plugin and multiselect plugin.
Note: We have to install the multiselect plugin first if we want the disable top blocks plugin to work properly as the multiselect plugin installs/overrides the disable top blocks plugin's context menu. Should we write this down somewhere or is it enough to indicate it in this comment? Multiselect Plugin: Backpack Plugin: |
This does not cause any bugs/conflicts/unintended use cases. I will document this in the README and push it to the blockly-v11 branch. |
Strict Connection Checker Plugin:https://github.com/google/blockly-samples/tree/master/plugins/strict-connection-checker Tested with the backpack plugin and multiselect plugin.
Without strict connection check: Multiselect Plugin: Backpack Plugin: |
Block Dynamic Connection Plugin:https://github.com/google/blockly-samples/tree/master/plugins/block-dynamic-connection Tested with the backpack plugin and multiselect plugin.
Note: If you watch the gif, when I undo the connection of the dynamic connection, some of the blocks are moved to a random location first. This only occurs with the multiselection. I am not sure what is causing the location to be moved randomly. However, it does not seem to be causing any major issues (unintended behavior). I will continue looking into what is causing it. Note: Another thing I noticed is that the dynamic create text block does not shrink back to its original number when I undo the action of dynamically connecting the block. This is also present without the multiselect plugin. I opened an issue at google/blockly-samples#2434 Test environment:
Multiselect Plugin: Backpack Plugin: |
After looking into the weird undo behavior, I found that each block undergoes a drag and then two move actions. This seems to be the same for the block that does not move to a random location prior to connecting (the block with the normal behavior also has 1 drag and then 2 move actions being called). I am not too sure what is causing this. If you have any suggestions, that would be helpful. However, this might also be related to the weird undo compatibility with the multiselect plugin. |
Block Shareable Procedures:https://github.com/google/blockly-samples/tree/master/plugins/block-shareable-procedures Tested with the multiselect plugin.
Test environment
index.html:
Multiselect Plugin: |
Test Blocks Plugin:https://github.com/google/blockly-samples/tree/master/plugins/block-test Tested with the multiselect plugin.
Note: The drag to dupe block is never added to the multiselection as it breaks the functionality (addressed previously in PR). Multiselect Plugin: |
Closing as the goal has been reached, thank you @changminbark |
Check for duplicates
Problem
Test the integration with other Blockly plugins, apply fixes whenever applicable (something to start with is https://groups.google.com/g/blockly/c/MuL2Ln8SwDU)
Request
To start with:
Workspace-related
Blocks-related
We might want to check with other plugins as well later if time allows
Alternatives considered
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: