Fix group initialization

[sapics]

#2095 @chrisrickard reported a problem of group.toObject. This PR would fix it.

There are two fixed points.

1. If toObject copied delegatedProperties, it affects to grouped objects at initialization (https://github.com/kangax/fabric.js/blob/master/src/shapes/group.class.js#L72).
   I removed delegatedProperties at toObject function, but it is also possible to fix by changing group initialization code. I felt that fix at toObject will be better for consistency.

2. At toObject, _objects copied normally. However, at initialization, _objects coordinates are updated (https://github.com/kangax/fabric.js/blob/master/src/shapes/group.class.js#L70).
It makes change position of grouped objects.
To fix it, I use _restoreObjectsState, before copying _objects.

closes #2119
closes #2095

[kangax]

I'm OK with this PR but I'd love to have 2 things before merging:

1. comment on top of delegatedProperties removal to explain why we're doing it
2. unit test(s) that fail without this fix and pass with it (to avoid regressions in the past and to show that this really fixes it)

[sapics]

@kangax Thank you for your reply!
I add a comment and fix unit test for group.toObject.

[sapics]

Sorry, in #2095, @chrisrickard reported a bug of this PR.
I would like to check and fix.

[sapics]

I fixed the bug of my previous commit.

This PR fixed a part of the #2095 problem which is caused by group.toObject and group.initialize.
I guess that the other problem #2095 might come from Path objeect.

[sapics]

1. At previous fix, I have misunderstood about group._calcBounds and group._updateObjectsCoords.
   Therefore, some code restore to original code, and, the text at top of this PR is strike-through.
2. I add a unit test, which confirm the need to remove delegatedProperties.

[sapics]

I detect the issue #926 which produce this bug, and it is merged #1868 in Dec, 2014.
#926 suggested that from extend(this, options) to this.callSuper('initialize', options).
It was merged after 1.4.13 released, and it makes me think about consistency.

The proposed code in this PR is consistent with after #1868 about a initialization.
However a json data which is produced before #1868, this PR and after #1868 are not consistent.

For better consistency for parsing a json data which is produced before #1868, we need to fix function group.initialize,

2nd proposition:

this.callSuper('initialize', options);

to

var delegatedOptions = {},
    undelegatedOptions = {},
    i;
for ( i in options ) {
  if ( this.delegatedProperties[i] ) {
    delegatedOptions[i] = options[i];
  } else {
    undelegatedOptions[i] = options[i];
  }
}
extend(this, delegatedOptions);
this.callSuper('initialize', undelegatedOptions);

,or, 3rd proposition:

this._objects = objects || [];
...
this.callSuper('initialize', options);

to

var delegatedOptions = {},
    undelegatedOptions = {},
    i;
for ( i in options ) {
  if ( this.delegatedProperties[i] ) {
    delegatedOptions[i] = options[i];
  } else {
    undelegatedOptions[i] = options[i];
  }
}
this.callSuper('initialize', delegatedOptions);
this._objects = objects || [];
...
this.callSuper('initialize', undelegatedOptions);

These (2nd or 3rd) fix would be more stronger than this PR for changing delegatedProperties at later version.
However, 2nd or 3rd fix is not consistent with after #1868 about a initialization.
It is also possible to combine, 1st+2nd or 1st+3rd. (1st is this PR code which fix group.toObject)

I vacillate between them, because it is a problem about consistency with a previous version.
Just my opinion, 2nd or 3rd fix looks better, because #1868 is merged after 1.4.13 release.

I'm sorry for long post.

[sapics]

I am sorry for changing very often.

I choose the 3rd proposition in previous comment.
This is good for consistency with json produced by 1.4.13, furthermore, I think that this is the best for reproducibility of group which is requested in #926.

[asturur]

@sapics can i ask you why you remove

- options = options || { };

in favor of

+ if (options) {

I imagine and undestand than creating options = { } is 'heavier' than checking if it exist, and of course if not needed from the code this object is useless.

i think it was made like that to avoid checking and more code nesting and indent level.
But i may be wrong.

Did you change for a matter of efficency or something else?

[asturur]

i add, before merging, we should consider this:

does this make sense:

var g = new fabric.Group([rect, circle, triangle, path], {fill: 'red'});

Do we expect the group to have objects filled with red or not?
and later in our code...
g.set('fill', 'red');
All objects will be red.

Do we expect the group to have objects filled with red or not?

json loading back is still a problem that we have to take care of.
But maybe all this logic should be in the fromObject or enlinvenobjects more than in initialize?

I m really asking, not pretending i know the answer.

[sapics]

@asturur Thank you for asking!
I wish to discuss about it, too!
I think that it is problem about consistency between versions.

V1.4.13 and before: not fill red to objects.
after v1.5.0: fill red to objects.

After v1.5.0 situation is done by request of #926, I feel that it is not expected, because #926 talks only about shadow or gradient etc...
Therefore, I believe that not fill red to objects is right.

[asturur]

but in normal code after initialization, a red fill will fill all. and this will stay, right?

Do you think is possible to handle this where the problem came out, in the load from json of group?

would an hack, like disabling temporarly the delegated properties when loading back from json or from object, be enough? or too bad?

i think that if delegated properties exist we should use them also in initialize.

i m sharing thoughts.

[sapics]

Yes, g.set('fill', 'red'); is fill red.

Sorry, I was thinking only about consistency between versions.
I understand your opinion "i think that if delegated properties exist we should use them also in initialize."
I am agree with your opinion is the good for consistency about how to use fabric.Group.

Therefore, you proposed that remove delegatedProperties at fromObject (and toObject?) as the solution, right?

[asturur]

yes but before make you waste time, i will look to see if it is possible and where and propose here.

[sapics]

Thanks a lot! If we fix only fromObject, just additional 3 lines would be enough.

  fabric.Group.fromObject = function(object, callback) {
    fabric.util.enlivenObjects(object.objects, function(enlivenedObjects) {
      delete object.objects;
+     for (var prop in fabric.Group.prototype.delegatedProperties) {
+       delete object[prop];
+     }
      callback && callback(new fabric.Group(enlivenedObjects, object));
    });
  };

[asturur]

Ok finally on computer, i can explain myself better.
we cannot delete delegated properties, for example opacity is needed to stay in group, and should not leak to objects contained. Because we render group opacity or one day we could decide to render background color or fill for group and for sure no one will remember of this caveat.
(opacity should not be delegated at all, but this is another issue).

Something like this, could solve both:

• Leaking of propagated properties in objects when reloading from object or json
• consistent behaviour between initialization and use the delegated properties normally

  fabric.Group.fromObject = function(object, callback) {
    fabric.util.enlivenObjects(object.objects, function(enlivenedObjects) {
      delete object.objects;
-      callback && callback(new fabric.Group(enlivenedObjects, object));
+      callback && callback(new fabric.Group(enlivenedObjects, object, true));  //name to be decided
    });
  };

     initialize: function(objects, options, skipPropagation) {
      options = options || { };

+      skipPropagation && this.callSuper('initialize', options);
      this._objects = objects || [];
      for (i = this._objects.length; i--; ) {
         this._objects[i].group = this;
      }

       this.originalState = { };

      if (options.originX) {
        this.originX = options.originX;
      }

      if (options.originY) {
        this.originY = options.originY;
      }

       this._calcBounds();
       this._updateObjectsCoords();

-      this.callSuper('initialize', options);
+      !skipPropagation && this.callSuper('initialize', options);

[sapics]

Awesome! Thank you very much!

When I use unit test, there are errors.
I think that the property which is not delegated set after this._updateObjectsCoords(); looks better, is it acceptable?

[asturur]

Yeah! because of _calcBounds will overwrite the dimension we just parsed.
God bless tests and who runs them.

top, left, are overwritten, width and height are recalculated same.

If we are restoring from object, and so if skipPropagation is true ( and so the name sucks, it should be something like restoringFromObject ) we should not do _calcBounds at all.

if(!skipPropagation){
  this._calcBounds();
  this._updateObjectsCoords();
  this.callSuper('initialize', options);
}

this may be too much skipping... and create other errors.
Other idea in _calcBounds we should optionally set Top and Left JUST if they are not present in the options.

- this.set(this._getBounds(aX, aY, onlyWidthHeight));
+ var dimensions = this._getBounds(aX, aY, onlyWidthHeight)
+ this.set('width', dimensions.width);
+ this.set('height', dimensions.height);
+ if (/*check for left not defined in this*/) -> this.set('left', dimensions.left);
+ if (/*check for top not defined in this*/) -> this.set('top', dimensions.top);

[sapics]

Your first suggestion looks very cool!!
There are no effect to original group and objects, it must be replicate original state!
Additionally, I guess that toObject need to save __origHasControls for _objects, because now situation would make hasControls to false forever after loadFromJSON.

Now, I am in home. I will do coding tommorow!

[sapics]

I add commit with your first idea,

    if(!skipPropagation){
      this._calcBounds();
      this._updateObjectsCoords();
      this.callSuper('initialize', options);
    }

Thank you very much! It pass all of unit test!
It looks work well https://jsfiddle.net/sapics/mq00xq3y/121/

I am torn between we need to setCoords to objects or not, like

    if(!skipPropagation) {
      this._calcBounds();
      this._updateObjectsCoords();
      this.callSuper('initialize', options);
    }
    else {
      var len = this._objects.length;
      for (i = 0; i < len; ++i) {
        this._objects[i].setCoords();
      }
    }

[asturur]

i think we need to call _updateObjectsCoords always, i would not avoid it
we need some time to analize it.

Is nearly done.

https://github.com/kangax/fabric.js/pull/2101/files#diff-b290a59d2e985ee793daf97398d5688dR105
here we should check for presence (type of undefined?) of '__originalHasControls' or we overwrite what we saved with the other pr.

(we need to squeeze those commits when the fix is done, can you use 'git rebase -i' ? if not i can explain you how)

[asturur]

What does !! mean?

[sapics]

If we can use _updateObjectsCoords, this fix is unnecessary.

If we cannot use _updateObjectsCoords, when we use loadFromJSON from json created by older version of fabric.js, _objects have no __origHasControls.
Therefore, I wish to not object.hasControls = undefined, but object.hasControls = !!undefined = false.

[asturur]

ok i learned something new!
I think we have to use _updateObjectsCoords... i need time to test it.

[sapics]

Thank you for analizing!

Thank you for your kindness! I know new feature of git! git is really difficult, but much fun!
Now, I am in google..., it looks difficult... (git rebase -i, and set squash to later commit?)
I would use git rebase -i after the fix is done!

Sorry, I am rookie in git and github... Thank you very much for many helps!!

[sapics]

@asturur Now version is uploaded to https://jsfiddle.net/sapics/mq00xq3y/122/.
This version looks work well.

This version dose not use _updateObjectsCoords.
The reason is that group.toObject dose not restore group._objects, thus, left-top position of _objects have been fixed already for group in json-data.
And I cannot find that property originalLeft or originalTop was used. Is it necessary property?
If we need originalLeft or originalTop, I will fix to add property originalLeft and originalTop.

[asturur]

I have this PR in my list of things to finish ASAP.
I want to see if the other way i was thinking is good as this.
(the one in my previous comment :
Other idea in _calcBounds we should optionally set Top and Left JUST if they are not present in the options.
)
Because this works, but the logic of initialize looks then a bit weird, and if we skip calcBounds totally does not make me entirely happy.
I did not forget about this fix. i think it should be present in 1.5.1

[asturur]

https://jsfiddle.net/asturur/mq00xq3y/125/

Adding this to show that this indeed fixes also #2119.

[asturur]

@sapics, this is another proposal, is just a matter of code style more than some meaningfull changes. What do you think about it? i still did not test it. i'm going out now just want to share this
It should do same as your fix but with less code added.
/cc: @kangax

-   initialize: function(objects, options) {
+   initialize: function(objects, options, instanceFromObj) {
      options = options || { };

+     this._objects = [];
+     instanceFromObj && this.callSuper('initialize', options);

      this._objects = objects || [];
-     for (var i = this._objects.length; i--; ) {
-       this._objects[i].group = this;
-     }

      this.originalState = { };

      if (options.originX) {
        this.originX = options.originX;
      }

      if (options.originY) {
        this.originY = options.originY;
      }

+     if (!instanceFromObj) {
        this._calcBounds();
-       this._updateObjectsCoords();
        this.callSuper('initialize', options);
+     }

+     this._updateObjectsCoords(instanceFromObj);

      this.setCoords();
      this.saveCoords();
    },

    /**
     * @private
     */
-   _updateObjectsCoords: function() {
-     this.forEachObject(this._updateObjectCoords, this);
+   _updateObjectsCoords: function(instanceFromObj) {
+     this.forEachObject(this._updateObjectCoords, this, instanceFromObj);
    },

    /**
     * @private
     */
-   _updateObjectCoords: function(object) {
+   _updateObjectCoords: function(object, instanceFromObj) {
+     if (!instanceFromObj) {
        var objectLeft = object.getLeft(),
            objectTop = object.getTop(),
            center = this.getCenterPoint();

        object.set({
          originalLeft: objectLeft,
          originalTop: objectTop,
          left: objectLeft - center.x,
          top: objectTop - center.y
        });
+     }
-     //cannot we set object.group here?
+     object.group = this;
      object.setCoords();
      // do not display corners of objects enclosed in a group
      object.__origHasControls = object.hasControls;
      object.hasControls = false;
    },

[sapics]

@asturur I am sorry for answer late, i was going out. Thank you for your new proposal! It looks much simpler and it might work well!

I guess that object.group = this; would work.
But looks strange because function name is _updateObjectCoords.
And there are some possibilities that user custom delegated properties cause some problem at this.callSuper('initialize', options); with _objects[0].group is undefined (it might be very minor case.).
How about keep for (var i = this._objects.length; i--; ) this._objects[i].group = this; for safety?

[asturur]

yes we can live group association to object in initialize.
it was just an idea.

but the other change to me looks good.

i would like to hear also @kangax opinion about those changes. so we can merge it

[kangax]

Can't we call _updateObjectCoords here?

[sapics]

Yes we can! I would fix by @asturur suggestion.

[sapics]

@kangax Thank you for your comment!

With @kangax and @asturur suggestion, I fixed code.
As you know, my english is poor, if you find better comment or parameter name, please tell me!

[kangax]

alreadyGrouped → isAlreadyGrouped

[kangax]

@sapics Left couple comments. Please update and we're good to go! Thanks again.

[sapics]

@kangax @asturur Thank you for your support! It is done!

[kangax]

+changelog