fix(): perform layout on poly change + initialization object subscription

[ShaMan123]

Motivation

closes #9534

Description

Subscribe to the poly change event to layout group

Changes

• fix: assign the layoutManager to group before initial layout so it subscribes objects properly. We should reconsider binding a group to a layout manager
• fix accounting for stroke width twice for Poly with exactBoundingBox

Gist

In Action

https://codesandbox.io/p/sandbox/fabric-vanillajs-sandbox-forked-9fgf6r?file=%2Fsrc%2Findex.ts%3A28%2C69

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	907.911 (-0.065)	305.184 (+0.020)

[ShaMan123]

DONE

[ShaMan123]

Fixed group layout edge case + border issue when Poly#exactBoundingBox

[asturur]

i removed destructuring because is extremely noisy in the constructor, please don't change this since the change was fresh and commented. the rest operator on the function parameter becomes larger code and also slower. We do not get any benefit from extracting the layoutManager and assign it as a default in the constructor, since the layout manager is not used in the super class of the group.

[ShaMan123]

I don't mind about destruction, though having super setting values has become something I hate. I have encountered a number of hard to debug issues caused by fabric's usage of it. But that is a different topic.
Your change not to destruct the object introduced a regression and confusion.
It has to be set on the instance before calling performLayout or else the layout manager will not subscribe the objects.
Now, after you made group bind a layout manager I think we should remove that check all together but we need to consider how it will affect sharing a layout manager between groups and if we allow it or not.
So yes I prefer destructing because there is less ambiguity. It is just that a naive syntax introduced bugs.
You did not write why it was not destructed on purpose in the comment but I guessed that.

[ShaMan123]

To conclude, the syntax is 99% of the times something I can live with and bugs suck

[asturur]

i think the bug is fixed by moving line 150 to 142. Correct?
Destructuring options in the constructor is what i asked you to not change.

Also want to clarify the destructuring or not has nothing to do with the bug.
The bug was to assign the layout manager after perform layout.

[asturur]

Is also unclear why we need to keep that check on the subscription
(this one: https://github.com/fabricjs/fabric.js/blob/master/src/LayoutManager/LayoutManager.ts#L109)
since we have 1 group 1 layout manager, and that was the goal of the changes i made before merging the pr.

Is there any case in which we end up executing that line with a layout manager and a group that are not bound together?

[ShaMan123]

Is there any case in which we end up executing that line with a layout manager and a group that are not bound together?

That is what I mentioned

Now, after you made group bind a layout manager I think we should remove that check all together but we need to consider how it will affect sharing a layout manager between groups and if we allow it or not.

[asturur]

Can we have PR with only the part 'fix(): perform layout on poly change + initialization object subscription' ?
The issues with exact bounding box are there, but i don't think excluding the stroke calculation for all the methods that usually account for it is a good without evaluating other options first

[ShaMan123]

@asturur I know how much you hate strokeUniform.
I have spent the past week and a half, maybe even 2 weeks on bug fixes all related to fabric's bugs when handling strokeUniform.
The amount of trouble was so big that at some point I decided to try your suggestion to listen to object:scaling and adapt objects' strokeWidth accordingly.
I regret I didn't write then when it was fresh in my mind beause I don't remember why I dumped that approach but I do remember it was extremely hard to manage and required me to override too many methods all across fabric.
In other words it made me implement strokeUniform just in a different way and it was not easier.
Until that tryout I thought the same.

It all comes down to math. I fixed the math and everything worked.
The bug in fabric is a real issue and should be fixed.
Once fixed I don't think it will bother us any longer but since it is not it keeps troubling.
Just as a spoiler: I could not use ObjectOrigin methods because they all depend on the broken _getTransformedDimensions so even finding a center point was not trivial. This method is used everywhere in fabric and is the source of all problems (including scale control jumps when you start scaling and many more)
That and moving away from decomposition will fix a vast amount of bugs that might have been considered won't fix

[asturur]

we need to make a good step forward.
You have to stop calling _getTransformedDimensions broken. Is the wrong approach to this codebase.
_getTransformedDimensions is not broken, it was doing its job, then stuff got added around it that weren't compatible, and now we point the finger to it. The finger needs to be pointed to strokeUniform.

Once that is clarified we can move on.

It doesn't look good to have '_getTransformedDimensions' to have to know if someone else will have to take in account for stroke. It doesn't even look good the boolean we have right now in getNonTransformedDimensions for polygon/polyline.

The exact bounding box has so much different math from text and rect that is probably fine as its own class with its own geometry methods. Who needs exact bounding box can initiate a different kind of polygon or polyline.

We can also merge this fix but my intention is to wipe the exact bounding box as they are today because mixed with the other logic they look as bad as the bug we had before having the option to use them.

[asturur]

Please improve with PR description.
This pr has a title that cover 2 of the 3 changes, and the changes are listed one in description and 2 in the changes.
There is a suggestion with no explanations in the description of a bug.

[ShaMan123]

we need to make a good step forward. You have to stop calling _getTransformedDimensions broken. Is the wrong approach to this codebase. _getTransformedDimensions is not broken, it was doing its job, then stuff got added around it that weren't compatible, and now we point the finger to it. The finger needs to be pointed to strokeUniform.

Once that is clarified we can move on.

It doesn't look good to have '_getTransformedDimensions' to have to know if someone else will have to take in account for stroke. It doesn't even look good the boolean we have right now in getNonTransformedDimensions for polygon/polyline.

The exact bounding box has so much different math from text and rect that is probably fine as its own class with its own geometry methods. Who needs exact bounding box can initiate a different kind of polygon or polyline.

We can also merge this fix but my intention is to wipe the exact bounding box as they are today because mixed with the other logic they look as bad as the bug we had before having the option to use them.

Until we discuss this together I don't see how to explain it to you more than I have.
There are more things affected by this call such as drawBorders.
All of them can be fixed.
All require a refactor.
Any approach must break something to fix a bug.
That is why we need to discuss this face to face and plan it.
Saying it is not broken is saying to me I don't understand how to use fabric and that I spent almost 2 weeks of bug fixing because of what then? I don't think that is the case. True, all these objects are strokeUniform.
Why not fix the math and make it work?
You can remove it, you can remove many things that aren't fixed. But it has a cost that should be discussed as well.
If you made up your mind and I have no input that is a different matter.

[ShaMan123]

What about this? Should I extract the exactBoundingBox logic?

[ShaMan123]

I dislike this as well, and it is due to not destructing

[ShaMan123]

reverted the exactBBox fix to a dedicated Pr

[github-actions]

Coverage after merging fix/layout-on-poly-change into master will be

82.79%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
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.36%	95.65%	100%	100%	178
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
LayoutManager.ts	88.05%	89.83%	76.92%	90.54%	136–137, 139–140, 140, 140, 140, 140, 234, 297–298, 309, 47
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	86.54%	64.71%	100%	96.30%	36, 43, 43, 43, 51, 78–79
utils.ts	89.47%	80%	100%	100%	28, 34
src/Pattern
Pattern.ts	89.74%	91.89%	80%	90.32%	119, 130, 139, 32, 36, 95
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	79%	76.99%	83.05%	79.84%	1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	93.21%	91.60%	94.44%	94.22%	1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1019, 1029, 1081–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
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	8.57%	0%	0%	15.79%	101, 107, 112, 127, 127, 127, 127, 127, 129–132, 132, 135, 142, 21, 29–33, 33, 33, 33, 33, 33, 33, 33, 54–60, 60, 60, 60, 60, 62, 67–68, 70, 80, 86–88, 88, 90, 93–94, 94, 94, 94, 94, 96
rotate.ts	19.57%	12.50%	50%	21.43%	41, 4

[asturur]

So what is left for the layout manager? check that optional binding with the group?

[ShaMan123]

Yes and deciding about the active selection ref because it is related to shouldResetTransform => #9561

[ShaMan123]

I have another idea that might make all the hard to reason about correction logic void.
But that is a change that I am not sure you will like before v6.
It borrows the pathOffset concept and adds a transformation layer between the group and the objects, same as pathOffset for the path/poly points.
It might mean that correction will not be needed and that objects can have any origin on group and not center only as it is now.