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(): Canvas dom handling delegation to utility class #8930

Merged
merged 24 commits into from
May 29, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented May 18, 2023

Motivation

Mine/Our discomfort of what is going on with prototype chains

Description

This is a POC here for discussion, show casing the concept of flattening the prototype chain of canvas into delegates/services/managers (call them how'd you like).
Each of them should have a well defined scope.
The final purpose being not having a prototype chain at all. Not for canvas nor for objects. Or at least minimizing significantly. So that subclassing, customizing etc. is more straight forward.
It should also make the code more easy to understand because of scoping.
I chose to begin with canvas deliberately to prove it can be done and prove it has value.
Took a lot more time than I thought.
Part of the concept is to make these changes only internal and non breaking. Then when the time is right swap everything, break and bump a version.

Changes

No changes to exposed interfaces, only to internals - NOT BREAKING

  • extracted all elements logic to ElementsManager
  • extracted target find logic to FindTarget and exposed renderTarget
  • initializing retina scaling now happens only on backstore size change (was firing always before) - Is that correct?
  • removed redundant size initializing since it happens when canvas calls setDimensions
  • Naming is not final

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2023

Build Stats

file / KB (diff) bundled minified
fabric 920.051 (-0.009) 302.319 (+0.402)

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2023

Coverage after merging delegation-poc into master will be

83.70%

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.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   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/Pattern
   Pattern.ts92.21%91.89%90%93.33%116, 127, 136, 29, 92
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 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.01%82.35%100%93.75%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, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.05%77.54%83.05%79.57%1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
   SelectableCanvas.ts94.42%92.24%94.23%96.15%1121, 1121–1122, 1125, 1145, 1145, 1194, 1202, 1327, 1329, 1331–1332, 525, 700–701, 703–704, 704, 704, 753–754, 815–816, 869–871, 903, 908–909, 936–937
   StaticCanvas.ts96.73%92.88%100%98.52%1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1262–1263, 1339, 1348, 1348, 1352, 1352, 1399–1400, 309–310, 326, 694, 706–707
   TextEditingManager.ts82.69%55.56%91.67%100%15, 15, 15, 15, 15, 15, 15, 15
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts100%100%100%100%
   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.ts93.33%87.88%91.67%97.78%175, 240, 327, 327, 362
   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.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.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–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Suggest order of review:

  1. src/canvas/ElementsManager/StaticCanvasElements.ts
  2. src/canvas/ElementsManager/util.ts
  3. src/canvas/StaticCanvas.ts
  4. src/canvas/ElementsManager/CanvasElements.ts
  5. src/canvas/SelectableCanvas.ts
  6. the rest

Copy link
Contributor Author

@ShaMan123 ShaMan123 May 18, 2023

Choose a reason for hiding this comment

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

This file might belong under the util dir

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I am happy with this.
I believe it is ready for reviewing.

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

done

@ShaMan123 ShaMan123 requested a review from asturur May 18, 2023 04:39
@ShaMan123
Copy link
Contributor Author

Build Stats

file / KB (diff) bundled minified
fabric 917.475 (+2.257) 304.881 (+0.805)

image

@ShaMan123
Copy link
Contributor Author

master
image

@asturur
Copy link
Member

asturur commented May 18, 2023

This is small enough to be discussed.
I can't read it today or tomorrow and write my own description of the changes to see if i understand correctly
Removing the long prototype chain is indeed a concern especially for objects, Canvas is probably a good place to experiment with since is a smaller one.

@asturur
Copy link
Member

asturur commented May 18, 2023

I didn't read the code deeply, but apart naming changes i think the targetFind part is rushed since it could fit in other kinds of changes, like the selection code and is not worth doing now.
Better one topic at time imho.
The DOM one is small enough to be tested, but we can't do more of this kind of changes now, i really want to go to a release.

The layout logic of group is exactly something i wanted to refactor in this style.

@ShaMan123
Copy link
Contributor Author

great
yeah I thought to leave out target find. It will be easy to revert:

ShaMan123 added 2 commits May 20, 2023 12:45
This reverts commit 0360770.
Revert "expose renderTarget"
e5871f821
This reverts commit bca2355.
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

  • reverted target find 7a069c0
  • renamed to DOMManager
  • exported
  • added dep warning on Canvas#containerClass

@asturur
Copy link
Member

asturur commented May 22, 2023

I need to figure out a couple of things then i can merge.

  1. why this code can't be minified as before, ok 2 new getters and some chained property access ( this.that.those.these ) but 576 byte extra just for code shuffling seems strange to me

  2. some util that is used exclusively for canvas dom manipulation can still be moved

I ll look at it tomorrow or later today

@asturur asturur changed the title POC(): Canvas Delegation refactor(): Canvas dom handling delegation to utility class May 29, 2023
@asturur
Copy link
Member

asturur commented May 29, 2023

I moved all the utilities that are used only by the dom manager in the dom manager folder.
Removed makeELemenentSelectable/unselectable from exports

@asturur asturur merged commit d295c94 into master May 29, 2023
@asturur asturur deleted the delegation-poc branch May 29, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants