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() BREAKING Remove calculate option from geometry methods #9483

Merged
merged 35 commits into from
Nov 12, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Nov 5, 2023

Motivation

All the geometry methods had a calculate optional parameter that was used to get the values without running setCoords.
setCoords is one of the learning step of fabricJS.
When you change a geometry property of an object, you have to call setCoords for the basic standard coordinates to be cached correctly.

FabricJS does that for every user interaction point, but for the code written by developer that is on the developer.
calculate: boolean defaulted to false and created more harm than good.

You could either:

  • call setCoords, update the object, and then call obj.isOnScreen
    OR
  • call obj.isOnScreen(true) and get the same result but with wrong cached coords

this wasn't true for all the parts of fabricJS that work on the object coordinates, target check for example is one.
This led to situation where you could use calculate true, but then have to call setCoords anyway to get things working correctly.

At this point is easier to learn that if you change object position or size with your code, you have also to call setCoords for things to just works.

Affected methods

@asturur asturur marked this pull request as draft November 5, 2023 11:08
@asturur
Copy link
Member Author

asturur commented Nov 5, 2023

Needs rebase when the oher one is merged.

Copy link
Contributor

github-actions bot commented Nov 5, 2023

Build Stats

file / KB (diff) bundled minified
fabric 904.345 (-4.908) 304.025 (-0.559)

@asturur asturur marked this pull request as ready for review November 6, 2023 11:44
@asturur
Copy link
Member Author

asturur commented Nov 6, 2023

this is reviewable now

@asturur asturur requested a review from ShaMan123 November 8, 2023 10:04
ShaMan123
ShaMan123 previously approved these changes Nov 9, 2023
Copy link
Contributor

@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 would like to rename aCoords, oCoords to something like cornerCoords and controlCoords

src/shapes/Object/ObjectGeometry.ts Outdated Show resolved Hide resolved
@ShaMan123
Copy link
Contributor

I also want to remove _getCoords and maybe calcACoords or at least rename it

ShaMan123
ShaMan123 previously approved these changes Nov 9, 2023
Copy link
Contributor

@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.

looks good

Update ObjectGeometry.ts
Copy link
Contributor

github-actions bot commented Nov 9, 2023

Coverage after merging remove-calculate into master will be

82.75%

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/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, 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, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.02%76.99%83.05%79.88%1003–1004, 1004, 1004–1005, 1011, 1013, 1041–1043, 1046–1047, 1051–1052, 1175–1177, 1180–1181, 1254, 1371, 1493, 155, 180, 287, 287–292, 297, 320–321, 326–331, 351, 351, 351–352, 352, 352–353, 361, 366–367, 367, 367–368, 370, 379, 385–386, 386, 386, 39, 429, 43, 437, 441, 441, 441–442, 444, 526–527, 527, 527–528, 534, 534, 534–536, 556, 558, 558, 558–559, 559, 559, 562, 562, 562–563, 566, 575–576, 578–579, 581, 581–582, 584–585, 597–598, 598, 598–599, 601–606, 612, 619, 656, 656, 656, 658, 660–665, 671, 677, 677, 677–678, 680, 683, 688, 701, 729, 781–782, 782, 782, 782, 782, 782, 785–786, 789, 789–791, 794–795, 877, 884, 884, 884, 897, 930, 951–952, 968–969, 969, 969–971, 974–975, 975, 975, 977, 985, 985, 985–987, 987, 987, 994–995
   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.ts89.58%83.33%91.67%91.67%16–17, 17, 17
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.ts7.25%0%0%13.51%103, 108, 120, 120, 120, 120, 120, 122–125, 125, 128, 135, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
   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.ts

@asturur asturur merged commit 3adbb0f into master Nov 12, 2023
22 checks passed
@asturur asturur deleted the remove-calculate branch November 12, 2023 20:19
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