#6936: call remove on every object on canvas.clear

[Basa0]

Suggested fix for #6936: calls canvas.remove on every canvas object, instead of only clearing the canvas._objects array (this would result in firing additional removal events and removing the canvas property of the objects previously added to the canvas).

I understand we should be careful if the initial implementation may have skipped this removal on purpose. In terms of performance, I don't think removal itself is particularly costly in any way. Calling canvas.remove will fire 'object:removed' on canvas and 'removed' on each object for every object, the listeners may be costly or unwanted in some way.

If this is a cause of concern, either because it may be a performance issue or because it may be considered a breaking change: we could maybe implement an option parameter to canvas.remove, maybe called "suppressObjectRemoveEvents", that would default to true - so that this new behavior would be opt-in (at least temporarily).

[asturur]

thanks for opening the pr and sorry for the delay.
I think the fix is right, i m just not sure if we should fire the remove event here or just manually remove the canvas reference from the objects.
I'm usually worried from uncontrolled events firing.
There is also the risk that calling remove will try to fire a render for each object ( that eventually is then cancelled by the next one called, but still extra work ).

[Basa0]

Thanks @asturur, no worries on any delays - look how late I am 😅

I made a change to call remove only once with all the object (instead of once per object), to avoid calling requestRenderAll after each of those.

As to the overall issue, I'm unsure how to approach it. I believe it is the correct behavior: any handler a dev would add to removal events should theoretically be valid and even welcomed - objects are actually being removed. But because this was not the behavior until now, it may lead to unwanted results in some implementations. I'm thinking particularly from a performance perspective, that handlers of such events may be expensive in some way.

I would suggest that if there isn't a way to make an educated decision on whether this is safe to introduce, consider this as a breaking change for a future major release (should there ever be another one).

[asturur]

i'm keen to risk on this change.
It seems more logical to me to have clear do a bunch of stuff rather than developers knows to hunt references of canvases and activeObjects around.