refactor(): `getActiveControl` return value #9515

ShaMan123 · 2023-11-28T04:10:56Z

Motivation

getActiveControl is a convenient method we exposed in #9102 but not enough.
Using it made me realize the need to expose both the control and the coord of the control with it.
For us it is easy to get that but for a dev it means they need to understand the cood system.
Also it is not convenient trying to access the control/coord if the control key is undefined

Description

Changes

BREAKING beta refactor(): getActiveControl(): string => getActiveControl(): { key: string; control: Control; coord: TOCoord; } | undefined
Made Transform#action optional so it is easier to manipulate transform. I want to do the same with corner but this is a big diff so i will in a dedicated PR.
squash _beforeTransform into _setupCurrentTransform
rm unused Transform#reset

Gist

In Action

github-actions · 2023-11-28T04:18:41Z

Build Stats

file / KB (diff)	bundled	minified
fabric	907.451 (-0.158)	304.718 (-0.050)

ShaMan123

Made Transform#action optional so it is easier to manipulate transform. I want to do the same with corner but this is a big diff so i will in a dedicated PR.
squash _beforeTransform into _setupCurrentTransform
_setupCurrentTransform return value void => transform
rm unused Transform#reset

fix test undef

backward compat

x

ShaMan123 · 2023-11-28T07:29:35Z

src/canvas/SelectableCanvas.spec.ts

migrated test from test/unit/canvas_events.js

ShaMan123 · 2023-11-28T07:30:57Z

@asturur I wish this to enter v6 so we don't break getActiveControl

ShaMan123 · 2023-11-28T07:51:54Z

Also I have found something I should PR to fix
If alreadySelected is false the control is not set in current transform but its mouseDownhandler is called

 if (target) { 
   const alreadySelected = target === this._activeObject; 
+   const corner = target._findTargetCorner( 
+     this.getViewportPoint(e), 
+     isTouchEvent(e) 
+   ); 
   if (target.selectable && target.activeOn === 'down') { 
     this.setActiveObject(target, e); 
   } 
-   const corner = target._findTargetCorner( 
-     this.getViewportPoint(e), 
-     isTouchEvent(e) 
-   );

fabric.js/src/canvas/Canvas.ts

Lines 1086 to 1110 in 3dee2e3

    
           if (target) { 
        
             const alreadySelected = target === this._activeObject; 
        
             if (target.selectable && target.activeOn === 'down') { 
        
               this.setActiveObject(target, e); 
        
             } 
        
             const corner = target._findTargetCorner( 
        
               this.getViewportPoint(e), 
        
               isTouchEvent(e) 
        
             ); 
        
             if (target === this._activeObject && (corner || !grouped)) { 
        
               this._setupCurrentTransform(e, target, alreadySelected); 
        
               const control = target.controls[corner], 
        
                 pointer = this.getScenePoint(e), 
        
                 mouseDownHandler = 
        
                   control && control.getMouseDownHandler(e, target, control); 
        
               mouseDownHandler && 
        
                 mouseDownHandler.call( 
        
                   control, 
        
                   e, 
        
                   this._currentTransform!, 
        
                   pointer.x, 
        
                   pointer.y 
        
                 ); 
        
             } 
        
           }

ShaMan123 · 2024-01-06T18:17:25Z

@asturur I want this in before we publish
Should I split this up into chunks?

ShaMan123

I just reviewed again
This is a focused PR IMO

asturur · 2024-01-11T12:28:23Z

next in line for possibly merging

asturur · 2024-01-11T12:30:16Z

src/canvas/Canvas.ts

          mouseDownHandler.call(
            control,
            e,
            this._currentTransform!,
            pointer.x,
            pointer.y
          );
-        }


those changes here are applied by the minifier later on, but imho they take away on readibility. In the default safe lint or prettier profile are not considered, so i won't hold anything on it since may fall in the opinion realm

asturur · 2024-01-11T12:45:55Z

src/controls/util.ts

@@ -25,7 +25,7 @@ export const NOT_ALLOWED_CURSOR = 'not-allowed';
 */
 export const getActionFromCorner = (
  alreadySelected: boolean,
-  corner: string,
+  corner: string | undefined,


why not using the '?'

its usually better to init all props even if undefined, rather than having some missing and some not
that way browser engine can do smarter optimizations when objects have similar shapes

I don't remember
I agree with '?'

Ahh because it can't be optional because it is not the last arg

oh i thought it was an object.

asturur · 2024-01-13T12:23:11Z

had a rough end of week, back on this now.

github-actions · 2024-01-13T14:07:54Z

Coverage after merging refactor-get-active-control into master will be

82.80%

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	74.14%	65%	66.67%	82.76%	131, 139, 139, 139, 139, 139–141, 152–154, 30
constants.ts	100%	100%	100%	100%
src/LayoutManager
LayoutManager.ts	87.58%	89.47%	76.92%	90%	135–136, 138–139, 139, 139, 139, 139, 233, 291–292, 303, 46
constants.ts	100%	100%	100%	100%
src/LayoutManager/LayoutStrategies
ClipPathLayout.ts	82.05%	72.22%	100%	88.89%	30–31, 42, 54, 57–58, 65
FitContentLayout.ts	100%	100%	100%	100%
FixedLayout.ts	20%	0%	0%	40%	20–21, 23, 23, 23, 23, 23
LayoutStrategy.ts	86%	64.71%	100%	96.15%	36, 43, 43, 43, 51, 71–72
utils.ts	91.67%	85.71%	100%	100%	28, 34
src/Pattern
Pattern.ts	90.54%	93.94%	80%	90.32%	119, 130, 139, 32, 36
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, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	78.91%	76.99%	82.76%	79.72%	1005–1006, 1006, 1006–1007, 1013, 1015, 1043–1045, 1048–1049, 1053–1054, 1165–1167, 1170–1171, 1244, 1356, 1477, 158, 183, 289, 289–294, 299, 322–323, 328–333, 353, 353, 353–354, 354, 354–355, 363, 368–369, 369, 369–370, 372, 381, 387–388, 388, 388, 42, 431, 439, 443, 443, 443–444, 446, 46, 528–529, 529, 529–530, 536, 536, 536–538, 558, 560, 560, 560–561, 561, 561, 564, 564, 564–565, 568, 577–578, 580–581, 583, 583–584, 586–587, 599–600, 600, 600–601, 603–608, 614, 621, 658, 658, 658, 660, 662–667, 673, 679, 679, 679–680, 682, 685, 690, 703, 731, 783–784, 784, 784, 784, 784, 784, 787–788, 791, 791–793, 796–797, 879, 886, 886, 886, 899, 932, 953–954, 970–971, 971, 971–973, 976–977, 977, 977, 979, 987, 987, 987–989, 989, 989, 996–997
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	93.44%	91.80%	94.34%	94.57%	1004, 1012, 1123, 1125, 1127–1128, 1198–1199, 460–461, 463–464, 464, 464, 513–514, 591, 596, 666, 671–672, 699–700, 760–761, 766–770, 772, 931, 931–932, 935, 955, 955
StaticCanvas.ts	96.53%	93.09%	98.92%	98.29%	1019, 1029, 1080–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	86%	71.43%	91.67%	91.67%	17, 17, 17–18, 18, 18
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	10.87%	0%	0%	16.13%	114, 114, 114, 114, 114, 116–119, 119, 122, 129, 24–26, 50–52, 58–59, 61, 71, 77–79, 79, 81, 84, 86, 90–92, 94, 99
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,

ShaMan123 force-pushed the refactor-get-active-control branch from f1c6671 to 70f11a5 Compare November 28, 2023 04:17

ShaMan123 requested a review from asturur November 28, 2023 04:18

ShaMan123 force-pushed the refactor-get-active-control branch 2 times, most recently from a21cc94 to 9594e0b Compare November 28, 2023 05:03

ShaMan123 commented Nov 28, 2023

View reviewed changes

ShaMan123 and others added 5 commits November 28, 2023 07:27

refactor(): getActiveControl return value

44ce093

fix test undef

optional origin

4c2b3d5

update CHANGELOG.md

30fd773

rm Transform#reset

8778ca7

squash _beforeTransform => _setupCurrentTransform

763daa6

backward compat

ShaMan123 force-pushed the refactor-get-active-control branch from 9594e0b to 763daa6 Compare November 28, 2023 05:27

ShaMan123 added 3 commits November 28, 2023 08:15

revert _setupCurrentTransform return value

aa8dbac

migrate test

82f86c6

x

cleanup

3dee2e3

ShaMan123 force-pushed the refactor-get-active-control branch from bff373f to 3dee2e3 Compare November 28, 2023 07:10

ShaMan123 commented Nov 28, 2023

View reviewed changes

src/canvas/SelectableCanvas.spec.ts Outdated

Copy link

Contributor Author

ShaMan123 Nov 28, 2023

Choose a reason for hiding this comment

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

migrated test from test/unit/canvas_events.js

ShaMan123 commented Jan 6, 2024

View reviewed changes

Merge branch 'master' into refactor-get-active-control

ae4b13a

asturur reviewed Jan 11, 2024

View reviewed changes

just saving bytes because i like that

5191ec5

asturur approved these changes Jan 13, 2024

View reviewed changes

asturur merged commit b53aefe into master Jan 13, 2024
22 checks passed

asturur deleted the refactor-get-active-control branch January 13, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(): `getActiveControl` return value #9515

refactor(): `getActiveControl` return value #9515

ShaMan123 commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Nov 28, 2023

ShaMan123 commented Nov 28, 2023

ShaMan123 commented Nov 28, 2023 •

edited

Loading

ShaMan123 commented Jan 6, 2024

ShaMan123 left a comment

asturur commented Jan 11, 2024

asturur Jan 11, 2024

asturur Jan 11, 2024

Smrtnyk Jan 11, 2024

ShaMan123 Jan 11, 2024

ShaMan123 Jan 11, 2024

asturur Jan 11, 2024

asturur commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

refactor(): getActiveControl return value #9515

refactor(): getActiveControl return value #9515

Conversation

ShaMan123 commented Nov 28, 2023 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Nov 28, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Nov 28, 2023

Choose a reason for hiding this comment

ShaMan123 commented Nov 28, 2023

ShaMan123 commented Nov 28, 2023 • edited Loading

ShaMan123 commented Jan 6, 2024

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Jan 11, 2024

asturur Jan 11, 2024

Choose a reason for hiding this comment

asturur Jan 11, 2024

Choose a reason for hiding this comment

Smrtnyk Jan 11, 2024

Choose a reason for hiding this comment

ShaMan123 Jan 11, 2024

Choose a reason for hiding this comment

ShaMan123 Jan 11, 2024

Choose a reason for hiding this comment

asturur Jan 11, 2024

Choose a reason for hiding this comment

asturur commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

refactor(): `getActiveControl` return value #9515

refactor(): `getActiveControl` return value #9515

ShaMan123 commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

ShaMan123 commented Nov 28, 2023 •

edited

Loading