Fabric.js: canvas.forEachObject Resets object count after calling .remove()

[mtickle]

Version

1.7.1

Test Case

https://jsfiddle.net/mtickle/sn1k0gbL/

Steps to reproduce

1. Add a series of objects to a canvas with unique IDs
2. Use a forEachObject loop to remove objects matching the IDs

Expected Behavior

All of the objects in forEachObject will be removed.

Actual Behavior

Every other object is removed. It seems like the collection in forEachObject resets when an object is removed from the canvas by canvas.remove(). Multiple passes at the function will keep removing objects until they are all gone.

[ericmandel]

I'd like to second the fact that this is a problem, while offering an explanation of the cause as well as a possible solution. The problem is due to the fact that fabric forEachObject is just a loop over an objects array, calling the specified callback function (my code is 1.6.6):
forEachObject: function(callback, context) { var objects = this.getObjects(); for (var i = 0, len = objects.length; i < len; i++) { callback.call(context, objects[i], i, objects); } return this; }
When the callback func is "remove", this results in a splice being taken of the objects array to remove the object:
objects.splice(index, 1);
Splice always changes the array structure, but because the loop increases from 0 to n, the change affects array elements that have not yet been processed: if you splice out the 0th element of the array, splice moves all of the other array elements "to the left", so that what was element 1 becomes element 0, etc. This means the next iteration of the loop acts on index 1, but the original element 1 has now moved to index 0, and does not get processed. Instead index 1 now contains the object that originally was in index 2. That's why you are seeing every other object get removed.

In v1.6.3, the code for this routine looped in the opposite direction:
forEachObject: function(callback, context) { var objects = this.getObjects(), i = objects.length; while (i--) { callback.call(context, objects[i], i, objects); } return this; }
This "reverse" direction of looping does allow one to remove object inside the loop, since the splice now potentially changes the array order of elements that already have been processed, not the ones yet to be processed.

This is a generic problem for Javascript forEach loops and one could argue that we developers should just deal with it. But since going back to the old looping order fixes the problem, and greatly simplifies the code to remove objects, I wonder if there is a reason why we can't simply revert to the "reverse order" code.

[asturur]

there was some reason why we removed the inverse loop.
Oh that was the adding of elements to some other situation and you would add them in reverse order.
So this.getObjects() could return a cloned instance of the array, that would be better maybe.
I guess if this would break something else.
getObjects already return a close if you ask a specific subtype of objects.

[ericmandel]

Please don't change getObjects(), that certainly would break other code ...

The current processing order makes forEachObject() act like the Javascript forEach(): it explicitly executes the callback on array elements in ascending order. So at least this behavior can be documented.

That means one has to specially code an object remove loop:

1. use getObjects() instead of forEachObject() to return the original array, and process the elements in descending order (i.e. use the code from 1.6.3 above)
2. use forEachObject() to first generate a cloned array, and loop through the cloned array,
3. etc.

[mtickle]

Thank you, @ericmandel and @asturur. I suspected that the behavior was similar to forEach() after a few days of messing with this. I'll try @ericmandel's solution of reversing through the loop. I also may have another more unconventional solution of removing the text from the text objects to "hide" them instead of actually trying to remove them. Thanks again for your help. Since this isn't a bug, I'll close the issue.

-Mike

[asturur]

the point is that forEachObject is supposed to do something on each object.
It should not operate on the _objects array directly.
For sure if i touch it i will break something or some user app, the point is that operating on the original array is dangerous as we see in this example.

[ericmandel]

Correct, and thus forEachObject() should support functionality where the underlying implementation is not manipulated by the application programmer, e.g.:
canvas.forEachObject(function(obj){ canvas.remove(obj) });
Here, the application does not care about the underlying implementation -- whether canvas objects are stored in an array or an object -- it just wants to act on each canvas object by removing it. But this breaks, because the underlying implementation uses an array, and processes that array in a certain order -- all of which is unknown to the application programmer. I didn't realize what broke going from 1.6.3 to 1.6.6 until I looked at the implementation and saw that the processing order had changed.