feat(LayoutManager): BREAKING remove shouldResetTransform logic from layoutManager

[ShaMan123]

Description

Thanks to #9561 it is now possible to remove the badly designed shouldResetTransform hook from the layout manager, me to blame.
Thanks for the patience!

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	907.266 (-0.326)	304.659 (-0.101)

[ShaMan123]

cleanup is done
now decisions

[ShaMan123]

do we want this?

[ShaMan123]

I have noticed that we introduced a regression with polys flagged with exactBoundingBox

[ShaMan123]

removed resetting transform, ready to go

[asturur]

Let's take a moment to recap layout manager!
What about that binding issue? can we remove that too?

Can we also define what the layout manager does?
My understanding is that it takes care of the the size, of the group and the position of the group.

I have seen discussion about trying to use it to re-organize the objects inside the group but i never reviewed the code looking for that kind of feature, so i have no idea if is possible and if something.

[ShaMan123]

We can bind, sure. You want it in this PR?

My understanding is that it takes care of the the size, of the group and the position of the group.

position of the children, size of the group so doing stuff like table or flex layout is part of the scope.
We (@ebigtm and I) did a flex one already and we might publish it.
A table layout is part of what a project I am involved with in is doing now.
I also saw @iocoker do a calendar, which is a table layout.

So it is possible and the API is pretty solid so hooking into should be relatively straight forward.
The only pain is handling the correction, that makes it just hard for "no reason".
That is why I am thinking, what if we use left top origin instead of center. Maybe that will make it easier.
And I will try this small hack I have in mind that is equiv to pathOffset that might release us from the need to correct the position

[asturur]

I have noticed that we introduced a regression with polys flagged with exactBoundingBox

What is the regression?

[asturur]

why removing the comment?

[ShaMan123]

I think the jsdoc concats the comments of the super method so there is no need, it is spam

[asturur]

that is bad, because if i m just reading the codebase i can't concat anything

[ShaMan123]

And if you hover with your mouse?

[ShaMan123]

I can restore it but my point is that then you have to update a signle comment everywhere

That is not manageable

[asturur]

yeah VSCODE does this, i m just not sure how it is supposed to be by the JSDOCS logic

[asturur]

We can make another pr for the binding issue

[ShaMan123]

I have noticed that we introduced a regression with polys flagged with exactBoundingBox

What is the regression?

I should open a ticket.

1. stroke uniform poly
2. perform multiselection, select and deselect the poly
3. poly will jump a few pixels, maybe related to the stroke width value

[ShaMan123]

So can we merge?

[github-actions]

Coverage after merging chore/rm-shouldResetTransform into master will be

82.81%

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	87.58%	89.47%	76.92%	90%	135–136, 138–139, 139, 139, 139, 139, 233, 291–292, 303, 46
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%	64.71%	100%	96.15%	36, 43, 43, 43, 51, 71–72
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	79.02%	76.99%	83.05%	79.88%	1005–1006, 1006, 1006–1007, 1013, 1015, 1043–1045, 1048–1049, 1053–1054, 1177–1179, 1182–1183, 1256, 1371, 1492, 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, 932, 953–954, 970–971, 971, 971–973, 976–977, 977, 977, 979, 987, 987, 987–989, 989, 989, 996–997
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	93.17%	91.84%	94.34%	94.01%	1001, 1009, 1120, 1122, 1124–1125, 460–461, 463–464, 464, 464, 513–514, 575–576, 589, 629–631, 663, 668–669, 696–697, 757–758, 763–767, 769, 928, 928–929, 932, 952, 952
StaticCanvas.ts	96.53%	93.09%	98.92%	98.29%	1019, 1029, 1080–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	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, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73&nda

[asturur]

The layoutManager.getLayoutResult is not using targets at all, it is using target.getObjects().

[ShaMan123]

hmmm
that is maybe something to discuss
targets are objects that changed (I think it is used only for subscribing)

[asturur]

Adding or removing this line doesn't do anything else than helping TS, is also a bit confusing in the test definition that one test 'LAYOUT_TYPE_INITIALIZATION ' pass empty targets to override this one here while 'LAYOUT_TYPE_IMPERATIVE' doesn't.

Also on my vscode both '[]' and '[rect]' give a red error in typescript.

I will investigate this typing issue right after merging, i don't think a function that doesn't use 'targets' should requires it in the typings