refactor(): Change how LayoutManager handles restoring groups

[asturur]

Motivation

Trying to remove the special boolean for the constructor for objects relative to center.
The issue with restoring an object from data is that the data is correct, and so the layout operation is not needed at all.
Instead of having logic inside the layout manager to do calculation but don't mutate objects or the position, we can just skip the layout manager all together.

I wish there was a way make that option not usable by a dev but i couldn't think of a simple solution.

TODO:

• document what to expect from group layout manager
• add a jest test that shows that the layout manager isn't called when requested

FOLLOWUP:
proper layout manager serialization and restore

Description

Changes

Gist

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	906.745 (+0.107)	304.632 (-0.112)

[ShaMan123]

This is good work!
I thought there was an option to declare a private constructor overload but TS doesn't allow mixing private/public for a single signature.
What if we pass a value in the layoutManager option instead?

[ShaMan123]

Can we also rename Group#triggerLayout to performLayout?

[ShaMan123]

I suggest we subclass FitContentLayout

export class FitContentLayout2 extends FitContentLayout {
  shouldPerformLayout(context: StrictLayoutContext) {
    return context.type !== LAYOUT_TYPE_INITIALIZATION;
  }
}

This seems to me the best approach. It is hidden from the dev and can't be used (we do not need to expose this class) and it subscribes the objects in initializion as it should onBeforeLayout.

[ShaMan123]

However it doesn't respect overrides if someone wants to override FitContentLayout and use it in fromObject... but that might be solved in serialization

[ShaMan123]

so actually all we need is to pass an option to FitContentLayout in fromObject

export class FitContentLayout {
  performInitializationLayout = true
  shouldPerformLayout(context: StrictLayoutContext) {
    return context.type !== LAYOUT_TYPE_INITIALIZATION || this.performInitializationLayout;
  }
}

And then in fromObject we flag it

[asturur]

i would like the layout manager to no have to deal with fromObject quirks.
What about the idea of passing for init a layout manager that is no-op and then we add the real one?

[asturur]

something like this example commit here:
0d3474c

[ShaMan123]

something like that can work
but bear in mind overriding and customizing possibilities

[asturur]

something like that can work but bear in mind overriding and customizing possibilities

I want to see how everything looks like when group can be properly serialized with the constructor and the layout managers in the serialization process.

Then if someone really need anyhing more than fit-content, fixed size and clip-path layouts will eventually complain or silently find a solution

[asturur]

The change with the class has been done, i don't plan to touch anything else if not requested changes. @ShaMan123

[asturur]

@ShaMan123 do you have time to review?

[ShaMan123]

Not today
Tomorrow

[asturur]

@ShaMan123 any chance we can move forward with this and proceed to finish the layout manager?

[ShaMan123]

Looking good apart from a test I don't understand

[ShaMan123]

do we need this?

[asturur]

yes because is the layoutManager that is doing setCoords for the group initialization.
It doesn't have to, but is doing it, it changes width and height or top and left, so it make sense it also calls setCoords.
If the the layoutManager is a no-op then a manual call to setCoords is necessary

[ShaMan123]

The layout manager did not call setCoords in the intialization layout.

[github-actions]

Coverage after merging remove-group-init-quirks into master will be

82.75%

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	85.16%	89.83%	68%	87.32%	127–128, 130–131, 131, 131, 131, 131, 225, 288–289, 293, 39, 60, 78
constants.ts	100%	100%	100%	100%
src/LayoutManager/LayoutStrategies
ClipPathLayout.ts	81.58%	72.22%	100%	88.24%	29–30, 40, 52, 55–56, 63
FitContentLayout.ts	100%	100%	100%	100%
FixedLayout.ts	11.11%	0%	0%	25%	19–20, 22, 22, 22, 22, 22
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	92.31%	91.89%	90%	93.55%	119, 130, 139, 32, 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%	100, 106, 111, 126, 126, 126, 126, 126, 128–131, 131, 134, 141, 20, 28–32, 32, 32, 32, 32, 32, 32, 32, 53–59, 59, 59, 59, 59, 61, 66–67, 69, 79, 85–87, 87, 89, 92–93, 93, 93, 93, 93, 95
rotate.ts	19.57%	12.50%	50%	21.43%

[asturur]

Ok off to the second part then, restoring fromObject.