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

Make state an observable property of ol.source.Source #2248

Closed
wants to merge 1 commit into from

Conversation

pagameba
Copy link
Member

This changes ol.source.Source to extend ol.Object and adds state as an observable property so that it is easier to detect when a source's state has changed.

This changes ol.source.Source to extend ol.Object and adds `state` as an observable property so that it is easier to detect when a source's state has changed.
@pagameba
Copy link
Member Author

While I can confirm this works (change:state is triggered), I couldn't find a convenient place to add a test. Ideally it would be in the vectorsource.test.js file, but the change:state event is only triggered by specific format sources (eg ol.source.GeoJSON) and there are no existing tests for these that I can see. I can add a new test file (test/spec/ol/source/geojson.test.js or staticvectorsource.test.js) if its deemed desirable.

@elemoine
Copy link
Member

I don't think there are new tests to add for this change.

@tschaub
Copy link
Member

tschaub commented Jun 26, 2014

It looks to me like with this change the change event will not be dispatched when the state is changed (and the revision will not be incremented). There are some fragile and unguessable things that may break. Tests would prove otherwise.

@elemoine
Copy link
Member

You're very right Tim. We need to be very careful here.

@ahocevar
Copy link
Member

@pagameba Your application could also just listen for the change event and check the state in the listener. Then this pull request would be obsolete.

@pagameba
Copy link
Member Author

Did you read my cautionary tale? change is fired from the source when features change. state becomes ready once and is still ready when features change. If you change features in response to change and testing for ready then you end up with recursion. This can be avoided by deregistering the event listener when state is ready for the first time, but it is very easy to get tripped up on this.

I'm happy to add tests, I just need some guidance on where to put them.

Also, probably dispatchChangeEvent() and seems like an omission.

@pagameba
Copy link
Member Author

Discussed with @elemoine, @tschaub, and @ahocevar (in details I didn't always follow), there is a larger issue in the library concerning the use of ol.Object and propertychange vs ol.Observable and the more generic change event. It seems inadvisable to proceed with this PR until the larger architectural issue has been discussed and an approach selected.

@pagameba pagameba closed this Jun 26, 2014
@pagameba pagameba deleted the source-state-property branch July 7, 2014 13:02
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.

None yet

4 participants