On Tue, 2009-03-31 at 15:01 -0600, Justin Deoliveira wrote: > Good stuff David. I looked over the proposal quickly and did a quick > code review and here is what i got: > > * I wonder if the interface is better named MapDecoration, or > MapOverlay? Decoration seems a bit too abstract
My order of preference is Decoration > MapDecoration > MapOverlay. The Map prefix seems unnecessary since this is in the wms package. I am not planning to dig in my heels on this issue though. > * I notice the old package naming structure is being used in the > community module Well... I am hesitant to rename the org.vfny.geoserver.wms packages as they are referenced a lot in applicationContext files (which aren't checked by the compiler). I guess we have a few reasonable options with respect to this: * leave everything in org.vfny.geoserver... including new stuff. a bit lame, but plays it safe. * leave everything that was already in org.vfny.geoserver in that namespace, but the new classes go in org.geoserver... * go ahead and migrate the wms module to the org.geoserver package structure. I'd be +1 on the second option in 1.7.x (that is, Decoration, DecorationLayout, and the decorations package go in org.geoserver.wms) and possibly the third on trunk. > > * Decoration.findOptimalSize() requires javadocs. > * This was brought up in IRC but I assume you are going to merge this > into the core wms module? And I assume that the > DecoratedRasterMapProducer class and the png-decorated output format > will no longer be needed? The differences between DecoratedRasterMapProducer and DefaultRasterMapProducer will need to be folded in (afaik, no changes have been made on DefaultRMP since this module was created, so I'll just copy over the class and rename it.) png-decorated will go. > * I notice when defining a decorator bean the example declares it as a > non-singleton? Nothing in the interface to me would indicate that > decorators would have to hold onto any state... so a singleton would be > ok or is there some other reason? These are non-singleton because they do have some state (which I think is implied by the loadConfiguration() method in the Decoration interface.) I guess we could have a different model for handling options. Two ideas off the bat: * pass in the configuration map to each method in the Decoration interface. this would let decorations be singletons again, but also forces decorations to parse the configuration twice if their desired size depends on the configuration at all (which I imagine it will for many decorations) * pass in the configuration map to the Decoration class's constructor, possibly using reflection to allow calling default constructors when no constructor accepts a map. this would still require multiple decoration instances. > * Layouts are stored in the data directory? Are they stored under a > "layouts" directory? The code indicates yes, just might want to mention > it in the proposal. Yep, this is also documented on the wiki page linked from the GSIP. I'll update the GSIP with this and the other stuff from this thread tomorrow. -- David Winslow OpenGeo - http://opengeo.org/ ------------------------------------------------------------------------------ _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
