Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(): Layout Manager #9152

Merged
merged 138 commits into from
Nov 30, 2023
Merged

refactor(): Layout Manager #9152

merged 138 commits into from
Nov 30, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Aug 14, 2023

Motivation

#9148

Description

Extracted logic to LayoutManager that accepts a LayoutStrategy (which is what the dev should override in order to create custom layout mechanisms e.g. css).

Exposed on Group:

  • triggerLayout method
  • layout:before, layout:after events.

Changes

  • added relative correction
  • added object_modifying triggers that run on moving, scaling etc. + moved text:changed to here as it should be
  • supports measuring selected objects (needs more work in Redo coords #8767 )
  • transform is set on group before initialization layout

BREAKING beta:

  • removed layout prop in favor of passing an instance of LayoutStrategy
  • rename _applyLayoutStrategy => performLayout
  • rename layout event => layout:after
  • removed layout_change trigger in favor of the accurate context.strategyChange
  • rename getLayoutStrategyResult => LayoutStrategy#calcLayoutResult
  • rename prepareBoundingBox => calcBoundingBox and squash getObjectsBoundingBox into it
  • rename prepareInitialBoundingBox => calcInitialBoundingBox (this method needs revisiting) removed
  • rename + break signature of _adjustObjectPosition => layoutObject
  • rename onLayout => onAfterLayout
  • moved changed to object_modifying trigger
  • initialization trigger context has changed - options are not passed. x, y are passed thanks to fixing this trigger and initial transform.

TODO:

  • fix that strange initialization logic with rotation, skewing
  • move triggers to manager for _watchObject
  • support adding relative objects adding relative objects has 2 different options, adding before layout and adding after (which means the group needs to perform a different layout altogether because it must know the new center before the objects are added) - so I have decided to drop support for this. Adding relative objects before layout, which is the more sane use case can be achieved by group.add(sendObjectToPlane(relativeObject, group.calcTransformMatrix()))
  • support triggers for objects under active selection
  • dispose

Gist

In Action

https://codesandbox.io/p/sandbox/fabric-vanillajs-sandbox-forked-qh3256

@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023

Build Stats

file / KB (diff) bundled minified
fabric 906.413 (+1.563) 304.718 (+0.951)

Comment on lines +296 to +297
'object:layout:before': LayoutBeforeEvent & { target: Group };
'object:layout:after': LayoutAfterEvent & { target: Group };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

object or collection?

@jiayihu
Copy link
Contributor

jiayihu commented Nov 21, 2023

@asturur @ShaMan123 Sorry to put pressure, is there anything left blocking for this PR? We're releasing in production in two weeks but the current Layout implementation is bugged so we'd like to be able to move to Layout Manager and test it.

The two most severe bugs we have are:

  1. Strokeuniform + strokewidth result in slightly wrong positioning for grouped objects
  2. Despite having a fixed layout, triggerLayout updates the Group bounding box

All issues that should be solved by the Layout Manager

@asturur
Copy link
Member

asturur commented Nov 22, 2023

I have some changes i have proposed in a branch and then there would be some followup to change some code for handlign the fromObject case.

I m not sure this PR is solving 1) tho.
We don't user case with code we can run over.

In theory we should be working just on this PR and i should insta-close any other PR, but then that never happen and this get pushed back

@asturur
Copy link
Member

asturur commented Nov 24, 2023

This are some needed changes from me from the Object classes perspective: #9510

@jiayihu
Copy link
Contributor

jiayihu commented Nov 27, 2023

I m not sure this PR is solving 1) tho.

Probably not, but @ShaMan123 said he's waiting for the Layout Manager to fix the issue

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Nov 27, 2023

_getTransformedDimensions is to blame for strokeUniform and bbox
The error is easy to see, the storke uniform vector is relative to the parent instead of the canvas.
Now that geometry is clean I can approach a partial fix. Note that all control action handlers use this ***** method and moving them away is an effort

@ShaMan123
Copy link
Contributor Author

The bright side is that I think that will end all strokeUniform bugs all across fabric making it a stable feature

@asturur
Copy link
Member

asturur commented Nov 30, 2023

i don't want to make other changes to transforms and geometry without talking about it before.
zero
I m sure i already said that but i m repeating to be sure i make my position clear.

I want to also defend _getTransformedDimensions.
The fact that a method does stuff in a way, we introduce without checking more functions on top of it, we can't blame the method that was there before, is conceptually wrong.

strokeUniform is a dumb feature that developer rely on because eventually is visually appealing for some geometry.

strokeUniform is just plain useless because you could just change the strokeWidth OR you could change object properties outside scale and skew and get the exact same visual effect.
Changing a boolean is easier of course, but that boolean brings all the complexity of all use cases inside the library.
So when we decide to support strokeUniform we are supporting 2 ways to do the same thing in the codebase.

@asturur
Copy link
Member

asturur commented Nov 30, 2023

ok i patched the fromObject function to get this merged, fromObject and toObject is what i want to follow up on.
If is all green i m merging

Copy link
Contributor

Coverage after merging layout-mananger into master will be

82.76%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/LayoutManager
   LayoutManager.ts85.80%90.91%68%87.32%127–128, 130–131, 131, 131, 131, 131, 231, 294–295, 299, 39, 60, 78
   constants.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts81.58%72.22%100%88.24%29–30, 40, 52, 55–56, 63
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts11.11%0%0%25%19–20, 22, 22, 22, 22, 22
   LayoutStrategy.ts86.54%64.71%100%96.30%36, 43, 43, 43, 51, 78–79
   utils.ts89.47%80%100%100%28, 34
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%119, 130, 139, 32, 95
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%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.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%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.ts79%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.ts100%100%100%100%
   SelectableCanvas.ts93.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.ts96.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.ts100%100%100%100%
   TextEditingManager.ts86%71.43%91.67%91.67%17, 17, 17–18, 18, 18
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts8.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.ts19.57%12.50%50%21.43%41,

@asturur asturur merged commit 7c44efb into master Nov 30, 2023
22 checks passed
@asturur asturur deleted the layout-mananger branch November 30, 2023 11:06
ShaMan123 added a commit that referenced this pull request Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants