Fixing onSelect being called when objects were not intersected #5632
Conversation
…the selected area.
…the selected area (fabricjs#5596)
…the selected area (fabricjs#5596)
…the selected area (fabricjs#5596)
| // only add one object if it's a click | ||
| if (isClick) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (group.length > 1) { | ||
| group = group.filter(function(object) { | ||
| return !object.onSelect({ e: e }); |
There was a problem hiding this comment.
what about wrapping group.push(currentObject) in the if with this condition?
if (!object.onSelect({ e: e })) {
group.push(currentObject);
}
so we avoid the other if and the filter operation, just because is 5 lines extra not because i do not like it.
There was a problem hiding this comment.
That was my initial implementation. However, if the selection only intersected one object the onSelect() method would be called 2 times. one in this method and another on __setActiveObject(). That requires changing the _setActiveObject() to have a flag to not call the onSelect() if we are 'coming' from the __onCollectObjects(). I was reluctant to change as it could impact many operations. However, if you like that aproach better let me know and I'll do a refactoring
There was a problem hiding this comment.
no, at this point this solution is better.
We still need tests. Can you write them?
There was a problem hiding this comment.
yes, i wasn't able this weekend, but I'll work on it this week. I'll keep you updated.
There was a problem hiding this comment.
@asturur Already implemented the tests. If you need anything more just let me know
|
Do not forget we need to add a unit test for this change. If you can't run them locally or you do not know how to do, please ask. I can help |
|
I'll see the implemented tests and try to create a test for this change. I'll probaly need yout help :) I'll let you know. |
|
i think is ok! |
|
Ok. Do you have any estimates of when this will be released? |
|
This will be in the bundle of fabricJS 3.0 |
PR fixing the bug stated on the issue #5596
The onSelect method is only called after we determine that the multiselect intersects the object. If the onSelect returns 'false' we discard them from the group.
close #5596