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

setOverlayImage and excludeFromExport = true #4104

Closed
ncou opened this issue Jul 15, 2017 · 9 comments · Fixed by #4119
Closed

setOverlayImage and excludeFromExport = true #4104

ncou opened this issue Jul 15, 2017 · 9 comments · Fixed by #4119

Comments

@ncou
Copy link
Collaborator

ncou commented Jul 15, 2017

Hi,

Tested with the v2.0b3

I set an overlay object, but i don't want it to be exported in json.
I was thinking the parameter "excludeFromExport = true" on the object used as overlay could result in an empty json node for the "overlayImage" tag. But the node is still present with the object even if the parameter "excludeFromExport" is set to true for the object.

Is that a bug, or a wanted behavior ?

Let me know if a jsfiddle is needed to confirm this ticket.

Thank you.

@asturur
Copy link
Member

asturur commented Jul 15, 2017

is a bug.

@asturur asturur added the bug label Jul 17, 2017
@asturur
Copy link
Member

asturur commented Jul 18, 2017

The function is __serializeBgOverlay in static_canvas.class.js.
Is very easy to fix for both bgImage and overlayImage.

If you have time and want to propose a fix ( with a test case ) i will not be able to do it untill tomorrow.

@ncou
Copy link
Collaborator Author

ncou commented Jul 18, 2017

Hi,

Not really sure if i could find some times tonight.
I looked at the source code, and i think there is 2 possibilities (but i didn't test it).

I could change line 1157 the two "if" like this (to add a check on the excludeFromExport value) :

if (this.backgroundImage && !this.overlayImage.excludeFromExport) {
        data.backgroundImage = this._toObject(this.backgroundImage, methodName, propertiesToInclude);
      }
      if (this.overlayImage && !this.overlayImage.excludeFromExport) {
        data.overlayImage = this._toObject(this.overlayImage, methodName, propertiesToInclude);
      }

But perhaps a more cleanner solution is to change the __toObject() function (there is already a filter in the "__toObjects()" function) in the static_canvas class.
add at the start of the function something like this :
if (instance.excludeFromExport) { return; }

PS : I don't really know how to handle the "test case", can you point me a "good" example i could reuse ? I will use Google later to better understand this part.

@asturur
Copy link
Member

asturur commented Jul 18, 2017

yes maybe adding an early exit to _toObject is better.
Remove the filter statement over the objects array.

I'll point you to a test case. There is no rush to fix this.

@ncou
Copy link
Collaborator Author

ncou commented Jul 18, 2017

When you said "Remove the filter statement over the objects array.

You mean removing the statement .filter(function(object) { return !object.excludeFromExport; }) code in the __toObjectS function (because there will be an early exit in the __toObject() function) ? is that right ?

@ncou
Copy link
Collaborator Author

ncou commented Jul 18, 2017

I made a quick test and it work fine (tested with the propertie "excludeFromExport" : true and false for the overlay object, not tested yet for the backgroundimage object).

Here are the changes i made : remove the .filter function in the _toObjects() function, and add an early exit in the _toObject() function.

   _toObjects: function(methodName, propertiesToInclude) {
      return this.getObjects().map(function(instance) {
        return this._toObject(instance, methodName, propertiesToInclude);
      }, this);
    },

    _toObject: function(instance, methodName, propertiesToInclude) {
      var originalValue;
      if (instance.excludeFromExport) {
      	return;
      }
      .........
    },

Is there some tools or preset to use to format the fabric.js code ?

@asturur
Copy link
Member

asturur commented Jul 22, 2017

ok i ll open a PR with this changes and closing the issue

@asturur
Copy link
Member

asturur commented Jul 22, 2017

i had to make the longer fix since we are not outputting null properties to object. so returning null would require extra code to then remove the empty objects or props.

@ncou
Copy link
Collaborator Author

ncou commented Jul 22, 2017

Strange, because with the modification i tested above, i didn't get 'null' in the outputed json, the node was not present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants