Skip to end of metadata
Go to start of metadata

Issues with the trunk code can be discussed here

Review on /lib/graphics:
function isWmsLayer()
Cameron:

I notice that all your Layer objects have a isWmsLayer() function. A more generic way of writing this could be getLayerType() which returns "wms" or "rss" or whatever the layer name hapens to be. Or even more generic, does JS provide a getObjectName() function or similar which returns the name of the JS class?

Patrice:

I had a hard time with this one for some obscure JS reason and needed to move on. I agree that this should get refactored at some point.

WmsLayer.MapImgLoadHandler()
Cameron:

This seems to be it's own class, which is stored in a JS file by another name. Although we have some old code which does this, all our recent code has one class per JS file. The consistancy makes the code easier to debug when you know where to look for a class.

Patrice:

This code was lifted as is from the old MapPane.js and is only used for the re-painting of WmsLayer. I am not sure why you would want to put it somewhere else.

MapLayer.js
Cameron:

MapLayer.js is storing state data like layerName, layerNode, visible, etc. This state data should be stored in the model class - usually Context.js and requested from Context.js when needed.

Patrice:

I am not sure why you would want to do this. This is used all the time and reparsing the DOM every time is not very effective. I am not a proponent of premature optimization but this one seemed natural at the time.

Cameron:

The key here is ensuring that you don't have 2 data sources which get out of
date.
I acknowlege your reasons for optimisation, but if we do it, we also need to add syncronisation code which updates the data in both locations when the other location changes, or force all accesses to state data to always be through one place - ie MapLayer.
(It can get messy).

WmsLayer
Cameron:

WmsLayer contains the following line:
imgDiv.imgId = Math.random().toString();
Is imgId supposed to be a unique Id? If so, I suggest using an algorithm which won't occasionally return non-unique ids like random(). Maybe use a date() value instead.

Patrice:

This was lifted as is from the old code (from Mike or you?). It has been working so I am not sure I wanted to fix it.

Cameron:

This code will work 99.9% of the time, and 0.1% of the time you will see an intermittent failure that is almost impossible to reproduce. I strongly recommend we fix it. Using an ID based on date is guaranteed to provide a unique number. It would by a one line fix.

MapLayerMgr.paint()
Cameron:

This function contains WMS specific rendering which should be moved into WmsLayer.js . A Context may contain Google Map layer and Vector layer only, and hence shouldn't need to load the WmsLayer code.

Patrice:

The layerMgr is keeping an imageStack (used for WMSLayers). This was lifted from old code and I did not want to touch it just yet. I agree with the second part but we do not know how to do this dynamic
loading.

Visible layers
Cameron:

Selecting or deselecting a layer in the legend doesn't cause the layer to be hidden in the MapPane. I suspect this may be caused by Context and MayLayerMgr both storing the same state data in different places.

Patrice:

This was working for me. MapPane is the one hiding/showing the divs. Which example is broken. I think there are 2 versions of this. Let me know.

AddLayer doesn't trigger paint
Steven:

I found that adding a layer to mappane2 doesn't trigger a refresh on the mappane2. Adding layers is now handled by the MapLayerMgr, but painting is done by the mappane. I think this should be done differently, not sure yet how.

  • No labels