>> >> * 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. This argument could be made for other classes. GetMapProducer --> Producer, WMSRequest --> Request. I just think the making the name more descriptive of what the function the class plays is usually a good idea. Especially for public api and extensions points. > >> * 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. Fair enough, if this is going to be rolled into core it becomes a larger problem of renaming the old packages. Which I think should be done regardless. But I know when I have implemented new classes I always use the new package structure. > >> * 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. Do you mean loadOptions(Map)? The reasoning makes sense. The upside of not requiring a non-singleton is it is one less thing for the plug-in writer to do / mess up. But yeah, at the same time continually parsing the options is undesirable as well. Anyways, this should at least be specified in the classes javadoc. > >> * 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/ > >
-- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
