fix(): fire Poly control events #9504

ShaMan123 · 2023-11-20T08:55:16Z

Motivation

#9503
PolyControl do not have a wrapWithFireEvent

Description

Fire an event when modifiying a poly via a poly control.

TODO:

I have found while testing that wrapWithFireEvent will fire a stale transform object on the first call because transform.actionPerformed is not yet set on transform but is true. It is set after wrapWithFireEvent is called. That needs fixing.

Changes

add wrapWithFireEvent
adapt poly control test

Gist

In Action

Update index.spec.ts

github-actions · 2023-11-20T08:56:44Z

Build Stats

file / KB (diff)	bundled	minified
fabric	904.571 (+0.063)	304.002 (+0.013)

lint

ShaMan123

ready

ShaMan123 · 2023-11-20T08:58:36Z

e2e/utils/ObjectUtil.ts

+      [this.objectId]
+    );
+  }
+
  async executeInBrowser<C, R>(


I want to rename this to evaluate

ShaMan123 · 2023-11-20T08:59:06Z

src/controls/polyControl.ts

@@ -10,6 +10,9 @@ import type {
  TransformActionHandler,
 } from '../EventTypeDefs';
 import { getLocalPoint } from './util';
+import { wrapWithFireEvent } from './wrapWithFireEvent';
+
+const ACTION_NAME = 'modifyPoly';


is this a good name?

ShaMan123 · 2023-11-20T08:59:51Z

e2e/tests/controls/polygon-controls/index.spec.ts

viewing this commit by commit might be easier to review

asturur · 2023-11-20T11:28:23Z

src/controls/polyControl.ts

@@ -126,7 +132,7 @@ export function createPolyControls(
    idx++
  ) {
    controls[`p${idx}`] = new Control({
-      actionName: 'modifyPoly',
+      actionName: ACTION_NAME,


Did we call it modifyPoly?
This is a terrible name.
Shouldn't be this just resizing? or modifying? it doesn't matter for this pr of course.

I agree. BAD name. resizing... not percise. modifying too general.

i have to write an issue to cover the fact that actionName of wrapWithFireEvent will not be in sync with control.actionName.
Not sure if is worth fixing or just documenting
control.actionName is used for end transform event, while this one is used for in transform event.
The current situation must be at least explained somewhere

I think it ought to be the same value. Why else?

e2e/tests/controls/polygon-controls/index.spec.ts

This reverts commit 04046f9.

github-actions · 2023-11-20T14:22:13Z

Coverage after merging poly-control-events into master will be

82.75%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.85%	94.83%	87.10%	97.14%	109, 112, 215–216, 241–242
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	119, 130, 139, 32, 95
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	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.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	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.ts	79%	76.99%	83.05%	79.84%	1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	93.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.ts	96.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.ts	100%	100%	100%	100%
TextEditingManager.ts	89.58%	83.33%	91.67%	91.67%	16–17, 17, 17
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	97.50%	88.89%	100%	100%	33
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	94.44%	93.10%	91.67%	96.77%	183, 249, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	8.57%	0%	0%	15.79%	100, 106, 111, 126, 126, 126, 126, 126, 128–131, 131, 134, 141, 20, 28–32, 32, 32, 32, 32, 32, 32, 32, 53–59, 59, 59, 59, 59, 61, 66–67, 69, 79, 85–87, 87, 89, 92–93, 93, 93, 93, 93, 95
rotate.ts	19.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.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	<

ShaMan123 added 2 commits November 20, 2023 09:42

cleanup test

57efe4c

Update index.spec.ts

modifyPoly event

a897c07

test

04046f9

lint

ShaMan123 force-pushed the poly-control-events branch from 8af3045 to 04046f9 Compare November 20, 2023 09:02

Update CHANGELOG.md

af72dcb

ShaMan123 commented Nov 20, 2023

View reviewed changes

ShaMan123 requested a review from asturur November 20, 2023 09:04

asturur reviewed Nov 20, 2023

View reviewed changes

e2e/tests/controls/polygon-controls/index.spec.ts Show resolved Hide resolved

ShaMan123 and others added 3 commits November 20, 2023 14:51

Revert "test" change

d970fb9

This reverts commit 04046f9.

Create polyControl.spec.ts

8e63dc1

eslint disable no-restricted-globals

1ed30b2

asturur approved these changes Nov 20, 2023

View reviewed changes

asturur merged commit ab7a0a4 into master Nov 20, 2023
22 checks passed

asturur deleted the poly-control-events branch November 20, 2023 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(): fire Poly control events #9504

fix(): fire Poly control events #9504

ShaMan123 commented Nov 20, 2023

github-actions bot commented Nov 20, 2023

ShaMan123 left a comment

ShaMan123 Nov 20, 2023

ShaMan123 Nov 20, 2023

ShaMan123 Nov 20, 2023

asturur Nov 20, 2023

ShaMan123 Nov 20, 2023

asturur Nov 20, 2023

ShaMan123 Nov 20, 2023

github-actions bot commented Nov 20, 2023

fix(): fire Poly control events #9504

fix(): fire Poly control events #9504

Conversation

ShaMan123 commented Nov 20, 2023

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Nov 20, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Nov 20, 2023

Choose a reason for hiding this comment

ShaMan123 Nov 20, 2023

Choose a reason for hiding this comment

ShaMan123 Nov 20, 2023

Choose a reason for hiding this comment

asturur Nov 20, 2023

Choose a reason for hiding this comment

ShaMan123 Nov 20, 2023

Choose a reason for hiding this comment

asturur Nov 20, 2023

Choose a reason for hiding this comment

ShaMan123 Nov 20, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2023