Skip to content
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.

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

Deep clone transformMatrix, strokeDashArray and styles properties #2407

Merged
merged 3 commits into from
Aug 13, 2015
Merged

Deep clone transformMatrix, strokeDashArray and styles properties #2407

merged 3 commits into from
Aug 13, 2015

Conversation

asturur
Copy link
Member

@asturur asturur commented Aug 11, 2015

This will be hardly noticed by anyone.
But still is a bug.

closes #2396.

@asturur
Copy link
Member Author

asturur commented Aug 11, 2015

Added clone for the iText style.
@kangax what do you think?

@asturur asturur changed the title Deep clone object properties Deep clone transformMatrix, strokeDashArray and styles properties Aug 11, 2015
@asturur
Copy link
Member Author

asturur commented Aug 11, 2015

Added failing tests for the modified properties.
ready to merge for me.

@@ -1121,8 +1121,16 @@
* @return {Object} object representation of an instance
*/
toObject: function(propertiesToInclude) {
var newStyle = { }, i, j, row;
Copy link
Member

Choose a reason for hiding this comment

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

newStyleclonedStyles

@asturur
Copy link
Member Author

asturur commented Aug 11, 2015

if it happens with styles it will happen with array also.

@kangax
Copy link
Member

kangax commented Aug 11, 2015

But styles has objects, array has primitives; can't modify primitives' references

@asturur
Copy link
Member Author

asturur commented Aug 12, 2015

I do not understand what you mean, but the bug is there.
Go to kitchensink, execute this code:

// clear canvas
canvas.clear();

// remove currently selected object
canvas.remove(canvas.getActiveObject());

var transform = [2,0,0,2,4,4];
var strokedash = [3, 3];
// add red rectangle
var rect = new fabric.Rect({
  width: 50,
  height: 50,
  left: 50,
  top: 50,
  fill: 'rgb(255,0,0)',
  transformMatrix: transform,
  strokeDashArray: strokedash,
  stroke: 'black',
  strokeWidth: 2
});

// add green, half-transparent circle
canvas.add(rect);
canvas.add(rect.clone().setFill('blue'));
canvas.add(rect.clone().setFill('green'));

obtain this
image

then run this code

canvas.getActiveObject().transformMatrix[0] = 1;
canvas.getActiveObject().strokeDashArray[1] = 8;
canvas.renderAll();

and you get this: ( all object get modified )
image

@kangax
Copy link
Member

kangax commented Aug 13, 2015

@asturur not sure I understand the bug here... in your last example, you're changing values of transformMatrix and seeing result rendered, no? Isn't this expected behavior?

@asturur
Copy link
Member Author

asturur commented Aug 13, 2015

i m changing the transformatrix of one rect, and all three are changing.
because in transformMatrix we have reference of the cloned object instead of a new array.

@kangax
Copy link
Member

kangax commented Aug 13, 2015

Oh, I didn't realize it's cloned. Of course, you're right.

@kangax
Copy link
Member

kangax commented Aug 13, 2015

One last thing — we don't want to use extend for arrays since it does for..in iteration and that kind of iteration doesn't just iterate over indices (0..n) but also anything in Array.prototype, etc. Just do transformMatrix.concat() to create a clone of array.

@asturur
Copy link
Member Author

asturur commented Aug 13, 2015

@kangax done.

kangax added a commit that referenced this pull request Aug 13, 2015
Deep clone transformMatrix, strokeDashArray and styles properties
@kangax kangax merged commit db34378 into fabricjs:master Aug 13, 2015
@kangax
Copy link
Member

kangax commented Aug 13, 2015

+changelog

@asturur asturur deleted the deep-clo branch August 30, 2015 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IText Clone object, pass reference to style object instead of cloning it.
2 participants