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

Load erroneous image in ol.Image#load and ol.ImageTile#load #5270

Merged
merged 2 commits into from Jun 8, 2016

Conversation

fredj
Copy link
Member

@fredj fredj commented Apr 26, 2016

Allow tiles in ol.ImageState.ERROR state to be loaded.

To be able to reload erroneous tiles.

Fixes #4338

@@ -122,10 +122,11 @@ ol.Image.prototype.handleImageLoad_ = function() {


/**
* Load not yet loaded URI.
* Load not yet loaded or erroneous image.
Copy link
Member

Choose a reason for hiding this comment

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

I think Load the image or retry if loading previously failed. would make for clearer docs. Loading an "erroneous" image makes it sound like the intent is to load the wrong image.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion; I will change

@fredj fredj changed the title [wip] Load erroneous image in ol.Image#load and ol.ImageTile#load Load erroneous image in ol.Image#load and ol.ImageTile#load May 26, 2016
@fredj
Copy link
Member Author

fredj commented May 26, 2016

Ready to be reviewed, thanks

@gjn
Copy link
Contributor

gjn commented May 27, 2016

@fredj Will this load ad-infinitum when e.g. connection to internet breaks?

@fredj
Copy link
Member Author

fredj commented May 30, 2016

@gjn no, this only expose the load function and allow error tiles/images to be loaded again

@ahocevar
Copy link
Member

I may be missing something, but to me this looks indeed like error tiles will be retried over and over.

@fredj
Copy link
Member Author

fredj commented Jun 6, 2016

I may be missing something, but to me this looks indeed like error tiles will be retried over and over.

This needs to be checked but ol3 doesn't retry to load tiles on error.

@ahocevar
Copy link
Member

ahocevar commented Jun 6, 2016

You are right @fredj: https://github.com/openlayers/ol3/blob/v3.16.0/src/ol/tilequeue.js#L115-120 and https://github.com/openlayers/ol3/blob/v3.16.0/src/ol/reproj/tile.js#L321-L323 only call ol.Tile#load() when the tile state is IDLE.

That said, this is good to merge.

@@ -122,10 +122,11 @@ ol.Image.prototype.handleImageLoad_ = function() {


/**
* Load not yet loaded URI.
* Load the image or retry if loading previously failed.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note that loading is taken care of by the tile queue, and calling this method is only needed for preloading or for reloading in case of an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

note added

@fredj
Copy link
Member Author

fredj commented Jun 8, 2016

Thanks for the review

@fredj fredj merged commit e63b4d1 into openlayers:master Jun 8, 2016
@fredj fredj deleted the load_error branch June 8, 2016 07:25
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.

None yet

4 participants