From: Andrea A. <aa...@op...> - 2010-08-30 06:40:21
|
Gabriel Roldan ha scritto: > Hi developers, > > I would like to ask for review and comments (and subsequently > approving once any concern that could raise is acted upon), of the > following GeoServer branch on my git repository that addresses the > long standing and much annoying WMS port to the same architecture > than the other OWS implementations: > <http://github.com/groldan/geoserver_trunk/tree/wmscleanup> > > If you ever had to deal with the WMS module source code, you'll be > amazed that you won't ever have (anymore) to: - wonder where to look > for some class related to certain specific functionality: now > everything's grouped into functionally cohesive packages, rather than > coincidentally or logically (please see the attached before/after > package screenshots too see what I mean). - figure out how the > awkward GetMapProducer API is turned into something the Dispatcher > can actually handle, nor get shocked when you find the only way is to > actually produce the map when the GetMapProducer.getMimeType() method > is called, nor wonder why a GetMapProducer needs to create a map and > write it, when the Dispacher has operations and responses to > separate out those responsibilities. - freak out about the 2003/2004 > Jalopy days DOCUMENT ME! anti-comments left overs > > In a word, this work is not only a package reorganization, but an > attempt to _finally_ bring the old servlet based, inheritance abusive > and too wired spaghetti WMS code into 2010's GeoServer standards. > > Some technical comments to guide you in your assessment: - > GetMapProducer refactored as GetMapOutputFormat with the sole > responsibility of producing a Map for a specific format - The > org.geoserver.wms.Map abstract class is the result of a GetMap > operation, and what a GetMapOutputFormat produces. The class name is > open to debate as I acknowledge it may be weird. - > org.geoserver.ows.Response concrete subclasses shall be registered as > Spring beans to deal with writing each specific org.geoserver.wms.Map > specialization (for example, BufferedImageMap, XMLTrasnsformerMap, > RawMap). So more than one GetMapOutputFormat implementation can > produce the same kind of Map (ie, XMLTransformerMap) which then will > be written down by a single Response implementation. - Contrary to > GetMapProducer, GetMapOutputFormats can be (and are recommended to > be) Spring singletons, meaning they're stateless by nature as the > state needed to further encode the map into the desired output format > is encapsulated in the returned org.geoserver.wms.Map. Besides more > programming simplicity and adherence to the Dispatcher framework, > this might lead to a little performance benefit since no new > instances need to be created for each and every GetMap request? - > Same thing goes for GetFeatureInfo and GetLegendGraphic output > formats, no more SPI mechanism, plain Spring beans lookup. For > example, the GetFeatureInfo operation returns a FeatureCollectionType > (like a WFS GetFeature one) and more response handlers can be easily > plugged in to deal with alternate output formats. > > Sorry it's becoming a long mail, please feel free to raise any > concerns. I don't expect the branch to be perfect yet but it > certainly looks to me like it's going in the right direction. > > The final goal is incorporating these changes into trunk asap. > Thankfully with the marvels of git that part is easy and conserves > full history. Gabriel, thanks for doing this work, it is indeed very much needed and appreciated. The amount of changes is staggering (80 commits) and it will take me a few days to go through them: the testing coverage in the wms module is light so a line by line visual inspection is needed before we land this patch. Even after a careful inspection I expect there will be numerous side effects, but the cleanup has long term benefits and it's worth some short term pain. Given FOSS4G is approaching I have no time to look into this for the next two weeks, but I hope to be able to come back to it once the FOSS4G storm calms down. Or maybe, if we find enough time, we can go through the changes togheter in Barcelona... uh, we could use the code sprint for this :-) Cheers Andrea -- Andrea Aime OpenGeo - http://opengeo.org Expert service straight from the developers. |