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(): _initRetinaScaling initializaing the scaling regardless of settings in Canvas. #8565

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jan 4, 2023

Motivation

Description

fixes the regression from #8510
I decided to open this PR to have a pure scope for this fix cause I guess that is what you want, instead of fixing as part of #8520

Decide if you want to close this and merge #8520 or merge this and then that. Anyways if you do the second when dealing with conflict #8520 should win.

Changes

now _initRetinaScaling should be called only if isRetinaScaling (doesn't check in the method as previously did)

Gist

In Action

@ShaMan123 ShaMan123 requested a review from asturur January 4, 2023 04:56
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Build Stats

file / KB (diff) bundled minified
fabric 932.050 (+0.011) 300.972 (+0.049)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Coverage after merging fix-retina-init-regression into master will be

83.36%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.js35%12.50%0%54.55%430, 432, 432, 432, 432, 432, 434–435, 437, 437, 437–438
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
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, 23, 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.86%85.42%100%93.58%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, 77, 89–91, 99
src/canvas
   canvas.class.ts92.81%88.60%94.12%95.70%1154, 1154–1155, 1158, 1178, 1178, 1240, 1276–1277, 1296–1297, 1305–1306, 1327, 1335, 1448–1449, 1451–1452, 1472–1473, 1636–1637, 1641, 510, 577–578, 583, 593, 722–723, 725–726, 726, 726, 772–773, 834–835, 888–890, 920, 925–926, 955–956
   canvas_events.ts78.19%75.30%82.81%79.48%1003–1004, 1004, 1004–1006, 1008–1009, 1009, 1009, 1011, 1019, 1019, 1019–1021, 1021, 1021, 1027–1028, 1036–1037, 1037, 1037–1038, 1043, 1045, 1075–1077, 1080–1081, 1085–1086, 1184, 1204–1206, 1209–1210, 1282, 1402, 145, 1496–1497, 1503, 1507–1508, 1524, 1546, 1593, 1598, 1637, 170, 318–319, 319, 319–320, 320, 323–327, 332, 334, 346–348, 351, 368, 368, 368–369, 369, 369–370, 378, 383–384, 384, 384–385, 387, 396, 402–403, 403, 403, 439, 443, 443, 443, 443, 443–444, 446, 521, 523, 523, 523–525, 545, 547, 547, 547–548, 548, 548, 551, 551, 551–552, 555, 564–565, 567–568, 570, 570–571, 573–574, 586–587, 587, 587–588, 590–594, 600, 607, 647, 647, 647, 649, 651–655, 661, 667, 667, 667–668, 670, 673, 678, 691, 718, 774–775, 775, 775–776, 778, 781–782, 782, 782–783, 785–786, 789, 789–791, 794–795, 805, 887, 901, 908, 929, 961, 982–983, 989
   static_canvas.class.ts94.56%88.97%97.92%97.11%1125–1126, 1126, 1126–1127, 1247, 1257, 1311–1312, 1315, 1350–1351, 1429, 1438, 1443, 1492–1493, 1721, 1721–1722, 1772, 1775, 1778, 1778, 1778, 1781, 1784, 1784, 1784, 309, 345, 358, 411–412, 414–415, 424, 428–429, 432–433, 885
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%
   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.ts40.54%40%60%36.36%28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
   base_filter.class.ts20.83%20.83%33.33%18.18%102–104, 104, 104–105, 112–115, 115, 115–116, 122, 122, 122–125, 143, 173–178, 182–183, 183, 183–186, 186, 186,

@asturur
Copy link
Member

asturur commented Jan 5, 2023

How the regression was created do we know? the fix you propose seems unrelated to the changes that happend in that pr.
What is the regression actually?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 5, 2023

If you look at the override of _initRetinaScaling you can see it doesn't check if _isRetinaScaling though it should, as does the super method in static canvas.
https://github.com/fabricjs/fabric.js/pull/8510/files#r1062365124
https://github.com/fabricjs/fabric.js/pull/8510/files#r1062366689

_initRetinaScaling() {
super._initRetinaScaling();
this.__initRetinaScaling(this.upperCanvasEl, this.contextTop);
}

_initRetinaScaling() {
if (!this._isRetinaScaling()) {
return;
}
this.__initRetinaScaling(this.lowerCanvasEl, this.contextContainer);
}

@ShaMan123
Copy link
Contributor Author

the most simple fix should be

this._isRetinaScaling() && this.__initRetinaScaling(this.upperCanvasEl, this.contextTop); 

But it makes more sense to check outside of a method named initRetinaScaling

@asturur
Copy link
Member

asturur commented Jan 5, 2023

ok now i understand.
That message should have be on the description.

Regarding what makes more sense is usually the smaller change visually.
Imagine if you added the check inside the subclass in the canvas file, that would have explained the issue too.
While with those changes the question was, why are we moving a check from inside out and how that fixes anything.

asturur
asturur previously approved these changes Jan 5, 2023
@asturur asturur changed the title fix(): _initRetinaScaling regression fix(): _initRetinaScaling initializaing the scaling regardless of settings in Canvas. Jan 5, 2023
@asturur asturur merged commit 0bc63dc into master Jan 5, 2023
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
@ShaMan123 ShaMan123 deleted the fix-retina-init-regression branch February 5, 2023 05:18
ShaMan123 added a commit that referenced this pull request Mar 16, 2024
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