Skip to content

Remove stateful concept ( without passing for deprecation first ). #7920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Remove stateful concept ( without passing for deprecation first ). #7920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d58ce81
remove stateful usage
asturur May 1, 2022
5302edf
remove remove
asturur May 1, 2022
d481e41
forgot those
asturur May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ var filesToInclude = [
'src/mixins/object_ancestry.mixin.js',
'src/mixins/object_stacking.mixin.js',
'src/mixins/object.svg_export.js',
'src/mixins/stateful.mixin.js',

ifSpecifiedInclude('interaction', 'src/mixins/object_interactivity.mixin.js'),

Expand Down
2 changes: 1 addition & 1 deletion src/canvas.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* @tutorial {@link http://fabricjs.com/fabric-intro-part-1#canvas}
* @see {@link fabric.Canvas#initialize} for constructor definition
*
* @fires object:modified at the end of a transform or any change when statefull is true
* @fires object:modified at the end of a transform that perfomed an action
* @fires object:rotating while an object is being rotated from the control
* @fires object:scaling while an object is being scaled by controls
* @fires object:moving while an object is being dragged
Expand Down
3 changes: 1 addition & 2 deletions src/mixins/canvas_events.mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@

target.setCoords();

if (transform.actionPerformed || (this.stateful && target.hasStateChanged())) {
if (transform.actionPerformed) {
this._fire('modified', options);
}
},
Expand Down Expand Up @@ -749,7 +749,6 @@
*/
_beforeTransform: function(e) {
var t = this._currentTransform;
this.stateful && t.target.saveState();
this.fire('before:transform', {
e: e,
transform: t,
Expand Down
107 changes: 0 additions & 107 deletions src/mixins/stateful.mixin.js
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions src/shapes/group.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,29 +417,6 @@
this._drawClipPath(ctx, this.clipPath);
},

/**
* Check if cache is dirty
*/
isCacheDirty: function(skipCanvas) {
if (this.callSuper('isCacheDirty', skipCanvas)) {
return true;
}
if (!this.statefullCache) {
return false;
}
for (var i = 0; i < this._objects.length; i++) {
if (this._objects[i].isCacheDirty(true)) {
if (this._cacheCanvas) {
// if this group has not a cache canvas there is nothing to clean
var x = this.cacheWidth / this.zoomX, y = this.cacheHeight / this.zoomY;
this._cacheContext.clearRect(-x / 2, -y / 2, x, y);
}
return true;
}
}
return false;
},
Copy link
Member Author

@asturur asturur May 1, 2022

Choose a reason for hiding this comment

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

Why this goes all away?
https://github.com/fabricjs/fabric.js/pull/7920/files#diff-216444c1bc3837d560c8241c4d9643b6de8c147095081c5b38715b9e058c24cbL427
if is not statefulCache, return false, stop there.
The rest of the code was executed only if the group had statefullCache activated. Since it is no more possible to activate it, all of this function becomes just a callSuper, and so it can be removed.

Copy link
Contributor

@ShaMan123 ShaMan123 May 1, 2022

Choose a reason for hiding this comment

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

I don't understand and I don't even know what statefulCache is about

Copy link
Member Author

@asturur asturur May 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

statefulCache determine that there is a cache invalidation comparing all the properties of the object with a copy of those when the cache was created. It is slow bothersome since you are looking for a fast iteration over objects and render a cached copy instead you have to navigate the object structure and verify if something changed.

Copy link
Contributor

@ShaMan123 ShaMan123 May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I need to answer this question.
Group invalidation is done in Object#_set, this piece of code is an artifact, and a damaging one too.

Copy link
Member Author

@asturur asturur May 3, 2022

Choose a reason for hiding this comment

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

it wasn't a question :D it was more what i was trying to answer to with the comment.
I would expect a reader does not get automatically why a large chunk of code is deleted

Copy link
Member Author

@asturur asturur May 3, 2022

Choose a reason for hiding this comment

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

I'm not sure I need to answer this question. Group invalidation is done in Object#_set, this piece of code is an artifact, and a damaging one too.

Group invalidation works if you use set. if you didn't use it, this piece of code was looking for you, with an extensive comparing of the children objects. From today you are forced to use set

Copy link
Contributor

@ShaMan123 ShaMan123 May 3, 2022

Choose a reason for hiding this comment

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

OK now I get it
Question:

Why this goes all away?

Answer:

https://github.com/fabricjs/fabric.js/pull/7920/files#diff-216444c1bc3837d560c8241c4d9643b6de8c147095081c5b38715b9e058c24cbL427


/**
* @override
* @return {Boolean}
Expand Down
4 changes: 2 additions & 2 deletions src/shapes/image.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@

/**
* List of properties to consider when checking if cache needs refresh
* Those properties are checked by statefullCache ON ( or lazy mode if we want ) or from single
* calls to Object.set(key, value). If the key is in this list, the object is marked as dirty
* Those properties are checked by calls to Object.set(key, value).
* If the key is in this list, the object is marked as dirty
* and refreshed at the next render
* @type Array
*/
Expand Down
26 changes: 5 additions & 21 deletions src/shapes/object.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,18 +510,6 @@
*/
objectCaching: objectCaching,

/**
* When `true`, object properties are checked for cache invalidation. In some particular
* situation you may want this to be disabled ( spray brush, very big, groups)
* or if your application does not allow you to modify properties for groups child you want
* to disable it for groups.
* default to false
* since 1.7.0
* @type Boolean
* @default false
*/
statefullCache: false,

/**
* When `true`, cache does not get updated during scaling. The picture will get blocky if scaled
* too much and will be redrawn with correct details at the end of scaling.
Expand Down Expand Up @@ -586,6 +574,8 @@
* List of properties to consider when checking if state
* of an object is changed (fabric.Object#hasStateChanged)
* as well as for history (undo/redo) purposes
* This list identify a list of properties that change how the object looks alike.
* Every App has its own needs, so change this list as you may see fit
* @type Array
*/
stateProperties: (
Expand All @@ -597,8 +587,8 @@

/**
* List of properties to consider when checking if cache needs refresh
* Those properties are checked by statefullCache ON ( or lazy mode if we want ) or from single
* calls to Object.set(key, value). If the key is in this list, the object is marked as dirty
* Those properties are checked by calls to Object.set(key, value).
* If the key is in this list, the object is marked as dirty
* and refreshed at the next render
* @type Array
*/
Expand Down Expand Up @@ -1072,9 +1062,6 @@
this._removeCacheCanvas();
this.dirty = false;
this.drawObject(ctx);
if (this.objectCaching && this.statefullCache) {
this.saveState({ propertySet: 'cacheProperties' });
}
}
ctx.restore();
},
Expand All @@ -1085,7 +1072,6 @@
this._createCacheCanvas();
}
if (this.isCacheDirty()) {
this.statefullCache && this.saveState({ propertySet: 'cacheProperties' });
this.drawObject(this._cacheContext, options.forClipping);
this.dirty = false;
}
Expand Down Expand Up @@ -1262,9 +1248,7 @@
}
else {
if (this.dirty ||
(this.clipPath && this.clipPath.absolutePositioned) ||
(this.statefullCache && this.hasStateChanged('cacheProperties'))
) {
(this.clipPath && this.clipPath.absolutePositioned)) {
if (this._cacheCanvas && this._cacheContext && !skipCanvas) {
var width = this.cacheWidth / this.zoomX;
var height = this.cacheHeight / this.zoomY;
Expand Down
1 change: 0 additions & 1 deletion src/shapes/text.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,6 @@
// once text is measured we need to make space fatter to make justified text.
this.enlargeSpaces();
}
this.saveState({ propertySet: '_dimensionAffectingProps' });
},

/**
Expand Down
1 change: 0 additions & 1 deletion src/shapes/textbox.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@
}
// clear cache and re-calculate height
this.height = this.calcTextHeight();
this.saveState({ propertySet: '_dimensionAffectingProps' });
},

/**
Expand Down
8 changes: 0 additions & 8 deletions src/static_canvas.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,6 @@
*/
includeDefaultValues: true,

/**
* Indicates whether objects' state should be saved
* @type Boolean
* @default
*/
stateful: false,

/**
* Indicates whether {@link fabric.Collection.add}, {@link fabric.Collection.insertAt} and {@link fabric.Collection.remove},
* {@link fabric.StaticCanvas.moveTo}, {@link fabric.StaticCanvas.clear} and many more, should also re-render canvas.
Expand Down Expand Up @@ -606,7 +599,6 @@
* @param {fabric.Object} obj Object that was added
*/
_onObjectAdded: function(obj) {
this.stateful && obj.setupState();
if (obj.canvas && obj.canvas !== this) {
/* _DEV_MODE_START_ */
console.warn('fabric.Canvas: trying to add an object that belongs to a different canvas.\n' +
Expand Down