Hi all,
for the second time in a week I'm cross posting on geotools and
geoserver list for a topic that is of interest of both communities.
Some months ago, right before the GeoTools 2.7.0 release, Jody
(and Michael too I guess?) made and implemented this proposal:
http://docs.codehaus.org/display/GEOTOOLS/MapContext+Refactor
The proposal was not fully executed as the renderer still works against
MapContext and friends.
Now Michael has worked a patch to complete the proposal, on which
he is waiting for in order to get some work done on the swing module:
http://jira.codehaus.org/browse/GEOT-3565
The original patch deprecated just MapContext. However, since MapContent
is made of Layer classes (which is actually a hierarchy or type specific
classes, see attached screenshot), it also makes sense to deprecate
MapLayer (which is-not-a Layer).
This generates a number of deprecated usages for applications that
work a lot with maps.
Most applications that just setup a map and have it rendered
will have it somewhat easy, just change the part that builds the
map, however others, more complex ones will be hit harder.
GeoServer is one such example, MapContext and MapLayer are not
just built to feed StreamingRenderer, but they are also used later
to analyze the structure of the map and fed into at least five custom
renderer, the kml one, the streaming svg one, rss and atom, and
the html image map one.
Also the testing code sets up and uses a lot those classes too.
I've spent all the day performing a tentative switch of the code base
and attached the resulting patch here:
http://jira.codehaus.org/browse/GEOS-4597
As you can see the patch is not small, weighting 121KB, and affects
pretty much all of the wms module, plus the CRS area of validity
preview in the GUI modules, gwc integration, html image map output
format and the python module.
The patch basically does two things:
- switches WMSMapContext to be a MapContent extension
- switches all MapLayer usage to using Layer subclasses
The first is mechanical, a lot of getAreaOfInterest/setAreaOfInterest
needed to be changed to getViewport().getBounds/setBounds.
The latter is more of black magic, and I probably made mistakes
during the conversion, especially in the KML generation subsytem
which was the most heavily hit.
The latter is kind of a problem because all of the code using layers
in GeoServer assumes that:
- all layers are uniform unless otherwise needed
- every layer has a query
- every layer has a style
- every layer can return a feature source
The "Layer" hierarchy breaks all of the above assumptions.
Throughout the code I had to make guess on what kind of layer
the code was meant to work with (feature? grid coverage reader?
wms layer?) and perform casts accordingly, of course with
quite the number of "instanceof" checks spread around.
Now, the patch survives a build and some interactive testing.
I did not, however, check KML generation: there are a million
different code paths and configurations and I'm not well
setup to test them on Linux (GE does not really work well
here).
All the patch needs a review, but the KML part really needs
a detailed one along with tester willing to check out all of the
usage cases (network links, superoverlays, cached regionation
hierarchies, extrusion and time support, gwc integration, and so on).
Need help on this one, I'm not going to do it myself.
The patch also has maintenance implications for GeoServer: it will
make harder to backport to the stable branch any patch affecting
WMS, as we'll have to write it for Layer on trunk and recode it
for MapLayer on 2.1.x.
In particular it will very likely intersect with my time/elevation
work, making the first patch landing require reworks in the other.
Anyways, during the switch I took down notes about things that
bite me during the switch and probably need improvement on
the GeoTools side:
- GridReaderLayer constuctors force one to specify the title to
pass also the parameters. Title might not be always needed,
but some readers will not give the best without custom param
settings. Please add a constructor wtih reader, style and params
- getAreaOfInterest() replacement is getViewport().getBounds(),
made me stare for a good five minutes at the API looking for
it though. I guess we need an upgrade guide and this one should
be cited
- getViewport().getBounds() returns null if not explicitly initialized.
That also got me into quite a bit of trouble, the old classes
would initialize the bounds to the layer bounds if no manual
initialization was made.
Please get back this behavior, otherwise upgraders will have to
fight quite a bit of NullPointerExceptions
- GridCoveageLayer and GridReaderLayer really need a GridLayer
base class, right now to recognize if a layer is raster one has
to make two instanceof, two casts, and if the code relying on the
ability to extract a feature source out of the layer is hard to
change, two casts to call the toFeatureCollection() method that
both expose
Opinions, suggestions, concerns... even flames if you really need to...
welcomed!
Cheers
Andrea
--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 962313
http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf
-------------------------------------------------------
(attachments)
