fix(): selection logic to support nested multiselection #8665
Conversation
|
Build Stats
|
ShaMan123
changed the title
src/canvas/Canvas.ts
Outdated
| const shouldRender = this._shouldRender(target), | ||
| shouldGroup = this._shouldGroup(e, target); | ||
| const shouldRender = this._shouldRender(target); | ||
| let shouldGroup = false; |
There was a problem hiding this comment.
how do we know that this was always false in all possible combinations?
If this is a micro optimization ( don't run should group if i should clear selection ) i would not do it.
or i would carefully check the commit history before changing it.
There was a problem hiding this comment.
I looked into this very closely.
Let me explain.
shouldGroup = this._shouldGroup(e, target);
if (this._shouldClearSelection(e, target)) {
this.discardActiveObject(e);
} else if (shouldGroup) {
// in order for shouldGroup to be true, target needs to be true
this._handleGrouping(e, target!);
target = this._activeObject;
}
_shouldClearSelection and _shouldGroup are opposites of each other
fabric.js/src/canvas/SelectableCanvas.ts
Line 725 in 84b7a35
fabric.js/src/canvas/Canvas.ts
Line 1477 in a39e01c
The whole point of this change is to squash them together. The logic is too complex and scattered around everywhere
There was a problem hiding this comment.
so when you agree I will squash them here
There was a problem hiding this comment.
| QUnit.test('active group objects reordering', function(assert) { | ||
| var rect1 = new fabric.Rect({ width: 30, height: 30, left: 130, top: 130 }); | ||
| var rect2 = new fabric.Rect({ width: 50, height: 50, left: 100, top: 100 }); | ||
| var circle1 = new fabric.Circle({ radius: 10, left: 60, top: 60 }); | ||
| var circle2 = new fabric.Circle({ radius: 50, left: 50, top: 50 }); | ||
| canvas.add(rect1, rect2, circle1, circle2); | ||
| assert.equal(canvas._objects[0], rect1); | ||
| assert.equal(canvas._objects[1], rect2); | ||
| assert.equal(canvas._objects[2], circle1); | ||
| assert.equal(canvas._objects[3], circle2); | ||
| var aGroup = new fabric.ActiveSelection([rect2, circle2, rect1, circle1], { canvas: canvas }); | ||
| // before rendering objects are ordered in insert order | ||
| assert.equal(aGroup._objects[0], rect2); | ||
| assert.equal(aGroup._objects[1], circle2); | ||
| assert.equal(aGroup._objects[2], rect1); | ||
| assert.equal(aGroup._objects[3], circle1); | ||
| canvas.setActiveObject(aGroup) | ||
| canvas.renderAll(); | ||
| // after rendering objects are ordered in canvas stack order | ||
| assert.equal(aGroup._objects[0], rect1); | ||
| assert.equal(aGroup._objects[1], rect2); | ||
| assert.equal(aGroup._objects[2], circle1); | ||
| assert.equal(aGroup._objects[3], circle2); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@asturur please explain this to me as part of the discussion of click order vs. stack order
I believe this is what convinced me it was a bug, that rendering changed the order of the objects in active selection to the canvas stacking order.
Now with this PR active selection will respect both click order and stack order
There was a problem hiding this comment.
I would say before it didn't respect either
Originally posted by @asturur in #8665 (comment) @asturur you were wrong, it never was click order. It was canvas stacking order always. Regardless I have promoted the fix to support both and then I added tests and saw this. Lines 377 to 389 in ab6b4b4 |
fix selecting active selection as part of a `n` multi selection click add `multiSelectionStacking`, `multiSelectAdd`
There was a problem hiding this comment.
Alright
Now it is done
You can resolve any conversation you like. I used it to navigate the code
Updated description. I had a fatal error, mixed up the names of the new methods and add more about the stack ordering solution
I also added tests that should cover all cases of handleMultiSelect
| // find target from active objects | ||
| target = this.searchPossibleTargets( | ||
| prevActiveObjects, | ||
| this.getPointer(e, true) | ||
| ); |
There was a problem hiding this comment.
instead of this.findTarget(e, true)
src/canvas/SelectableCanvas.ts
Outdated
| aObjects.length > 1 && | ||
| isActiveSelection(activeObject) && | ||
| !skipGroup && | ||
| activeObject === this._activeSelection && |
There was a problem hiding this comment.
this condition is a bit silly because if aObjects > 1 we are dealing with the active selection but I think you put that for TS
|
and of course something is wrong |
The multiple selection is in click order. On top of that i can't see anymore the activeSelection.toGroup method. what happened to it? |
|
ok toGroup and toActiveSelection have been killed in the group transition for some reason, i will add them back in a separate pr using whatever code we have now to do a similar thing |
|
@ShaMan123 cloned this branch locally, i will be offline 2 days reviewing the new selection process. |
I documented this I think. new Group(canvas.getActiveObjects())Why would we expose a method for this? |
|
And to add to active selection That is the whole point of a strict object tree |
For me this is inconsistent behavior. First it does stack order and then click order. That is why I considered it a bug when I saw it. Another thing I would like to understand from you is why does the multiselection logic runs on mouse down and not up |
| /** | ||
| * Adds objects with respect to {@link multiSelectionStacking} | ||
| * @param targets object to add to selection | ||
| */ | ||
| multiSelectAdd(...targets: FabricObject[]) { | ||
| if (this.multiSelectionStacking === 'selection-order') { | ||
| this.add(...targets); | ||
| } else { | ||
| // respect object stacking as it is on canvas | ||
| // perf enhancement for large ActiveSelection: consider a binary search of `isInFrontOf` | ||
| targets.forEach((target) => { | ||
| const index = this._objects.findIndex((obj) => obj.isInFrontOf(target)); | ||
| const insertAt = | ||
| index === -1 | ||
| ? // `target` is in front of all other objects | ||
| this.size() | ||
| : index; | ||
| this.insertAt(insertAt, target); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
why calling this add and not using the fact that this is a subclass of group, exactly to handle multi selections, and so its own add method is either super.add or this.insertAt?
I would gladly rename this in add. Maybe i ll merge this as it is, and i ll just open a PR with the things i wouldn't want to do
There was a problem hiding this comment.
thought of that as well.
wasn't sure what I/you think about that.
The reason I am not for is in case the dev wants to change the stack order, using insertAt is valuable so if we change it it can be annoying.
There was a problem hiding this comment.
but anyone can do this._objects.insertAt so I am ok with it
There was a problem hiding this comment.
so this will be add and insertAt will be left as is?
There was a problem hiding this comment.
i was just saying to rename multiSelectAdd in add.
Because this class has to handle only multi selection logic and nothing else. so seems legit that its own add is special.
Are you thinking that people may want to call add of an active selection directly to do things in the active selection outside the canvas flow?
There was a problem hiding this comment.
Yes
Perhaps
What do you think?
| if ( | ||
| this.canvas?.preserveObjectStacking && | ||
| this._objects[i].group !== this | ||
| ) { | ||
| ctx.save(); | ||
| ctx.transform(...invertTransform(this.calcTransformMatrix())); | ||
| this._objects[i].render(ctx); | ||
| ctx.restore(); |
There was a problem hiding this comment.
this is the edge case handling, this is exactly how you do it.
The only improvement would be that instead of going back with an invert and a calc transform, you go straight to viewport transform using ctx.setTransform(this.canvas.viewportTransform).
I will add it in a follow up
There was a problem hiding this comment.
I think this logic should be handled by active selection.
We should discuss this, there are a lot of implications.
|
@ShaMan123 as is i m merging this because there are many improvements here compared to the things i want to remove.
|
why? new Group(canvas.getActiveObjects()) // toGroup
(canvas.getActiveObject() || canvas.getActiveSelection()).add(...group.objects) // toActiveSelection |
|
They were simply part of the api, not deprecated and useful, and have been removed in a larger PR without noticing |
Motivation
ports and closes #7878
ports logic from #7882 not including rendering logic that will be done after this PR
closes #6840
closes #4529
Description
Selection
This PR refactors what was known as canvas grouping logic (formerly the canvas grouping mixin).
We are talking about a bunch of methods that were in charge of selecting objects in response to user events.
Selection is distinct into 2.
What is known as multiselection (shift+click) and what is known as selection/group selection (drag) defined as
_groupSelector(the blue rect that selects objects)Logic was scattered and unscoped (methods shared logic, e.g #8665 (comment)) for some reason, what made this effort difficult, confusing and time consuming. This is why I squashed them as follows:
handleSelection(renamed_maybeGroupObjects): now called before_shouldClearSelection, not callingsetCursor_collectObject: movedcollectObjectslogic to collection (moved some tests as well)_groupSelectedObjectshandleMultiSelection(renamed_handleGrouping): usesearchPossibleTargetsinstead of hackingfindTarget30ee763_shouldGroup: The sensitive commit 9a0736a moved the checkactiveObject.__cornerto_shouldGroup, see comment_updateActiveSelection:object stacking question to be resolvedfix(): selection logic to support nested multiselection #8665 (comment)_createActiveSelectionfix(): selection logic to support nested multiselection #8665 (comment)I updated the logic to act with the constant active selection ref (see below) - #8665 (comment).
So basically the result of this PR is 2 methods that are solely in charge of selection logic.
I decided to rename the main methods because:
Active Selection Ref
Made canvas hold a constant ref to active selection. This is done to allow devs to listen to events and handle it as they wish, what was impossible until now (you could subscribe after an active selection was created). It also allows to subclass active selection and make canvas use it.
#8665 (comment)
isActiveSelectioncalls with a simpleobj === canvas._activeSelectionMultiSelection Stacking Order
multiSelectStacking: 'canvas-stacking' | 'selection-order'that controls how multiselected objects are added to the active selection. Logic is handled byActiveSelection#multiSelectAdd_updateActiveSelectionfindTarget(e, skipGroup) => FabricObject=>findTarget(e) => FabricObject:skipGroupis not needed anymore, it was buggy so now callingsearchPossibleTargetdirectly and scoped internal logic30ee763
Other Changes
_chooseObjectsToRender_groupSelectorkeys for clarity 58c050d_previousPointerGist
TO DO
In Action