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

patch(TS): canvas patch init 2 #8520

Merged
merged 40 commits into from
Jan 15, 2023
Merged

patch(TS): canvas patch init 2 #8520

merged 40 commits into from
Jan 15, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Dec 18, 2022

Motivation

Description

Refactor canvas init logic, more work can be done in this scope

Changes

  • canvas init
  • dumped event binding tests since we bind in constructor from now on
  • remove requestRenderAllBound, renderAndResetBound
  • fix _initRetinaScaling regression from chore(TS): Convert Canvas to TS #8510, BREAKING since now should be called only if isRetinaScaling (doesn't check in the method as previously did)

Gist

In Action

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.

READY

src/canvas.class.ts Outdated Show resolved Hide resolved
src/static_canvas.class.ts Outdated Show resolved Hide resolved
@ShaMan123 ShaMan123 changed the base branch from ts-convert-event-mixins to master December 29, 2022 21:36
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.

updated from master and rebased

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Build Stats

file / KB (diff) bundled minified
fabric 935.259 (-4.866) 294.941 (-0.752)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Coverage after merging ts-canvas-patch-init into master will be

83.19%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.ts100%100%100%100%
src
   cache.ts96.97%90%100%100%56
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   env.ts72.73%53.33%100%79.17%22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%118, 124, 135, 144, 96
   point.class.ts100%100%100%100%
   shadow.class.ts98.48%96%100%100%156
   typedefs.ts100%100%100%100%
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   pattern_brush.class.ts97.06%87.50%100%100%21
   pencil_brush.class.ts91.81%85.42%100%93.52%122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
   spray_brush.class.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
   TextEditingManager.ts100%100%100%100%
   canvas.class.ts93.21%89.96%94%95.48%1133, 1133–1134, 1137, 1150–1152, 1157, 1157, 1216, 1266–1267, 1288, 1296, 1409–1410, 1412–1413, 1433–1434, 556–557, 562, 572, 701–702, 704–705, 705, 705, 751–752, 813–814, 867–869, 899, 904–905, 934–935
   canvas_events.ts78.04%75.31%81.82%79.29%1004–1005, 1013–1014, 1014, 1014–1015, 1020, 1022, 1052–1054, 1057–1058, 1062–1063, 1179–1181, 1184–1185, 1258, 1378, 1472–1473, 1479, 1483–1484, 1500, 1522, 1569, 1574, 1613, 1622, 163, 188, 295–296, 296, 296–297, 297, 300–304, 309, 311, 324–327, 330, 349, 349, 349–350, 350, 350–351, 359, 364–365, 365, 365–366, 368, 377, 383–384, 384, 384, 411, 420, 424, 424, 424, 424, 424–425, 427, 502, 504, 504, 504–506, 526, 528, 528, 528–529, 529, 529, 532, 532, 532–533, 536, 545–546, 548–549, 551, 551–552, 554–555, 567–568, 568, 568–569, 571–575, 581, 588, 628, 628, 628, 630, 632–636, 642, 648, 648, 648–649, 651, 654, 659, 672, 699, 755–756, 756, 756–757, 759, 762–763, 763, 763–764, 766–767, 770, 770–772, 775–776, 786, 868, 882, 889, 910, 942, 963–964, 980–981, 981, 981–983, 985–986, 986, 986, 988, 996, 996, 996–998, 998, 998
   static_canvas.class.ts94.80%89.88%97.92%97.01%1089–1090, 1090, 1090–1091, 1211, 1221, 1275–1276, 1279, 1314–1315, 1393, 1402, 1407, 1456–1457, 1685, 1685–1686, 1735, 1738, 1741, 1741, 1741, 1744, 1747, 1747, 1747, 286–287, 384–385, 387–388, 763, 763–764, 849
src/color
   color.class.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   changeWidth.ts100%100%100%100%
   control.class.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts86.67%66.67%100%100%122, 129
   drag.ts100%100%100%100%
   polyControl.ts6.35%0%0%11.43%100, 105, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts20%12.50%50%22.22%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.41%92.68%100%93.59%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%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/filters
   2d_backend.class.ts92%83.33%100%93.75%35–36
   FilterBackend.ts88.89%88.89%100%85.71%15–16
   WebGLProbe.ts37.14%40%60%30%28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51,

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 3, 2023

@asturur this is high priority
I have no idea the full extent of the regression but to fix it I had to init size on upper canvas, something I didn't locate before #8510
I then tried to use setDimensions instead of initSize with no luck.
please review

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.

ready

src/canvas/canvas.class.ts Show resolved Hide resolved
Comment on lines -1265 to -1263
this.cacheCanvasEl.setAttribute('width', `${this.width}`);
this.cacheCanvasEl.setAttribute('height', `${this.height}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

size will be set by setDimensions on init

/**
* @private
*/
private _bindEvents() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to have this now that it is impossible to init canvas twice

Comment on lines 276 to 277
renderAndResetBound: () => void;
requestRenderAllBound: () => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped

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.

mergable

Comment on lines 284 to 285
this.renderAndReset = this.renderAndReset.bind(this);
this.requestRenderAll = this.requestRenderAll.bind(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these were renamed,
see description

Copy link
Member

Choose a reason for hiding this comment

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

Yes yes i have read everything i was in the middle of things and then i had to go to bed.
Probably going to let them both names.

@asturur
Copy link
Member

asturur commented Jan 15, 2023

Bind on constructor is a great normal thing we should have done as soon as we started moving canvas, i really didn't thin of it.
I don't want to have requestRenderAll and renderAndReset naturally bound for those reasons:

  • are not meant to be callbacks or event handlers
  • an unexperienced dev would see them working and not understand those need to be bound
  • you could override them or swap them on the prototype and then assign back to the instance and loose the binding
    So i delete the convenience bound functions ( with the bound suffix ) but i m not binding this.
    If there is a complain that the old one were useful we can add them back

The reorganization is great, i don't like the idea of adding a parameter to setDimensions to have it skip rendering just because we need it for initialization, since we managed to remove eventsBound: boolean, let's not add a similar one.
I separated setDimenions in setDimensions + protected setDimensionsImplementation, and teh constructor can call the latter.

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 absolutely agree

@ShaMan123
Copy link
Contributor Author

@asturur why e1ffe0c?

@asturur
Copy link
Member

asturur commented Jan 15, 2023

it is how it was before and i wouldn't mind if it doesn't save half kb on the minified build.

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.

None yet

2 participants