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

Add an option only to pick the child selectable per click in multi-select mode #150

Closed
HollowMan6 opened this issue Nov 17, 2022 · 13 comments
Milestone

Comments

@HollowMan6
Copy link
Contributor

HollowMan6 commented Nov 17, 2022

Is your feature request related to a problem? Please describe.
It would be great if DragSelect could implement an option, such that in multi-select mode, when there are two selectable, one is a child of another with regard to DOM element, then only the child get selected in a single click event.

Describe the solution you'd like
We may implement a filter for the click event in multi-select mode, when two or more blocks get selected at the same time in the same click, check if some blocks are the parents of other selected/deselected blocks, and filter the selection/deselection action for those parent blocks.

Additional context
Please see here: mit-cml/workspace-multiselect#6 (comment)
#152

@ThibaultJanBeyer
Copy link
Owner

Hi @HollowMan6, thanks for the contribution, this is an interesting one. Do you have some playground or code examples so I could play around with this? I’d like to understand your use case better to be able to support you better. Can you add such a test-case for this change in the PR please?

@HollowMan6
Copy link
Contributor Author

Hi @ThibaultJanBeyer , just added the test case.

For use case, you can read from mit-cml/workspace-multiselect#6 (comment) to learn more about the issue. You can also go to https://hollowman.ml/workspace-multiselect/ to play with my patched version. Run npm start using this commit to play with the version before patch.

image

@ThibaultJanBeyer
Copy link
Owner

Hi @HollowMan6 thanks for providing this, did you already add your fix on https://hollowman.ml/workspace-multiselect/ ? Because I can’t really reproduce your issue, see:

Kapture.2022-11-24.at.21.01.32.mp4

I’ll test the PR soon and probably approve it if it looks fine. But for the long run we should think of a move sustainable way to enhance/overwrite the internal methods from the outside. I’ve read that you want to re-write your own solution but might be better maybe if we can make DragSelect more extensible and I don’t mean adding more options, because they seem very specific to your edge-case, but maybe adding a way to enhance/extend internal methods instead, so that every future edge case can be handled without internal change. What do you think? So far it’s just an abstract idea of mine. Do you have ideas maybe?

Thank you and to be hones I love what you are doing with the tool it’s pretty fun! 🎉

@HollowMan6
Copy link
Contributor Author

HollowMan6 commented Nov 24, 2022

Hi, thanks for replying! Yes, I've already added the fix on https://hollowman.ml/workspace-multiselect/ . The code in https://github.com/mit-cml/workspace-multiselect/tree/main still uses the unpatched one currently, you can manually clone and run that if you like.

I’ll test the PR soon and probably approve it if it looks fine. But for the long run we should think of a move sustainable way to enhance/overwrite the internal methods from the outside. I’ve read that you want to re-write your own solution but might be better maybe if we can make DragSelect more extensible and I don’t mean adding more options, because they seem very specific to your edge-case, but maybe adding a way to enhance/extend internal methods instead, so that every future edge case can be handled without internal change. What do you think? So far it’s just an abstract idea of mine. Do you have ideas maybe?

Yeah, I agree. I think maybe a good way is to make a user-defined hook after calculating selection list available to developers like us, just like how my PR handles this time. You can turn the function call here into excuting some user specific code (function signature can be just the same as the filterParent so that user can get enough information, or add also the unselect list and unselect rect list into the parameter as well, and the hook returns the new select and unselect list, so that we can possibily make something unselected get selected for maybe other use cases), then users can write the logic into that hook to fine-tuning the selection list. If that's possible, then my PR will no longer be needed.

Thanks for for hard work on DragSelect, too. There can't exist my plugin without your project.

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Nov 25, 2022

Hi @HollowMan6 I think for the long run we might want to have a way to override internal workings by overwriting public methods if necessary but this will take longer to implement and needs more thinking.

Good news, we can achieve something similar already with subscriptions! 🎉
I am wondering if this would work for you:

ds.subscribe('elementselect', ({ item }) => {
  const selected = ds.getSelection()
  // do & filter whatever you want here, for example like this:
  const rects = selected.map((element) => element.getBoundingClientRect())
  const toRemove = filterParent(selected, rects, ds.Selector.rect)
  // or only the item maybe like this: filterParent(item, item.getBoundingClientRect(), ds.Selector.rect)
  ds.removeSelection(toRemove)
})
  • I guess you would in the "filtering" not even have to go through all selected elements? You could just check from the newly selected element by using item. Then check the items parents and remove them, this could even be more performant 🤔
  • If this works well, I think adding ds.getSelector, ds.getSelectorRect would be useful 🤔

What do you think, would you mind trying it out?

@HollowMan6
Copy link
Contributor Author

HollowMan6 commented Nov 25, 2022

Hi @HollowMan6 I think for the long run we might want to have a way to override internal workings by overwriting public methods if necessary but this will take longer to implement and needs more thinking.

Good news, we can achieve something similar already with subscriptions! tada I am wondering if this would work for you:

ds.subscribe('elementselect', ({ item }) => {
  const selected = ds.getSelection()
  // do & filter whatever you want here, for example like this:
  const rects = selected.map((element) => element.getBoundingClientRect())
  const toRemove = filterParent(selected, rects, ds.Selector.rect)
  // or only the item maybe like this: filterParent(item, item.getBoundingClientRect(), ds.Selector.rect)
  ds.removeSelection(toRemove)
})
  • I guess you would in the "filtering" not even have to go through all selected elements? You could just check from the newly selected element by using item. Then check the items parents and remove them, this could even be more performant thinking
  • If this works well, I think adding ds.getSelector, ds.getSelectorRect would be useful thinking

What do you think, would you mind trying it out?

Hi, That won't work. I want to fine-tune what get selected in a single event (like a single click event), it's not something related to previous selection state. I mean, if the parent already got selected previously, we don't want to cancel the selection of that. What you proposed is not related to this.

So I would say the best choice would still be implementing a hook if possible

@ThibaultJanBeyer
Copy link
Owner

Hi @HollowMan6 I’m back from vacation and looking into this. I have concerns adding the filtering logic within the project. I would like to give a way to pass in a filter method so this could be re-used in other use-cases. It could have the same function call. I’m looking into this now 👀

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Jan 23, 2023

Oki sooo, I looked more into it and I think it makes most sense to have 2 public methods that can theoretically be overwritten in the Selection module: one to overwrite the isCollision in the loop do you think that would be enough? This would be ideal because least code change and most performant on your side. I started playing around trying to match your change to changing the filtering, I’ve no more time today but will continue tomorrow.
If that is not enough then a new "filter" method that can be overridden somewhere here, to filter the selection after it was build up (like u originally did) 🧀

@HollowMan6
Copy link
Contributor Author

Hi @ThibaultJanBeyer ! Great to see this going forward, I think overwrite the isCollision in the loop may be enough, but still need further investigation.

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Jan 24, 2023

I played with it more and I think in the loop is not enough, because as I understand you want to check wether the element that gets selected/unselected is a child or another element that would potentially be selected and if so, then exclude the parent from that selection. Is that correct? If so, then it’s only possible when we already know all the elements that will be selected and not during the process of figuring that out. So for your use-case I think best is to provide a way to override the list after the initial check is done like you initially did even though that’s less performant for you but seems good enough for your use-case. I’ll take over your PR from here. Today is my birthday haha but tomorrow I’ve booked a timeslot to work on this. Thanks a lot for your contribution! 👍

@HollowMan6
Copy link
Contributor Author

Happy birthday @ThibaultJanBeyer 😀

@ThibaultJanBeyer ThibaultJanBeyer added this to the 2.6.0 milestone Jan 31, 2023
@ThibaultJanBeyer
Copy link
Owner

Available in 2.6.0

@HollowMan6
Copy link
Contributor Author

Awesome! Thank you!

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 a pull request may close this issue.

2 participants