feat(LayoutManager): Handle the case of activeSelection with objects inside different groups #9651

asturur · 2024-02-05T10:35:01Z

Description

This PR is supposed to help with the issue described in #9600

The strategy tried here is to:

when the active selection is created, it subscribes its own objects.
when this subscription happens detect if those objects have a parent that is not the active selection
when that happens subscribe the group ( so the active selection ) on the parent layout manager ( the original group ) to perform a performLayout with target the the parent.

This should allow for N groups to subscribe the same active selection to modify perform layout on the groups.

The test case in the vanilla template is a good way to play around with those changes

codesandbox · 2024-02-05T10:35:03Z

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

github-actions · 2024-02-05T10:36:55Z

Build Stats

file / KB (diff)	bundled	minified
fabric	914.707 (+3.422)	305.517 (+0.576)

asturur · 2024-02-05T10:56:36Z

Ok this reproduction case uses clone, so it shares the bug with the objects not being registered.
So we need to solve that first.

asturur · 2024-02-15T00:43:52Z

The bug was split around 2 locations:

missing registration of active selection events
missing dirty flag for layout strategies that do not require a perform layout

I have to check the jest failures, but this fixes the group interactivity

asturur · 2024-02-16T10:37:10Z

This PR requires an end to end test

ShaMan123 · 2024-02-16T12:29:22Z

I would like to review this thoroughly when I find the time

asturur · 2024-02-17T17:37:50Z

This could be probably done differently by defining an active selection strategy, but the registration part is in the layout manager and i don't want to flip more things right now.
But this is not the only way to fix this bug or the best one

asturur · 2024-02-17T17:38:39Z

I would like to review this thoroughly when I find the time

Yes but please when you have time for fabricJS repo, do this first, we have still 2 bugs in the layout manager that block us from drawing a line and cutting 6.0

src/LayoutManager/LayoutManager.ts

asturur · 2024-02-17T17:44:26Z

src/LayoutManager/LayoutManager.ts

-          this.performLayout({
+      ...layoutingEvents.map((key) =>
+        object.on(key, (e) => {
+          target.layoutManager.performLayout({
            trigger: key,
            e: { ...e, target: object },


target here should be outside of e and called something like 'triggerSource' or something on that way.
Usually what we call e is the event that generated the call to the callback, and in this case target isn't part of it.
So adding it there is a bit confusing. I understand we want to comunicate to the dev which object moved and triggered a layout, but that can be done with the specific name.

out of scope for this pr

Sounds correct

asturur · 2024-02-17T17:50:06Z

This could be probably done differently by defining an active selection strategy, but the registration part is in the layout manager and i don't want to flip more things right now. But this is not the only way to fix this bug or the best one

On top of that it would be just smarter if when creating an active selection we would understand is pointless to register for events its own children and just register the activeSelection itself.
But that would require a different layoutManager or moving the registering part to the stratetgy. ( that could be done easily ).

ShaMan123

See comments
The biggest question to answer is: Do you want to create a subclass ActiveSelectionLayoutManger?
It is a design question.
I think it makes it easier to understand
What do you think?

e2e/tests/selection/multi-selection-in-groups/index.ts

ShaMan123 · 2024-02-25T08:19:07Z

e2e/tests/selection/multi-selection-in-groups/index.ts

ShaMan123 · 2024-02-25T08:22:52Z

src/LayoutManager/LayoutManager.ts

-          this.performLayout({
+      ...layoutingEvents.map((key) =>
+        object.on(key, (e) => {
+          target.layoutManager.performLayout({
            trigger: key,
            e: { ...e, target: object },


Sounds correct

ShaMan123 · 2024-02-25T08:25:16Z

src/LayoutManager/LayoutManager.ts

@@ -130,11 +142,27 @@ export class LayoutManager {
    const { target } = context;
    const { canvas } = target;
    // handle layout triggers subscription
+    // @TODO: gate the registration when the group is interactive


This is redundant IMO since if the group is not interactive nothing will trigger imperative layout because the user will not be able to select and transform a nested object.
It does subscribe the objects with no need but if the group becomes interactive we save the dev from a gotcha needing to subscribe the targets when toggling interactivity

toggling interactivity will subscribe all the targets, that is why i deprecated .interactive = true for a setInteractive(true). We already need to take in account that subTargetCheck also needs to be true, better to make it a controlled process.
As a counter argument interactive group is an edge case, for now we don't need to go to the extreme that objects get registered for handlers only when they get selected or when a trasform action starts, but at least when the group is interactive yes.

As a counter argument interactive group is an edge case, for now we don't need to go to the extreme that objects get registered for handlers only when they get selected or when a trasform action starts, but at least when the group is interactive yes.

This is not bad

However toggling interactivity... I am not sure. There is another PR I did which I mentioned in this PR regarding it
I will find it

src/LayoutManager/LayoutManager.ts

ShaMan123 · 2024-02-25T08:31:08Z

src/LayoutManager/LayoutManager.ts

+      // so we need to subscribe the active selection event to trigger the parent performLayout
+      withDifferentParent.forEach(({ group, parent }) => {
+        if (parent) {
+          // we may subscribe an active selection more than once,


this doesn't sound good, bad for perf and for logic

This is in contrast with wanting to register all the objects all the time.
Right now if an active selection has 2 objects in the same group i think will try to register twice, but trying a different approach may fix that.

src/LayoutManager/LayoutManager.ts

ShaMan123 · 2024-02-25T08:33:30Z

src/LayoutManager/LayoutStrategies/LayoutStrategy.ts

@@ -27,6 +27,8 @@ export abstract class LayoutStrategy {

  /**
   * Used by the `LayoutManager` to perform layout
+   * @TODO/fix: if this method is calcResult, should calc unconditionally.


interesting
please elaborate

ShaMan123 · 2024-02-25T08:35:49Z

src/shapes/Group.ts

+   * This will be not removed but slowly replaced with a method setInteractive
+   * that will take care of enabling subTargetCheck and necessary object events.
+   * There is too much attached to group interactivity to just be evaluated by a


I disagree
IMO there is no need
In some open PR I made interactive include logically subTargetCheck because that is how we use it so the code should reflect it.
It makes it easier to deal with and restores subTargetCheck to what it originally was.
So the dev can flag interactive and that is enough

There is no need to register events for objects that will never move or be target of actions.
Group interactivity was put in as a part of the layout manager while it has nothing to do with layouting, nor layouting should take care of attaching event for detecting movements.
Those 2 parts now are strongly bound together and i can't think of my layout logic without having to think of the interaction and registration part.

Registering everything all the time doesn't bring performance issues, but is also completely unnecessary.

src/shapes/Group.ts

ShaMan123 · 2024-02-25T13:06:14Z

This could be probably done differently by defining an active selection strategy, but the registration part is in the layout manager and i don't want to flip more things right now. But this is not the only way to fix this bug or the best one

On top of that it would be just smarter if when creating an active selection we would understand is pointless to register for events its own children and just register the activeSelection itself. But that would require a different layoutManager or moving the registering part to the stratetgy. ( that could be done easily ).

I am not against

asturur · 2024-02-25T23:17:18Z

@ShaMan123 this is the version with the subclassed ActiveSelectionLayoutManager.
#9687

Seems leaner but of course it isn't.
But is an option

asturur · 2024-02-25T23:33:34Z

Oh well is an option only at the cost of making subscribe public, because we need to register the event on the parent layout manager while the activeSelection won't have any real registration.
Not sure how cleanup will work then.

asturur · 2024-02-27T11:35:28Z

There is still more work to do to have something that looks readable and is robust.

asturur · 2024-02-29T08:48:58Z

Ok this should be ready to go now.
I managed to condense the logic to a single override that can't be broken by making unrelated changes to the LayoutManager, and tests are covering its functionality in detail.

Code size is larger than the initial approach but there is no workaround for that.
There is still a possibility to reduce the size of the object registration by merging modified and modifying, but it is out of scope for this that is a fix of a bug.
I tried and requires some clever typescripting

src/LayoutManager/ActiveSelectionLayoutManager.ts

src/LayoutManager/LayoutManager.ts

ShaMan123

I reviewed the code.
The only thing that I think needs to change is the dirty flagging.

src/LayoutManager/ActiveSelectionLayoutManager.spec.ts

ShaMan123

@asturur please review
Should we extract the subscription logic before the release? It will be breaking. Up to you.

ShaMan123 · 2024-03-24T07:49:49Z

src/LayoutManager/ActiveSelectionLayoutManager.spec.ts

+        const event = { foo: 'bar' };
+        triggers.forEach((trigger) => as.fire(trigger, event));
+        expect(asPerformLayout).not.toHaveBeenCalled();
+        expect(groupPerformLayout.mock.calls).toMatchObject([


I wanted to do the same for groupPerformLayout2 but it throws a weird error or max call stack exceeded

It happened to me too, and there was a bug in the code in that moment. Maybe is worth double checking

still happening

Ok i ll look with fresh eyes, maybe there is a situation in which you get an infinite nested loop.

no issues for me, it works fine

I think you should have committed it

i did i pushed up commits with the checks for groupPerformLayout2 mirrored to groupPerformLayout

Missed that! Great!

ShaMan123 · 2024-03-24T07:50:57Z

src/LayoutManager/LayoutManager.ts

+   * Subscribe an object to transform events that will trigger a layout change on the parent
+   * This is important only for interactive groups.
+   * @param object
+   * @param context
   */
  protected subscribe(
    object: FabricObject,
    context: RegistrationContext & Partial<StrictLayoutContext>


I think we should change the type for all subscription methods

Suggested change

context: RegistrationContext & Partial<StrictLayoutContext>

context: RegistrationContext

asturur · 2024-03-24T08:13:43Z

@asturur please review Should we extract the subscription logic before the release? It will be breaking. Up to you.

No. We just declare the LayoutManager as beta and amen. Do not extend it, do not touch it, it will change.

asturur · 2024-03-24T08:16:33Z

How can i see the changes from the latest commit? it gives me that everything was done 50 minutes ago

ShaMan123 · 2024-03-24T08:37:07Z

view commits 6aa468c...HEAD

ShaMan123 · 2024-03-24T08:38:35Z

The e2e test is failing because dirty flag is not being set for some reason

asturur · 2024-03-24T08:41:15Z

The e2e test is failing because dirty flag is not being set for some reason

Because you deleted the catch'all dirty flag. Indeed when reviewing i was thinking why do that

asturur · 2024-03-24T08:43:13Z

I restored it, let's see if is that one.

ShaMan123 · 2024-03-24T08:45:17Z

because it didn't make a difference from what I could see

asturur · 2024-03-24T08:48:49Z

because it didn't make a difference from what I could see

We talked about this, who is going to clear the cache for the groups that do not have a layout result like clipPath or fixed?

Indeed the test pass now, and that cache invalidation was the part 2 of the 2 part fixes i discussed during the PR all the time, active selection events registration was just half of the fix.
Do you remember?

ShaMan123 · 2024-03-24T08:51:21Z

yes, weird. I restored it locally and re-ran. Maybe I forgot to build.
In that case we can restore your change to layoutObject if you wish, not using set.
I am good with this anyways.

ShaMan123 · 2024-03-24T08:52:13Z

Confirmed, all good

ShaMan123 · 2024-03-24T08:53:37Z

src/LayoutManager/LayoutManager.ts

@@ -231,7 +251,7 @@ export class LayoutManager {
      target.setPositionByOrigin(nextCenter, CENTER, CENTER);
      // invalidate
      target.setCoords();
-      target.set({ dirty: true });
+      target.set('dirty', true);


this can be removed now because of the one in onAfterLayout

asturur · 2024-03-24T08:55:16Z

Monday i will add some comments on the test because is a bit hard to remember why mock should be called or not and then i will merge.

github-actions · 2024-03-26T00:26:56Z

Coverage after merging nested-active-selection into master will be

82.73%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	14.29%	100%	0%	25%	23, 26, 29, 41, 44, 47
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.85%	94.83%	87.10%	97.14%	109, 112, 215–216, 241–242
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.55%	95.65%	100%	100%	213
cache.ts	97.06%	90%	100%	100%	57
config.ts	74.14%	65%	66.67%	82.76%	131, 139, 139, 139, 139, 139–141, 152–154, 30
constants.ts	100%	100%	100%	100%
src/LayoutManager
ActiveSelectionLayoutManager.ts	96.15%	100%	85.71%	100%
LayoutManager.ts	89.03%	92.86%	76.92%	90.41%	169–170, 172, 174, 174, 174, 269, 331–332, 343, 51
constants.ts	100%	100%	100%	100%
src/LayoutManager/LayoutStrategies
ClipPathLayout.ts	82.05%	72.22%	100%	88.89%	30–31, 42, 54, 57–58, 65
FitContentLayout.ts	100%	100%	100%	100%
FixedLayout.ts	20%	0%	0%	40%	20–21, 23, 23, 23, 23, 23
LayoutStrategy.ts	85.11%	64.71%	100%	95.65%	37, 44, 44, 44, 52, 72–73
utils.ts	91.67%	85.71%	100%	100%	28, 34
src/Pattern
Pattern.ts	90.54%	93.94%	80%	90.32%	119, 130, 139, 32, 36
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	78.66%	77.04%	82.14%	79.33%	1001–1003, 1006–1007, 1011–1012, 1123–1125, 1128–1129, 1202, 1314, 1435, 158, 183, 289, 289–294, 299, 322–323, 328–333, 353, 353, 353–354, 354, 354–355, 363, 368–369, 369, 369–370, 372, 381, 387–388, 388, 388, 42, 431, 439, 443, 443, 443–444, 446, 46, 528–529, 529, 529–530, 536, 536, 536–538, 558, 560, 560, 560–561, 561, 561, 564, 564, 564–565, 568, 577–578, 580–581, 583, 583–584, 586–587, 599–600, 600, 600–601, 603–608, 614, 621, 658, 658, 658, 660, 662–667, 673, 679, 679, 679–680, 682, 685, 690, 703, 731, 783–784, 784, 784, 784, 784, 784, 787–788, 791, 791–793, 796–797, 879, 886, 886, 886, 899, 928–929, 929, 929–931, 934–935, 935, 935, 937, 945, 945, 945–947, 947, 947, 954–955, 963–964, 964, 964–965, 971, 973
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	90.22%	87.30%	94.55%	91.74%	1004, 1012, 1123, 1125, 1127–1128, 1199, 1220–1221, 1239–1240, 460–461, 463–464, 464, 464, 513–514, 591, 596, 666, 671–672, 699–700, 724, 727–730, 730, 730–731, 731, 731, 737–738, 740, 760–761, 766–770, 772, 807–808, 811, 811, 811–812, 931, 931–932, 935, 955, 955
StaticCanvas.ts	96.44%	92.72%	98.91%	98.27%	1045, 1055, 1106–1108, 1111, 1146–1147, 1223, 1232, 1232, 1236, 1236, 1283–1284, 188–189, 205, 585, 597–598, 928–929, 929, 929–930
StaticCanvasOptions.ts	100%	100%	100%	100%
TextEditingManager.ts	86%	71.43%	91.67%	91.67%	17, 17, 17–18, 18, 18
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	97.50%	88.89%	100%	100%	33
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	94.44%	93.10%	91.67%	96.77%	183, 249, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	10.87%	0%	0%	16.13%	114, 114, 114, 114, 114, 116–119, 119, 122, 129, 24–26, 50–52, 58–59, 61, 71, 77–79, 79, 81, 84, 86, 90–92, 94, 99
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57,

asturur added bug v6 labels Feb 5, 2024

asturur marked this pull request as draft February 5, 2024 11:38

asturur marked this pull request as ready for review February 15, 2024 00:42

asturur commented Feb 17, 2024

View reviewed changes

src/LayoutManager/LayoutManager.ts Outdated Show resolved Hide resolved

asturur commented Feb 17, 2024

View reviewed changes

src/LayoutManager/LayoutManager.ts Outdated Show resolved Hide resolved

asturur commented Feb 17, 2024

View reviewed changes

asturur requested a review from ShaMan123 February 17, 2024 17:50

ShaMan123 reviewed Feb 25, 2024

View reviewed changes

asturur mentioned this pull request Feb 25, 2024

feat(LayoutManager): Handle the case of activeSelection with objects inside different groups #9687

Closed

asturur requested a review from ShaMan123 February 29, 2024 08:49

ShaMan123 reviewed Mar 3, 2024

View reviewed changes

src/LayoutManager/ActiveSelectionLayoutManager.ts Outdated Show resolved Hide resolved

ShaMan123 reviewed Mar 3, 2024

View reviewed changes

src/LayoutManager/ActiveSelectionLayoutManager.ts Outdated Show resolved Hide resolved

ShaMan123 reviewed Mar 3, 2024

View reviewed changes

src/LayoutManager/ActiveSelectionLayoutManager.ts Outdated Show resolved Hide resolved

ShaMan123 reviewed Mar 3, 2024

View reviewed changes

src/LayoutManager/LayoutManager.ts Outdated Show resolved Hide resolved

ShaMan123 reviewed Mar 3, 2024

View reviewed changes

src/LayoutManager/ActiveSelectionLayoutManager.spec.ts Outdated Show resolved Hide resolved

src/LayoutManager/ActiveSelectionLayoutManager.spec.ts Show resolved Hide resolved

explicitly clear cache on after layout

6b26a1a

ShaMan123 force-pushed the nested-active-selection branch from 487a881 to 225432f Compare March 24, 2024 07:27

ShaMan123 added 2 commits March 24, 2024 09:30

register AS on group

6aa468c

changelog

46f2167

ShaMan123 force-pushed the nested-active-selection branch 3 times, most recently from f0c5982 to a610c2f Compare March 24, 2024 07:44

ShaMan123 approved these changes Mar 24, 2024

View reviewed changes

cleanup

39e3732

ShaMan123 force-pushed the nested-active-selection branch from e769487 to 4e061da Compare March 24, 2024 07:53

bubble through parent

653a067

ShaMan123 force-pushed the nested-active-selection branch from 4e061da to 653a067 Compare March 24, 2024 08:15

Put back the cache clear for group that do not commit layout

98768a5

ShaMan123 reviewed Mar 24, 2024

View reviewed changes

ShaMan123 approved these changes Mar 24, 2024

View reviewed changes

added comments

a22cc7c

asturur merged commit 0f5c333 into master Mar 26, 2024
22 checks passed

asturur deleted the nested-active-selection branch March 26, 2024 00:35

	context: RegistrationContext & Partial<StrictLayoutContext>
	context: RegistrationContext

feat(LayoutManager): Handle the case of activeSelection with objects inside different groups #9651

feat(LayoutManager): Handle the case of activeSelection with objects inside different groups #9651

Conversation

asturur commented Feb 5, 2024 • edited Loading

Description

codesandbox bot commented Feb 5, 2024

Review or Edit in CodeSandbox

github-actions bot commented Feb 5, 2024 • edited Loading

asturur commented Feb 5, 2024 • edited Loading

asturur commented Feb 15, 2024

asturur commented Feb 16, 2024

ShaMan123 commented Feb 16, 2024

asturur commented Feb 17, 2024

asturur commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Feb 17, 2024

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Feb 25, 2024

asturur commented Feb 25, 2024

asturur commented Feb 25, 2024

asturur commented Feb 27, 2024

asturur commented Feb 29, 2024

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Mar 24, 2024

asturur commented Mar 24, 2024

ShaMan123 commented Mar 24, 2024

ShaMan123 commented Mar 24, 2024

asturur commented Mar 24, 2024

asturur commented Mar 24, 2024

ShaMan123 commented Mar 24, 2024

asturur commented Mar 24, 2024

ShaMan123 commented Mar 24, 2024

ShaMan123 commented Mar 24, 2024

Choose a reason for hiding this comment

asturur commented Mar 24, 2024

github-actions bot commented Mar 26, 2024

asturur commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

asturur commented Feb 5, 2024 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

asturur Feb 25, 2024 •

edited

Loading

asturur Mar 24, 2024 •

edited

Loading