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

Added image error handling #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ytyukhnin
Copy link

Hi could you please merge this small change? I think it's useful. Thank you.

If an image cannot be downloaded it throws lots of exceptions during
drawing, added onError handler to the images solves this issue.

If an image cannot be downloaded it throws lots of exception during
drawing, add a onError handler to the images solves this issue
@goat1000
Copy link
Owner

goat1000 commented Nov 7, 2014

I've just done some testing with this, and it's not very clear to me how this change helps. If you add an error handler to the images to fix the image, then it looks like TagCanvas can used the fixed image without your modification.

If the error handler doesn't update the image, then TagCanvas will still have a broken image and the change won't help.

I can see how adding an error handler to TagCanvas would be useful, but I think it might be better to make TagCanvas deal with the error itself instead of using a handler from the image element.

Do you have any examples of where this change is really useful?

@ytyukhnin
Copy link
Author

Hi,

Thank you for your reply. A use case is exactly that you'd like to update
your image in the error handler.

Here is it:

I have my images hosted out of my control, so some of them are OK, some are
no longer available (404), some are hosted on some crappy servers (500) or
sent with an incorrect content type.
I'm generating a list of As and IMGs dynamically:

Broken or unavailable images while drawn on the canvas are generating an
infinite number of errors, hanging all the UI.

image

With my update it's easy to handle

I'd like to display another image in place of the broken one (while
additionally keeping it possible to provide a tooltip and other info which
might be useful to display)

The update I'm proposing helps to handle this use case while keeping the
TagCloud behaviour as it was if no error handler was provided.

Hope this example is clear enough. Should you have any questions, please
don't hesitate.

Thank you.

2014-11-07 11:30 GMT+01:00 goat1000 notifications@github.com:

I've just done some testing with this, and it's not very clear to me how
this change helps. If you add an error handler to the images to fix the
image, then it looks like TagCanvas can used the fixed image without your
modification.

If the error handler doesn't update the image, then TagCanvas will still
have a broken image and the change won't help.

I can see how adding an error handler to TagCanvas would be useful, but I
think it might be better to make TagCanvas deal with the error itself
instead of using a handler from the image element.

Do you have any examples of where this change is really useful?


Reply to this email directly or view it on GitHub
#6 (comment).

@goat1000
Copy link
Owner

I've tried out the onerror handler, and it replaces the broken image with a new one as expected.

But the extra line in TagCanvas doesn't appear to make any difference. Without the "onerror=" in the img element, TagCanvas doesn't work with or without the extra line. With the "onerror=" in the img element it does work with or without the extra line.

I'm just trying to understand under what conditions it makes a difference to have the new line of code. Is it only required for specific browsers, or when it takes a long time for the image to fail?

@ytyukhnin
Copy link
Author

Hi,

Sorry for this delay. Actually I was trying to reproduce it using jsfiddle
but I cannot, seems that you are right about that line has no effect. I'll
check this change one again where it was used get back to you.

Thank you.

2014-11-11 16:38 GMT+01:00 goat1000 notifications@github.com:

I've tried out the onerror handler, and it replaces the broken image with
a new one as expected.

But the extra line in TagCanvas doesn't appear to make any difference.
Without the "onerror=" in the img element, TagCanvas doesn't work with or
without the extra line. With the "onerror=" in the img element it does work
with or without the extra line.

I'm just trying to understand under what conditions it makes a difference
to have the new line of code. Is it only required for specific browsers, or
when it takes a long time for the image to fail?


Reply to this email directly or view it on GitHub
#6 (comment).

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.

2 participants