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

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!

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

No branches or pull requests

3 participants