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

onLoad not called when loading paper.Raster from canvas #924

Closed
sebastian-nowak opened this issue Jan 22, 2016 · 5 comments
Closed

onLoad not called when loading paper.Raster from canvas #924

sebastian-nowak opened this issue Jan 22, 2016 · 5 comments

Comments

@sebastian-nowak
Copy link

According to the docs source: HTMLImageElement⟋HTMLCanvasElement⟋String — the source of the raster so paper.Raster(source, ...) constructor should work correctly when given a canvas.

Let's try this:

var raster = new paper.Raster(someCanvas, new paper.Point(0, 0));
raster.onLoad = function ()
{
  alert('This function is not called');
};

If we replace someCanvas with url or data url, the onLoad callback is called. When given a canvas, it loads successfully but callback isn't invoked. Tested on paper 0.9.25.

// Edit: after looking into the source, it appears that onLoad won't be called when passed an image too.

@lehni
Copy link
Member

lehni commented Jan 22, 2016

I don't think an onLoad even makes sense for a passed canvas object, as that's a buffer in memory, not something you load form an external source. But it would make sense for it to be called if you pass an image object which hasn't been loaded yet. So that would be a bug.

@sebastian-nowak
Copy link
Author

I have to disagree and here's a simple example why such behavior is quite weird.

Just imagine you're writing a library which accepts a picture in the configuration options, loads it to paper.Raster and do some processing. It should be possible to just forward the picture to the paper.Raster constructor and everything else should work without additional code.

If you don't call onLoad for some types, the library would have to check what type the passed picture is and run a different code for each of them, and that's pointless as all of these types are directly supported by Paper.

So please, keep the api consistent and invoke the onLoad callback when paper.Raster finishes loading the passed picture, no matter the type. It's the most intuitive and friendly way.

@lehni
Copy link
Member

lehni commented Jan 23, 2016

Well, there are different ways of defining consistency. Currently, only the setter for Raster#source handles the onLoad() calls, just like setting image.src would trigger a load event on an native image, once the content is loaded (or was retrieved from the cache).

The setter for Raster#image doesn't trigger load, which I think is expected behavior, since you're passing an image and are responsible for its contents yourself. There is nothing to load for Paper.js, as it's assuming the image or canvas is in memory alread. Paper.js is not checking if the image is loaded already, and it is not waiting for it to be available. It copies its contents right away, and hence it would be misleading if it was emitting the load event, because that would suggest otherwise.

The constructor looks at the passed object and decides if it should use the source setter or the image setter. In a typed language, this would not be one constructor, it would be two, and only one of them would trigger the loadevents, the one receiving the string argument.

From where I stand, I think the current behavior is pretty consistent, and adding what you'd like to add would make it unclear as to what is actually happening internally.

@lehni
Copy link
Member

lehni commented Jan 23, 2016

Thinking a bit more about this, I think I could move the whole load event handling to the Raster#image setter, and then use that from Raster#source. I think that would probably be the more elegant solution anyway : )

@lehni
Copy link
Member

lehni commented Jan 26, 2016

This turned into a rather big undertaking that made me add functionality to jsdom (jsdom/jsdom#1365) and prioritize #739. Things are now in place for this to be addressed next.

lehni added a commit that referenced this issue Jan 26, 2016
- Trigger #onLoad() events from Raster#setImage() also
- Add support for Raster#onError() handler
Closes #849 and #924
@lehni lehni closed this as completed Jan 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants