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(Object): dirty unflagging inconsistency #8910

Merged
merged 3 commits into from May 11, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented May 11, 2023

Motivation

The Object is technically dirty until a new drawing is completed.
So it make sense to change the flag status AFTER and not before the render operation.
This because drawObject could be heavily overridden with custom logic that require the true status of the dirty flag.
Before this change the value would be always false if called in a caching sequence and that is incorrect.

Description

Object should unflag dirty after drawObject, see ref:

this.drawObject(this._cacheContext, options.forClipping);
this.dirty = false;

Changes

Consistently mark object as not dirty after calling drawObject

Gist

In Action

@ShaMan123 ShaMan123 changed the title fix(Object): flagging dirty inconsistency fix(Object): dirty flagging cleanup inconsistency May 11, 2023
@github-actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 902.416 (0) 304.561 (0)

@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023

Coverage after merging fix/flagging-dirty-incosistency into master will be

83.67%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
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%
   typedefs.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%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–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
   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%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
   Canvas.ts78.87%77.54%81.67%79.41%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, 1589, 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.39%91.16%94.55%96.63%1113, 1113–1114, 1117, 1137, 1137, 1195, 1248–1249, 1270, 1278, 1403, 1405, 1407–1408, 512, 692–693, 695–696, 696, 696, 745–746, 807–808, 861–863, 895, 900–901, 928–929
   StaticCanvas.ts96.85%92.86%100%98.61%1114–1115, 1115, 1115–1116, 1236, 1246, 1300–1301, 1304, 1339–1340, 1417, 1426, 1426, 1430, 1430, 1477–1478, 322–323, 340, 771, 783–784
   TextEditingManager.ts100%100%100%100%
src/color
   Color.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
   Control.ts93.90%88.89%90.91%97.73%235, 322, 322, 357
   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%
   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%130–131, 162–163, 170, 176, 178
   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
   index.ts100%100%100%100%
   node.ts73.08%33.33%66.67%88.24%27, 31–32, 32, 32, 43
src/filters
   BaseFilter.ts21.93%22.81%32%19.05%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182, 182, 182–184,

@ShaMan123 ShaMan123 changed the title fix(Object): dirty flagging cleanup inconsistency fix(Object): dirty unflagging inconsistency May 11, 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

@asturur
Copy link
Member

asturur commented May 11, 2023

Can you explain the issue in the PR please?
PRs need to be a bit self explanatory.
Work issue is clear between me and you, the average reader that could read that change has no clue.
Consistency is clear, but what issue is actually causing isn't.

@asturur asturur merged commit 2d37f17 into master May 11, 2023
18 checks passed
@asturur asturur deleted the fix/flagging-dirty-incosistency branch May 11, 2023 10:52
@ShaMan123
Copy link
Contributor Author

Code is heavily overriding the render cycle. There is a state check involving the dirty flag.
Since it wasn't consistent, non cache rendering was unflagging after drawObject while cache rendering was unflagging before it made the code cumbersome.
Regardless I am looking into the impl but as a first step this seems to make sense in the eyes of a maintainer.
Did I manage to explain?

@asturur
Copy link
Member

asturur commented May 11, 2023

yes i just want to have a PR description that is different from i m having some issue and this solves it for me

@ShaMan123
Copy link
Contributor Author

Yeah, I get that. For sure.

@asturur asturur mentioned this pull request May 19, 2023
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