refactor(controls): deprecate 🚫 SHARED 🚫 controls #7914

ShaMan123 · 2022-04-30T09:39:04Z

This PR refactors Object#controls from a shared object attached to the prototype into a unique object attached to an instance.

Motivation

#7596 (comment)
I hate the shared controls and the shared concept altogether.
It's extremely unintuitive and many issues were raised because of this. I have spoken to fabric devs and they all mentioned this.
IMO sharing instances should never be done in fabric (and maybe even at all - except for mega perf enhancements) and anyways rewriting JS to use class will and MUST deprecate this bad practice in favor of static properties/methods or simply bound properties/methods.

Gist

created ObjectControls, TextboxControls classes that hold the default controls.
controls must have an attach method - best practice is to subclass ObjectControls | TextboxControls. When migrating to TS we could/should use an abstract class to define this class that the dev will have to subclass.
BREAKING control API since fabricObject is no longer needed as it is referenced in the control.object
I have no idea why this test fails.

Migration

If a dev needs to change controls across many objects they can subclass or they can pass a controls object in initialization. But it cannot be SHARED

Post PR

We should consider moving control logic from src/mixins/object_interactivity.mixin.js to ObjectControls

support circular refs!

I don't know why there's a bug but I am really fed up with all this old js

ShaMan123

1 failing test - why does it fail?
1 comment

Besides this is good to go.

ShaMan123 · 2022-04-30T09:51:50Z

src/shapes/object.class.js

@@ -1002,6 +1004,9 @@
      else if (key === 'shadow' && value && !(value instanceof fabric.Shadow)) {
        value = new fabric.Shadow(value);
      }
+      else if (key === 'controls') {
+        value.attach(this);


not sure we need this because I couldn't get controls to be set in setOptions without breaking all tests

asturur · 2022-04-30T22:45:21Z

I don't know custom controls works, this a change in name of best practices of which i see very little utility.
We have a TON of breaking changes, if we are not fixing anything i don't want to change this.

ShaMan123 · 2022-05-01T08:17:15Z

I don't know custom controls works, this a change in name of best practices of which i see very little utility.

Didn't understand

We have a TON of breaking changes, if we are not fixing anything i don't want to change this.

Absolutely. And the TS rewrite will introduce even more.
That's why I think we should release v6 with this in it because it already requires a major adjustment. Plus, once we migrate this will happen naturally. It will require a very awkward workaround to continue sharing objects between instances when written with proper classes.

asturur · 2022-05-01T11:21:52Z

src/mixins/default_controls.js

-    actionHandler: scalingXOrSkewingY,
-    getActionName: scaleOrSkewActionName,
-  });
+  class ObjectControls {


since we are not migrating yet, this should still be the function + prototype class.

asturur · 2022-05-01T11:25:19Z

To be clear i see this is more polished.
I don't think the current control api is broken or terrible.
It allows you to plug in your functions.
Even if we want to move to class system, there is no need to use attach and to move render function in.
Let's mitigate the impact, we can still get rid of the shared object ( that would be mitigated anyway when you go and read how it works ) but we don't need to break the api.
I have a bunch of custom controls and they just works.

Also i have questions, if i have a control group class for each object, as soon as i want to add a single control to all objects, like a delete button, how do i do for the instantiated objects? I have to loop and reassign controls.

What is good of the current pattern is that you may have multiple control set ready to be used by object, for example:

locked object controls
cropping picture controls
normal controls
And just swap them when needed.
In this case you would have to write a class, and instantiate it each time.

I don't know custom controls works, this a change in name of best practices of which i see very little utility.

With this i meant, i don't like to fix what is not broken, in the name of this is a better practice.
Was late and i wrote hastly.

ShaMan123 · 2022-05-01T14:24:04Z

I don't think the control API is horrible at all.
I simply dislike the shared object practice. Someone new to fabric will bash their head against the wall until they understand that's the origin of what they are experiencing as did I. Numerous issues have been raised and the devs I speak to dislike it as well (dislike is a soft term).
I do think it makes sense to iterate over objects to set controls over them. Entirely on the dev.
And if I look from above it adds a logical layer to fabric that IMO should not exist.

Anyways you indulge me with the things I want/need/think are important so now it's my turn.

stale · 2022-06-12T12:56:10Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2022-06-28T00:17:09Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2022-07-30T19:18:24Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2022-09-21T02:40:40Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ShaMan123 added 13 commits April 21, 2022 07:50

welcome class

71dd7d5

refactor controls to class

66afcf0

appropiate binding

3072dc3

Update object_interactivity.js

7d60a2e

Update canvas_events.js

6cdb5dc

Update object_clipPath.js

a956b79

fix(Stateful): port from #7901

a2a342b

support circular refs!

Merge branch 'master' into refactor-controls

62cd813

remove keys prop

c5169f6

Update object_interactivity.js

61b1045

Update canvas_events.js

a5ebd3a

expose attach + cleanup

8ef5373

fix(): don't use set

c01f3df

I don't know why there's a bug but I am really fed up with all this old js

ShaMan123 added the v6 label Apr 30, 2022

ShaMan123 added 2 commits April 30, 2022 12:46

lint

e6f4f9b

JSDOC

15e585b

ShaMan123 commented Apr 30, 2022

View reviewed changes

ShaMan123 mentioned this pull request Apr 30, 2022

IMPORTANT: Migrating to modern javascript/typescript #7596

Closed

ShaMan123 changed the title ~~refactor(controls): deprecated SHARED controls~~ refactor(controls): deprecate 🚫**SHARED**🚫 controls Apr 30, 2022

ShaMan123 changed the title ~~refactor(controls): deprecate 🚫**SHARED**🚫 controls~~ refactor(controls): deprecate 🚫 **SHARED** 🚫 controls Apr 30, 2022

ShaMan123 changed the title ~~refactor(controls): deprecate 🚫 **SHARED** 🚫 controls~~ refactor(controls): deprecate 🚫 SHARED 🚫 controls Apr 30, 2022

asturur reviewed May 1, 2022

View reviewed changes

ShaMan123 mentioned this pull request May 30, 2022

Problem with setting offsetY attribute for the mtr control #7978

Closed

stale bot added the stale Issue marked as stale by the stale bot label Jun 12, 2022

ShaMan123 removed the stale Issue marked as stale by the stale bot label Jun 13, 2022

stale bot added the stale Issue marked as stale by the stale bot label Jun 28, 2022

ShaMan123 removed the stale Issue marked as stale by the stale bot label Jun 28, 2022

stale bot added the stale Issue marked as stale by the stale bot label Jul 30, 2022

ShaMan123 removed the stale Issue marked as stale by the stale bot label Aug 1, 2022

stale bot added the stale Issue marked as stale by the stale bot label Sep 21, 2022

ShaMan123 removed the stale Issue marked as stale by the stale bot label Sep 21, 2022

ShaMan123 mentioned this pull request Nov 13, 2022

Create functions to generate standard control set for whoever wants new control set and new controls per object #8415

Merged

asturur closed this in #8415 Nov 27, 2022

ShaMan123 deleted the refactor-controls branch February 6, 2023 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(controls): deprecate 🚫 SHARED 🚫 controls #7914

refactor(controls): deprecate 🚫 SHARED 🚫 controls #7914

ShaMan123 commented Apr 30, 2022 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Apr 30, 2022

asturur commented Apr 30, 2022

ShaMan123 commented May 1, 2022 •

edited

Loading

asturur May 1, 2022

asturur commented May 1, 2022

ShaMan123 commented May 1, 2022

stale bot commented Jun 12, 2022

stale bot commented Jun 28, 2022

stale bot commented Jul 30, 2022

stale bot commented Sep 21, 2022

refactor(controls): deprecate 🚫 SHARED 🚫 controls #7914

refactor(controls): deprecate 🚫 SHARED 🚫 controls #7914

Conversation

ShaMan123 commented Apr 30, 2022 • edited Loading

Motivation

Gist

Migration

Post PR

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Apr 30, 2022

Choose a reason for hiding this comment

asturur commented Apr 30, 2022

ShaMan123 commented May 1, 2022 • edited Loading

asturur May 1, 2022

Choose a reason for hiding this comment

asturur commented May 1, 2022

ShaMan123 commented May 1, 2022

stale bot commented Jun 12, 2022

stale bot commented Jun 28, 2022

stale bot commented Jul 30, 2022

stale bot commented Sep 21, 2022

ShaMan123 commented Apr 30, 2022 •

edited

Loading

ShaMan123 commented May 1, 2022 •

edited

Loading