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

Geocoder is not working if I provide true value explicitly #6826

Closed
SurajitChowbey opened this issue Jul 23, 2018 · 7 comments
Closed

Geocoder is not working if I provide true value explicitly #6826

SurajitChowbey opened this issue Jul 23, 2018 · 7 comments

Comments

@SurajitChowbey
Copy link

SurajitChowbey commented Jul 23, 2018

Geocoder is not working if I provide true value explicitly.

Please take a look on the following code:

Not working example:
var viewer = new Cesium.Viewer('cesiumContainer',{ geocoder: true, });

Working example:
var viewer = new Cesium.Viewer('cesiumContainer',{ /*geocoder: true,*/ });

@hpinkos
Copy link
Contributor

hpinkos commented Jul 23, 2018

Thanks for reporting this @SurajitChowbey! We currently expect either a GeocoderService or false for the geocoder property, which is confusing. We either need to update the documentation to reflect this, or use a separate property to set a custom geocoder service.

@mramato do you have an opinion here? I feel like overloading the geocoder argument in this way was the wrong decision and we should have options.geocoder be a boolean and add options.geocoderServices.

@OmarShehata
Copy link
Contributor

A better solution is to remove geocoder and just have geocoderServices. If it's defined then the feature is enabled, just like with imageryProvider and terrainProvider.

@mramato
Copy link
Contributor

mramato commented Jul 24, 2018

@mramato do you have an opinion here? I feel like overloading the geocoder argument in this way was the wrong decision and we should have options.geocoder be a boolean and add options.geocoderServices.

I believe this mirrors similar SDKs at the time it was written, but it was so long ago I don't remember for sure. I would recommend just handling the case where the value is true and call it a day. This has never been a problem before and it would create an immense amount of churn to change because all of the Viewer options follow this paradigm.

@mramato
Copy link
Contributor

mramato commented Jul 24, 2018

A better solution is to remove geocoder and just have geocoderServices. If it's defined then the feature is enabled, just like with imageryProvider and terrainProvider.

The geocoder is enabled by default, so it already works like imageryProvider and terrianProvider. There's technically no bug here, it's documented behavior, but it's an easy usability improvement to make true work for all of the widgets.

@emackey
Copy link
Contributor

emackey commented Jul 24, 2018

In retrospect we probably should have used null instead of false back in the day, then people wouldn't have thought to switch it to true, they would understand to supply an instance.

hpinkos pushed a commit that referenced this issue Jul 24, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Jul 25, 2018

Thanks @SurajitChowbey! This was fixed and will be included in the Cesium 1.48 release available August 1st

@SurajitChowbey
Copy link
Author

Welcome

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

5 participants