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

Allow promise to url for Cesium terrain, 3d tiles and TMS imagery #6204

Merged
merged 4 commits into from
Feb 13, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Feb 9, 2018

@mramato

I looked into adding this functionality to some of the other terrain and imagery providers, but some are a bit more complicated than others. I figured I'd start with these types and open up another PR with the others if this approach is what we want.

@cesium-concierge
Copy link

Signed CLA is on file.

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

var metadataError;

var layers = this._layers = [];
var attribution = '';
var overallAvailability = [];
when(options.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

If options.url rejects, this will fail silently, don't we need to move this entire block into requestMetadata itself? Also, request Metadata is using old (improper) promise usage, change the success and failure functions off of this promise when you move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you want the behavior to be when options.url rejects? I don't think calling metadataFailure is the right option because it will still try to load the tileset even if fetching the metadata fails. And we literally can't to anything with the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I assume we want to just propagate the url rejection to the readyPromise so that it rejects. This way a user gets the exact error.

var deferred = when.defer();
var imageryProvider = new UrlTemplateImageryProvider(deferred.promise);

var metadataError;
var resource;
var xmlResource;
when(options.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment to CesiumTerrainProvider regarding failure and requestMetadata.

@mramato
Copy link
Contributor

mramato commented Feb 12, 2018

Please add unit tests for the rejection use cases.

@mramato
Copy link
Contributor

mramato commented Feb 12, 2018

That's all of the comments I have. This approach looks like exactly what I had in mind. Thanks @hpinkos!

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 12, 2018

@mramato this should be ready now

requestMetadata();
})
.otherwise(function() {
deferred.reject(new RuntimeError('An error occurred while loading options.url'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than providing our own error here, do deferred.reject(e) where e is the argument to otherwise (which you left out in this case). Same comment in createTileMapServiceXXX function

@mramato
Copy link
Contributor

mramato commented Feb 12, 2018

Update CHANGES.

Just those two comments. I'm fine with merging this and updating other providers later (unless you have time/desire to update them all now).

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 13, 2018

@mramato ready

@mramato
Copy link
Contributor

mramato commented Feb 13, 2018

Thanks, I'll merge as soon as this is green.

@mramato mramato merged commit 0e4afd3 into master Feb 13, 2018
@mramato mramato deleted the promise-url branch February 13, 2018 18:01
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.

3 participants