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

fix(ActiveSelection): make sure canvas is in charge of setting initial coords #9322

Merged
merged 11 commits into from
Sep 10, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Sep 9, 2023

Motivation

Description

Moved the call to setCoords from the constructor.
It should not be there because the canvas ref is being set after that possibly, leading to wrong coords due to vpt.

Changes

Gist

In Action

@ShaMan123 ShaMan123 closed this Sep 9, 2023
@ShaMan123 ShaMan123 reopened this Sep 9, 2023
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 for review

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

Build Stats

file / KB (diff) bundled minified
fabric 915.345 (+0.245) 305.446 (+0.019)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

Coverage after merging as-init into master will be

82.97%

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%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%118, 129, 138, 31, 94
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.12%77.68%83.05%79.61%1000, 1003–1004, 1004, 1004, 1006, 1014, 1014, 1014–1016, 1016, 1016, 1023–1024, 1032–1033, 1033, 1033–1034, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1517, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 437, 445, 449, 449, 449–450, 452, 535–536, 536, 536–537, 543, 543, 543–545, 565, 567, 567, 567–568, 568, 568, 571, 571, 571–572, 575, 584–585, 587–588, 590, 590–591, 593–594, 606–607, 607, 607–608, 610–615, 621, 628, 665, 665, 665, 667, 669–674, 680, 686, 686, 686–687, 689, 692, 697, 710, 738, 738, 796–797, 797, 797–798, 800, 803–804, 804, 804–805, 807–808, 811, 811–813, 816–817, 887, 899, 906, 927, 959, 980–981, 997–998, 998, 998–999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts94.44%92.28%94.23%96.18%1099, 1101, 1103–1104, 301, 478–479, 481–482, 482, 482, 531–532, 593–594, 647–649, 681, 686–687, 714–715, 899, 899–900, 903, 923, 923, 972, 980
   StaticCanvas.ts96.78%93.09%100%98.53%1030, 1040, 1092–1093, 1096, 1131–1132, 1208, 1217, 1217, 1221, 1221, 1268–1269, 185–186, 202, 570, 582–583, 913–914, 914, 914–915
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts84.31%71.43%91.67%88%17–18, 18, 18–19, 19, 19
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.ts6.06%0%0%11.43%103, 117, 117, 117, 117, 117, 119–122, 122, 125, 132, 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.ts84.2

@ShaMan123 ShaMan123 changed the title patch(ActiveSelection): init logic fix(ActiveSelection): initial coords Sep 9, 2023
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.

this fix is ready
the test is broken on master

@asturur
Copy link
Member

asturur commented Sep 9, 2023

The changes is ok for me, but we should comunicate somewhere that using ActiveSelection manually is becoming highly not suggested.

]),
viewportTransform: [2, 0, 0, 0.5, 400, 150],
});
expect(canvas.getActiveSelection().getCoords()).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

let's test by picking aCoords and lineCoords that is what are we testing today.
We are testing that those are correct, without the need of being recalculated.
getCoords return an array and is mostly for internal usage of calculations, if we snapshot aCoords and lineCoords we also see tl,tr,bl,br.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are testing that those are correct, without the need of being recalculated.

They are not recalcuated.

I just have in mind #8767 so I avoid lineCoords already but whatever

Copy link
Member

@asturur asturur Sep 10, 2023

Choose a reason for hiding this comment

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

they are not recalculated no, but no one knows it, is not written the test is not explcit.
if getCoords change or recalculation becomes default, this test doesn't break but also becomes uneffective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not follow
What should be changed?

Copy link
Member

Choose a reason for hiding this comment

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

oh i thought you changed because i have seen outdated tag on the discussion
i ll change it never mind

@@ -55,4 +56,20 @@ describe('ActiveSelection', () => {
selection.add(new FabricObject({ left: 50, top: 50, strokeWidth: 0 }));
expect(selection.item(0).getCenterPoint()).toEqual({ x: 50, y: 50 });
});

it('sets coords after attaching to canvas', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a canvas test? is the canvas executing the new code no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I have no idea where it should belong

@ShaMan123
Copy link
Contributor Author

The changes is ok for me, but we should comunicate somewhere that using ActiveSelection manually is becoming highly not suggested.

I was thinking to make the constructor protected. Then you get a TS error you can ignore.

Update ActiveSelection.spec.ts
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.

ok

asturur
asturur previously approved these changes Sep 10, 2023
@asturur asturur changed the title fix(ActiveSelection): initial coords fix(ActiveSelection): make sure canvas is in charge of setting initial coords Sep 10, 2023
@asturur asturur merged commit 308544f into master Sep 10, 2023
26 checks passed
@asturur asturur deleted the as-init branch September 10, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants