Skip to content
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

v0.1.3 #6

Merged
merged 3 commits into from
Nov 16, 2022
Merged

v0.1.3 #6

merged 3 commits into from
Nov 16, 2022

Conversation

HollowMan6
Copy link
Collaborator

@HollowMan6 HollowMan6 commented Nov 4, 2022

Signed-off-by: Hollow Man hollowman@opensuse.org

README.md Outdated Show resolved Hide resolved
@mark-friedman
Copy link
Contributor

Sorry about this, it was just some stupid VSCode auto-completion that I didn't notice...

No problem! It looks the Readme still mentions using a patched version of the library, but I'm think that's no longer the case, since your PR was merged into DragSelect.

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@HollowMan6
Copy link
Collaborator Author

No problem! It looks the Readme still mentions using a patched version of the library, but I'm think that's no longer the case, since your PR was merged into DragSelect.

Ah Okay, has fixed this.

Copy link
Contributor

@mark-friedman mark-friedman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing and all seem fine. I noticed a small nit, which is that in a newly refreshed/reloaded workspace the very first time I shift-click a child block it only selects the child. Once I click away, from that point on when I shift-click a child block it selects the child and the top-level ancestor block.

@HollowMan6
Copy link
Collaborator Author

I noticed a small nit, which is that in a newly refreshed/reloaded workspace the very first time I shift-click a child block it only selects the child. Once I click away, from that point on when I shift-click a child block it selects the child and the top-level ancestor block.

Hi, thanks for testing! Actually I can't reproduce what you mentioned. In a newly refreshed/reloaded workspace the very first time I shift-click a child block it only selects the child, then when I click away, from that point on when I shift-click that child block again, it will still select that child block only.

GIF 2022-11-12 9-30-33

Maybe I have misunderstood what you said, it would be great if you can post some gifs to help me know.

@ewpatton
Copy link
Member

@mark-friedman I'm also not seeing the behavior as described. So far things seem to be working well for me.

@mark-friedman
Copy link
Contributor

@HollowMan6 @ewpatton
Below is a video of what I'm seeing. I have a clone of HollowMan6/multi-select and have checked out the main branch. I've run npm install and I'm running the Blockly Playground in that directory via npm start. I pulled out the blocks that you see and then did a browser reload. This is on a Mac and I see the same behavior on Chrome Version 107.0.5304.110, Firefox Version 106.0.2 and Safari Version 16.0

Note that after the reload my first click is the shift-click of the print block. All the clicks on the block are shift-clicks. After each shift-click there is a regular click on the workspace background.

multi-select.mov

@HollowMan6
Copy link
Collaborator Author

Okay, I see. Unfortunately, I cannot come up with an instant fix for this because this relates to the fundamental of how this plugin works, and I would say that the bug was not introduced by this PR and should exist since the first beginning. Currently, we rely on DragSelect to know which block gets selected. DragSelect seems to listen to the "blocks". However, it actually works by listening to the SVG path element, which is always a rectangle with some transparent parts forming a block.

For such irregularly shaped blocks in your example, the two blocks' SVG path elements overlap with each other, so if you shift-click on that area, DragSelect will think that both blocks get selected.

image
image

I would say a proper fix should be that Blockly implements some sort of "native version of DragSelect" and exposes it with an API, so that we can know for sure where the block actually locates, and that would be a lot of work. If such an API is available, we can then use that API instead of DragSelect.

@mark-friedman
Copy link
Contributor

Any idea why it works one way in a freshly loaded workspace and another way after click activity in the background?

@HollowMan6
Copy link
Collaborator Author

Any idea why it works one way in a freshly loaded workspace and another way after click activity in the background?

In a freshly loaded workspace, your focus is not on the workspace until you click on some part of it. The plugin cannot receive the keyboard event when your focus is not on the workspace, so the shift drag didn't actually get activated at that time (You can see that the icon in the right-bottom corner also suggests this). The first click on a freshly loaded workspace would always be the Blockly native one that selects only one block if you happen to click on some block, and that matches the typically expected behaviour as you will not select more blocks in a freshly loaded workspace.

@ewpatton
Copy link
Member

ewpatton commented Nov 15, 2022 via email

@HollowMan6
Copy link
Collaborator Author

It's possible we could attach the key handler to the document, or some other developer provided element.

I remember we discussed this issue before in the summer, as there can be multiple workspaces with multiple instances of plugins on one page. We can't just simply listen to the document.

After all, the behavior is expected, and I think this should not be a problem to keep as it is.

@ewpatton
Copy link
Member

ewpatton commented Nov 15, 2022 via email

@HollowMan6
Copy link
Collaborator Author

That supports my position that it could be developer provided as the developer knows more about their end product than we do. We might also be able at least in app inventor to focus the workspace to address this issue if that's the limitation. It ought to be as easy as recommending in the readme that people call .focus() on the workspace but I have tried that yet.

Yeah, I agree with the later part, we can call .focus() to get workspace selected in the first place, and just added that together with the issue discussed above into the README. I've also added the .focus() into the test, now it should not be a limitation any more when running the Blockly Playground via npm start.

For the former part, I think it's not necessary, it may complex the plugin by requiring more parameters. There may also exist some hidden issues if we implement that.

Signed-off-by: Hollow Man <hollowman@opensuse.org>
package.json Outdated Show resolved Hide resolved
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Copy link
Member

@ewpatton ewpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve this for now and we should file an issue to look into the DragSelect with invisible rectangles thing. I think it's solveable if we play with the event handling a bit since a child block is contained within its parent in the SVG, so we can always just pick the node deepest in the DOM tree.

@ewpatton ewpatton merged commit 4552703 into mit-cml:main Nov 16, 2022
@HollowMan6
Copy link
Collaborator Author

We should file an issue to look into the DragSelect with invisible rectangles thing. I think it's solveable if we play with the event handling a bit since a child block is contained within its parent in the SVG, so we can always just pick the node deepest in the DOM tree.

Yeah, but that's not a fix that can solve the fundamental issue:
GIF 2022-11-16 19-25-13

A proper fix should still be that Blockly implements some sort of "native version of DragSelect" and exposes it with an API, so that we can know for sure where the block actually locates.

@HollowMan6
Copy link
Collaborator Author

Has filed an issue here: ThibaultJanBeyer/DragSelect#150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants