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

Fix layer property reactivity #368

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fschmenger
Copy link
Collaborator

This is my take on how we could possibly fix the layer-property-reactivity issue mentioned in #365.
To keep changes minimal I decided to wrap up OL layers and OL layer collections by proxy objects. The proxy objects behave mostly transparent and forward invocations to the underlying layer / collection objects, so they can be basically used as a drop-in. Apparently, use them only for Vue components when reactivity is required.

Using LayerProxy around an OL layer:

  • Construct a proxy object with the list of properties that must be accessible on the layer: layerProxy = new LayerProxy(myLayer, ['legend', 'legendOptions'])
  • Proxy objects must be destroyed before they go out of scope: layerProxy.destroy()
  • Use the underlying layer to pass it on to child components and APis: layerProxy.getLayer()
    Internally, the layer proxy traps the getProperties() and get() methods to return a self managed object of key value pairs, which is synced via OL observables.

Similarly, using LayerProxyCollection around an OL Collection:

  • Construct a proxy object with the list of properties that must be accessible on the layers: collectionProxy = new LayerCollectionProxy(myLayerCollection, ['lid', 'displayInLayerList', 'isBaseLayer']);
  • Proxy objects must be destroyed before they go out of scope: collectionProxy .destroy()
  • Use the underlying collection to pass it on to child components and APis: collectionProxy.getCollection()
    Internally, the collection proxy traps the forEach(), item() and getArray() methods to return LayerProxy objects.

So far, this only the proof of concept. Here is the list of (open) tasks:

  1. Implement layer proxy object (done)
  2. Implement layer collection proxy (done)
  3. Use layer proxy and proxy collection in various vue components. A quick search yielded:
  • LayerList (done)
  • LayerListItem (done)
  • LayerLegendImage (done)
  • AttributeTable
  • AtributeTableWin
  • BgLayerList
  • BgLayerSwitcher
  • LayerPreviewImage
  • Geolocator
  • OverviewMapPanel
  1. Update the unit tests

@fschmenger
Copy link
Collaborator Author

Hi guys, here is where to take a closer look:

  • Are we happy with the overall solution? I couldn`t come up with something simpler but maybe there is...
  • LayerProxy currently only manages properties, which are passed into the constructor. Other properties are not accessible. Is this a good design or should we try and manage/sync all available properties on the layer? (However those would have reactivity restrictions as described in OL layer properties are no longer reactive #365)
  • Unfortunately proxy objects must be destroyed to free event listeners and there is no JS destructor syntax either. Currently I did not implement reference counting. So the assumption is that you only destroy Proxy objects that you have created. E.g. don`t destroy LayerProxy objects which are maintained by the LayerProxyCollection.
  • This is why LayerCollectionProxy currently does not trap and handle the pop(), push(), remove(), removeAt() and setAt() methods and just accepts/ returns raw OL layers there- see OL docs. I didn`t wanna reason about ownership patterns for now.

Unfortunately I will have to focus on other projects again in the upcoming months, so take your time for review.
If we're happy with the overall solution, feel free to work on it. It should`t be too much from here on.

Cheers Felix

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.

1 participant