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

knockout tracking the 'viewer.selectedEntity' return null #2734

Closed
wushengcq opened this issue May 19, 2015 · 9 comments
Closed

knockout tracking the 'viewer.selectedEntity' return null #2734

wushengcq opened this issue May 19, 2015 · 9 comments

Comments

@wushengcq
Copy link

Hello ~
After I update my Cesium from v1.4 to v1.9, the entity selected event tracking function can't working anymore. When I subscribe the tracking function at the Cesium's startup as below :

 Sandcastle.finishedLoading();
 Cesium.knockout.getObservable(viewer, 'selectedEntity').subscribe(function(entity) {
        if (entity !== undefined) {
             console.log(entity.id);
        }
});

browser console throw a exception :
Uncaught TypeError: Cannot read property 'subscribe' of null

@hpinkos
Copy link
Contributor

hpinkos commented May 19, 2015

It should work if you change it to Cesium.knockout.getObservable(viewer, '_selectedEntity')

In the future, please use the forum to ask questions and get support. Your questions will be seen by more people and you'll hear from other users who have solved similar problems. We try and only open GitHub issues for confirmed bugs or enhancements. Thanks.

@hpinkos hpinkos closed this as completed May 19, 2015
@wushengcq
Copy link
Author

It's works !!
Thank you very much for your code and reference to the forum.

@emackey
Copy link
Contributor

emackey commented May 20, 2015

Are we sure this isn't a bug? _selectedEntity is private, and I don't know of a public way to subscribe to this observable since we rolled the entityMixin into core Viewer.

@emackey
Copy link
Contributor

emackey commented May 20, 2015

I suspect this maybe was unplanned API breakage.

@hpinkos
Copy link
Contributor

hpinkos commented May 20, 2015

Yeah, this is an unfortunate side effect of using the knockout-es5 plugin. You have to know the real variable name of the observable to get it back.

@mramato
Copy link
Member

mramato commented May 20, 2015

We could fix this by using knockout.defineProperty to define the selectedEntity instead of using definedProperties, we actually do this a bunch of other places already.

That being said, longer term I think the knockout subscription approach is a bit obtuse. I'd rather users never have to know that knockout is used under the hood (even though I used to think otherwise). My vote would be to expose a selectEntity event. Even better would be the ability to cancel a selection before it happens. Same for tracking. Of course we shouldn't just do this without thinking about it first because I don't want to create too much churn in this area and we have some related issues that should be thought of in concert with this.

Ed, if you feel strongly about it, I'd be happy to merge a pull that uses knockout.defineProperty for trackedEnttiy and selectedEntity to go back to the pre-mixim-merge behavior.

@emackey
Copy link
Contributor

emackey commented Aug 11, 2015

I'm re-opening this since it was discussed again on the forum.

We should consider implementing @mramato's suggestion above.

@emackey emackey reopened this Aug 11, 2015
@forman
Copy link
Contributor

forman commented Dec 30, 2015

Is there any news on the status of this issue? I am facing exactly the same problem: I want to know when viewer.selectedEntity changes, which is btw a very important event for application development. Is the use of knockout now discouraged or not? Anyway, I tried @mramato's suggestion with Cesium 1.16 but unfortunately the selectedEntity event never seems to fire:

            var evt = viewer.selectedEntity.definitionChanged;
            evt.addEventListener(function (property) {
                alert("Definition changed: " + property);
            });

Maybe this is this another issue?

@emackey
Copy link
Contributor

emackey commented Mar 1, 2017

This is fixed in #5043.

@emackey emackey closed this as completed Mar 1, 2017
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

6 participants