Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add setter for GeoJsonDataSource.name #5653

Closed
cguldner opened this issue Jul 19, 2017 · 5 comments
Closed

Add setter for GeoJsonDataSource.name #5653

cguldner opened this issue Jul 19, 2017 · 5 comments
Labels

Comments

@cguldner
Copy link
Contributor

It would be nice to be able to set the name for a DataSource, as currently, an error isn't even thrown, but the code following a call to data.name is completely ignored.

Consider this code

let data_promise = Cesium.GeoJsonDataSource.load(url);
data_promise.then(data => {
    viewer.dataSources.add(data);
    console.log(1);
    data.name = "Test";
    console.log(2);
});

In this case, the 2 would not get printed, and the name of the dataSource wouldn't get updated. At the very least, I think an error should pop up so that people aren't left wondering why their code isn't executing, but being able to set the name for the layer would be a good thing to have, and should be very easy to implement.

@mramato
Copy link
Contributor

mramato commented Jul 19, 2017

While I have no objections to adding setters to DataSource names (not sure why prevented it in the first place), you are incorrect in that this is a silent failure. It will generate an exception which results in a rejected promise. If you aren't handling promise rejection, then you are ignoring a myriad of possible errors.

Here's a Sandcastle example showing the error being raised and handled:

http://cesiumjs.org/Cesium/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=ff3a8ec7cabf93b402a29d66d534a2d0

@hpinkos
Copy link
Contributor

hpinkos commented Jul 19, 2017

Hello @burn123. This does currently throw an error because name only has a getter, but the error is being lost because it's happening inside the promise. That's why 2 is never printed. You can get the error by adding an otherwise:

data_promise.then(function(data) {
    viewer.dataSources.add(data);
    console.log(1);
    data.name = "Test";
    console.log(2);
}).otherwise(function(error) {
    console.log(error);
});

Why would you like to set the name? Right now name only has a getter, but we can consider adding a setter if that would be useful. I'm honestly not really sure what uses DataSource.name.

@cguldner
Copy link
Contributor Author

I put my fail function as the second argument of then, but apparently that isn't running, which is why I thought that errors were being passed silently.
@hpinkos I'm using it as part of an app I'm developing, built on top of Cesium, and in the case of files like GeoJson, there isn't a name property that can be put inside the file like there can be with KML, so that's why I would like it, so I could programmatically set the name

@hpinkos hpinkos changed the title Setting name for DataSource fails quietly, should add setter Add setter for DataSource.name Jul 19, 2017
@hpinkos hpinkos added good first issue An opportunity for first time contributors category - data sources type - enhancement labels Jul 19, 2017
@hpinkos hpinkos changed the title Add setter for DataSource.name Add setter for GeoJsonDataSource.name Jul 19, 2017
@hpinkos
Copy link
Contributor

hpinkos commented Jul 19, 2017

Thanks @burn123! I opened a pull request here: #5656

@cguldner
Copy link
Contributor Author

You guys are so fast! Thanks @hpinkos!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants