PROPOSAL: removal of stateful checks

[asturur]

FabricJS has an internal pattern to fire object:modied with stateful property, that will check if the object changed color or position inspecting the properties. Similarly has an statefullCache that will detect the cache changes in case you don't use .set(prop, value).

In my opinion is developer job to use the set method to trigger cache invalidation while the object:modified with property changes does not make sense at all because is an event that would fire on the same code changes you write.

I proposing to remove this logic completely.

This requires a bit of rework of how text detect when a new dimension change is necessary, but i think is just an automatic behaviour that on the long run brings more de-optimization than other.

With this would go away all the usages of
setupState, hasStateChanged, stateful and statefullCache.

References:
https://github.com/fabricjs/fabric.js/blob/master/src/mixins/stateful.mixin.js

[melchiar]

So just to clarify, you're proposing just removing the state change logic, not the entire object:modified event correct?

[asturur]

object:modified would stay there to react to user interactions.
Any interaction initialized from controls ( or drag ) that return a true after the action is executed.
While if you change the color to red to blue, and then render, there wouldn't be any event.

[melchiar]

Yes, that sounds good. We've generally discouraged changing object properties directly anyways that require a render. The state tracking logic is to me unnecessary and I would not miss it.

[ShaMan123]

This is an important conversation because it is part of a larger discussion IMO.
Is fabric imperative or declarative? We should decide. And this must be the first thing the dev reads about fabric. Because it is very unclear and then expectations vary and confusion arises.
Sounds like you're aiming towards imperative. Which makes sense if the main goal of fabric is customization and flexibility. State makes things very clear and easy but comes with cost, konva is a good example for that.
I think that the js rewrite with proper setter/getter and classes will make it simple for the dev to override methods and react to state changes on their own. And it will make assignment of values to be done only through setters possible, which is fantastic in terms of state management.
Renaming object:modified might be a good idea, because it is confusing in terms of what's being watched and brings wrong expectations.

[asturur]

I'm not sure we need to take a full decision between imperative or declarative.
I want fabric to do less things but better, and keeping track of what is changed, when the only person in charges of changes is the developer, seems too much.
FabricJS should take ownership of alerting the developer of things done by the final user for which the developer is not writing any code, that would be default embedded interactions.

I prefer imperative for sure make things simpler.

Has today hasStateChanged is used for:
cache invalidation
text measurement invalidation

not much else

[ShaMan123]

This has now become a stumbling block to my efforts.
The saveProps method clones the clip path entirely - meaning we can't use circular refs on objects. This limitation must go. And is poorly designed. I need it for future fixes and rewriting to TS will surely use circular refs - delegates instead of mixins.
If we decide to keep state checks why not use a timestamp of last changed/rendered instead of all the cloning?
Then when we save state we only record the timestamp for later comparison

[ShaMan123]

#7901

[asturur]

stateful checks aren't really needed imho.
We should have a list of reason why we keep it, rather than how can we swap it with another functionality.
Stateful checks were here before i arrived, and the clone logic to keep checks working is slow and bothersome.
cloning style is expensive, some properties needs to be skipped, circular references yes were an issue also in the past

[ShaMan123]

I didn't look into it deeply to understand how fabric uses it.
Let's remove it then.

[asturur]

that timestamp idea is good but also a change count could be good enough. Let me try to open a pr for that

…

On Mon, 25 Apr 2022 at 06:16, Shachar ***@***.***> wrote: I didn't look into it deeply to understand how fabric uses it. Let's remove it then. — Reply to this email directly, view it on GitHub <#7656 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJDQQBQW73VVYMPB2M4373VGYMCHANCNFSM5NVH7MBA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ShaMan123]

a counter is definitely good as well and is like stateID = ctx.save(). The dev can use that for redo/undo ctx.restore(stateID)

[ShaMan123]

Checkout src/mixins/stateful.mixin.js on #7901
should definitely be ported to master as part of the discussed change if we decide not to remove

[asturur]

We can port the changes to fill-layout ( that i have no idea what it is ) as soon as we merge fill-layout.
I ll check it before anyway in case it adds something that is impossible without stateful.

[ShaMan123]

It doesn't need stateful. I had to fix stateful to get it working with circular refs