patch(): Control#shouldActivate

[ShaMan123]

Motivation

I need the control api to be more flexible

Description

Introduce the ability for a control to decide if it should be activated or not.
Before this was handled by the canvas in an hardcoded way with:

      !this.hasControls ||
      !this.canvas ||
      (this.canvas._activeObject as unknown as this) !== this

as an early return and later on on the control check if the control is visible.

Now the first early return doesn't take care of the object being selected, but the control later has a new method shouldActivate that by default return true only if the object is the active one and if the control is visible.

The developer can override this in a custom control and apply the logic that prefers.

Default behaviour does not change

Changes

• Control#shouldActivate - for overriding
• now the control loop and the visibility check run regardless if the object is active or not.

Gist

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	919.695 (+0.326)	303.655 (+0.091)

[ShaMan123]

Is there a need for a test?
I feel like not adding non crucial tests until jest is in

[ShaMan123]

made it simpler by reverting isPointerOverControl

[github-actions]

Coverage after merging patch-find-control into master will be

83.66%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	47.83%	100%	25%	60%	17, 20, 23, 40, 43, 46
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
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.21%	91.89%	90%	93.33%	116, 127, 136, 29, 92
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–98
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
Canvas.ts	78.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.ts	94.39%	91.16%	94.64%	96.62%	1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
StaticCanvas.ts	96.86%	92.91%	100%	98.61%	1102–1103, 1103, 1103–1104, 1224, 1234, 1288–1289, 1292, 1327–1328, 1405, 1414, 1414, 1418, 1418, 1465–1466, 310–311, 328, 759, 771–772
TextEditingManager.ts	100%	100%	100%	100%
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	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
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	5.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.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%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%	27, 31–32, 32, 32, 37
src/filters
BaseFilter.ts	21.62%	23.21%	32%	18.27%	100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157,

[ShaMan123]

One day I will refactor the control visibility awkward code. But not soon.

[ShaMan123]

DONE

[asturur]

looks like that both getImageLines and findCrossPoints are object method that are more utils than anyhitng else.
Take some parameter in, do not used this at all and give a result.

If you want to have the control class being the owner of the decision if the cursor is in the control, you can do that moving those to utils and exposing a method that uses the utils.

[asturur]

What made you change the idea around the pointer thing? something bad happens when doing that?

[ShaMan123]

I am looking at #8767 to guide me.
I don't want to introduce changes here, I prefer to do it there.
I do think the control should be in charge but I am happy with this limited scope and not worrying about merge conflict in #8767

[ShaMan123]

a4c0f62#diff-fe6477e182894fdb5db40e4d3bffa1f12e02af91a158b9619752aca781441332R199-R201

This can easily be exposed on the control

[asturur]

One day I will refactor the control visibility awkward code. But not soon.

That is there just for backward compatibility not because we like it.
Is probably a good idea to deprecate the current functions so that we inform developers those are going to be changed/removed.

[asturur]

You mean we could look at actionName and the relevant lockProperties?

[ShaMan123]

yes

[ShaMan123]

or better off, move the lock property to the control

[asturur]

well lock is by object and controls could be generic.
That is the same issue we have with visibility.

[asturur]

The PR is fine, i m merging.
The description is bad i fixed it.