Blockly v11 #62
Blockly v11 #62
Conversation
There was a problem hiding this comment.
Fix the linting issues. Looks like there are quite many here:
$ npm run lint
> @mit-app-inventor/[email protected] lint
> eslint . --fix
....
✖ 68 problems (55 errors, 13 warnings)
|
I'll continue reviewing your actual code once the above ones have been addressed. |
|
I think the merge conflict is just a matter of indentation. |
Anyway you need to fix this as it stops us from merging. |
|
I just merged the upstream main into your PR to facilitate my reviewing, remember to pull the merge commit to your local git repo @changminbark |
|
Looks like we will eventually end up with page crashed when I play around with multi-selection |
|
@HollowMan6 I didn't get any pages that crash. How long did you use it for and do you know what was the last action you did before it crashed? |
There was a problem hiding this comment.
We shouldn't be able to select shadow blocks for multi-select. These blocks are not selectable in Blockly and you can't actually manipulate them individually.
It seems to be pretty random, so I can't tell exactly, but not long, maybe 30 seconds to 1 minute with quick actions for testing. Also, this seems to be only reproducible in Chrome, not Firefox. While in Firefox, I can see this error here as frequently as the time for Chrome crashes: 
Also, I have caught another error here: |
|
Looks like after dragging, sometimes I may also get a block connected like this disassembled: 
|
|
Let's first get the above issues addressed, once these are done, I'll take a close look at the logic of the code. |
What do you think is causing the issue for chrome? I am not sure because there is no proper error message that gives me a hint as to what is causing it. |
I'm not quite sure either but I guess it can be related to the logic of the code, maybe somewhere we have a bug that causes the memory usage to explode, and the thread for rendering the page was killed? If I find something, I'll let you know, you can focus on the other issues first. |
|
I have made the requested fixes. As for the firefox error, I am not encountering it. Maybe with some of the changes I made, the issue is solved. As for the chrome web page crashing, I am also not encountering it; I'm guessing it may had something to do with the getAllBlocks instead of the getTopBlocks (which I changed). As for the third error that you showed related to reading the properties of 'x', I am not encountering it. I am not sure how to replicate this error, but for now I am not coming across this issue.
As for this issue in the picture, how are you encountering this bug? I am also not coming across this. Do you know what steps replicate this behavior? |
Clearly, these bugs are related to null/types checks since they indicate that something is undefined. For these bugs usually having a strong condition check is enough and you don't need to take time to replicate, but of course, if you like/want to find out the root cause, you can try to reproduce as well (didn't take the notes for reproducing these, but usually you can try with random blocks and do some random multi-selected actions).
I don't think so for this case, I was only having less than 10 blocks on my page. If you can't reproduce, maybe it's just because something is wrong with the Chrome on my computer, but let's see.
I don't have an exact description for reproducing this either, it's kind of hard to reproduce, but I guess it can be related to a possible edge case where you failed to find out the top-most block and drag all of them together. |
There was a problem hiding this comment.
We shouldn't have the already selected block deselected (This seems to only happen when I have only one block selected) when we drag a rectangle to select
In a similar situation (afterwards), now I have 2 blocks, the already selected one gets dragged while I was only try to drag a rectange (it can work fine as well sometimes but we have a stable reproduction if you do so just like in my GIF right after the above one).
I think that this may be related to the keyboard navigation plugin and its use of Blockly.FieldColour. However, the current version of Blockly that is being used does not have this.
I will look into this after finishing task 2 (cross-checking all the plugins). As for the other bugs, they were simple fixes. I have pushed the changes. Edit: I found a bug related to the undo function for comments. When I copy/paste a comment and undo the action, then when I try multiselecting a comment and removing it, I get this error:
This is definitely related to the undo bug that I asked Beka about. So, here is my updated checklist of things to do: List of things to checkout:
|
This could be because FieldColour is no longer part of the Blockly core fields. It is now a plugin (here). |
There was a problem hiding this comment.
This could be because FieldColour is no longer part of the Blockly core fields. It is now a plugin (here).
Ah, that’s right, thanks Mark! Then I guess it would be a good idea to add that plugin to devDependencies as well.
Should I add it into the index.js file as a default configuration? |
To fix the error, of course, it should be binding to the keyboard navigation plugin. |
I don't think adding the plugin will fix it. I think the problem lies in the keyboard navigation plugin. That plugin refers to the field color settings in Blockly, but it does not exist in Blockly anymore. It should be referring to the field colors plugin in the keyboard navigation plugin. I just tried installing the field color plugin and it does not resolve the warning message. |
Okay, then feel free to report this to the Blockly team by opening an issue there or help them fix the error by sending a PR if you want. |
|
Okay, then I will remove the devDependency and re-push the version without the field colors plugin. Issue link: google/blockly-samples#2439 |
|
I finished cross-checking the multiselect plugin with the other plugins. Now I will start looking into the undo-bug related to workspace comments. When I finish this, I will start looking into implementing the additional/optional features to support workspace comments. |
|
By this Saturday or next week, I think I will update the dependencies/package.json (probably during our meeting) as the undo-bug related to workspace comments should be tested by then. Then we should be able to release a stable version of the plugin next week. I will create a separate branch for the additional/optional features to support the workspace comments. |
There is no need for that, and I think we will only release one once all the goals for this GSoC are reached since it's not in a hurry anyway. You can still push those to this PR. |
The only thing is, I am not sure if I will be able to implement all the optional features by the end of the GSoC period. Hence, if I am not able to finish the additional support, we can still release a stable version (and then merge the branch with additional features into the main branch when we want to release them). |
Okay, let's see how it goes then. One of the major reasons why I prefer to delay the release is that I don't have access to release the plugin at npmjs, so I can't release a new plugin by myself. We will need to ask for help from @mark-friedman or @ewpatton. |
|
Last task in GSoC: Undo bug related to workspace comments So, after testing out the implementation of the isDeadOrDying() check in the isDeletable and isMovable methods, the comments seem to be behaving properly (like the blocks) with the undo actions. I have created an issue and also a PR that fixes this issue. Issue: google/blockly#8531 I have finished all of the required tasks for the GSoC project (updating the multiselect plugin and cross-checking it with other existing Blockly plugins). I think we should be able to release a stable version soon (after updating the dependencies with Songlin in tomorow's meeting). As for the optional/additional comment support, I am not sure how much of it I will be able to complete as my summer break is ending in 1 week. If anyone wants to help or contribute to the multiselect plugin and needs some guidance, please reply to this comment or reach out to me. This could also be part of next year's GSoC project. |
|
Great, I'll do a full review tomorrow after applying the patch you just proposed, and see how everything goes.
I personally hope you can stay in the loop and continue contributing after GSoC ends when you are free (which I remember you mentioned in your proposal, and that's also one criterion for us to select people), just like me :) I guess next year I won't have free time to mentor GSoC again since I've started my PhD, and if you want, you can also be the mentor! |
Yes, I will definitely continue to contribute to GSoC. However, I am not sure how busy I will be this semester as I am taking junior-level courses as well as applying to internships. I will help whenever and wherever I can! |
There was a problem hiding this comment.
This is definitely good enough to make a new release, @mark-friedman let me know if you have any other comments, otherwise I guess it's good enough to make a new release after we bump the dependencies versions in package.json and squash all the commits into one. Thank you for your hard work @changminbark !
Once you bump the version number and merge, I'll publish to NPM. Or if you add me as a maintainer I'm happy to do the version number bump and merge myself, before publishing. |
Unfortunately, I don't have the authorization to add anyone as a maintainer, and I guess this needs to be done by @ewpatton, but I can do the merge myself anyway, thank you! |
Signed-off-by: Hollow Man <[email protected]>
|
Hi @mark-friedman , this has been merged, and the version has been bumped https://github.com/mit-cml/workspace-multiselect/releases/tag/v1.0.0 Would you help with releasing it at the npmjs side? Thank you |
|
Will do.
…-Mark
On Sat, Aug 17, 2024 at 1:25 AM ℍ𝕠𝕝𝕝𝕠𝕨 𝕄𝕒𝕟 ***@***.***> wrote:
Hi @mark-friedman <https://github.com/mark-friedman> , this has been
merged, and the version has been bumped
https://github.com/mit-cml/workspace-multiselect/releases/tag/v1.0.0
Would you help with releasing it at the npmjs side? Thank you
—
Reply to this email directly, view it on GitHub
<#62 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANJWSRWJQJUX4Q7LSUZCY3ZR4CGJAVCNFSM6AAAAABJ6RMM42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJUG44TEMRXHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Done!
…-Mark
On Sat, Aug 17, 2024 at 2:37 PM Mark Friedman ***@***.***>
wrote:
Will do.
-Mark
On Sat, Aug 17, 2024 at 1:25 AM ℍ𝕠𝕝𝕝𝕠𝕨 𝕄𝕒𝕟 <
***@***.***> wrote:
> Hi @mark-friedman <https://github.com/mark-friedman> , this has been
> merged, and the version has been bumped
> https://github.com/mit-cml/workspace-multiselect/releases/tag/v1.0.0
>
> Would you help with releasing it at the npmjs side? Thank you
>
> —
> Reply to this email directly, view it on GitHub
> <#62 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AANJWSRWJQJUX4Q7LSUZCY3ZR4CGJAVCNFSM6AAAAABJ6RMM42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJUG44TEMRXHA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
The basics
The details
Resolves
Fixes #39
and part of #50 (backpack and scroll options plugins fixed and verified)
Proposed Changes
Updates the multiselect plugin to be compatible with Blockly version 11.
Reason for Changes
To make it compatible with the latest Blockly version and avoid the need to monkey patch.
Test Coverage
There are several test files in the test subdirectory.
Documentation
I have created javascript docs as well as comments for any new code.
Additional Information