David, Based on the commit logs I see you went ahead with this. A couple of things about the commit.
* Registering AttributionInfoImpl as the default implementation of the interface AttributionInfo was missed in the xstream persister. This is done to make the persisted XML more human readable. * The AttributionInfo interface has some minor inconsistencies with regard to the rest of the interfaces: - All the methods on the interface have the public modifier which is redundant - There are no javadocs on any of the methods, i realize they are simple getters and setters, but a simple description of what the property means should be there - getLogoUrl() --> getLogoURL(). This is just a convention I have been using for url and uri, using uppercase. I know this is minor stuff and probably a bit too anal but a lot of time and effort have gone into keeping the catalog and configuration interfaces clean in terms of javadocs and consistency. I would appreciate if we could keep that up. -Justin Gabriel Roldan wrote: > Justin Deoliveira wrote: >> David Winslow wrote: >>> On Wed, 2009-06-24 at 10:03 +0800, Justin Deoliveira wrote: >>>> All in all patch looks good David. And thanks for including test >>>> cases with it :). >>>> >>>> One question I noted on the issue. Is ResourceInfo.getTitle() not >>>> suitable for the title? Or is it a different intention? >>> The Attribution data describes the data provider, not the dataset >>> itself, so ResourceInfo.getTitle() is indeed different from the >>> Attribution Title. >> Fair enough. >>>> Also, just a point of discussion (not a issue with the patch), I >>>> wonder if we should break out an actual class for this type of >>>> metadata? >>> I would be +1 on this. I should note that when I tried to store an >>> AttributionInfo object in the metadata map, it was persisted using just >>> AttributionInfo.toString(). Is this the intended behavior? (and if so, >>> does this mean I need to implement a GeoTools Converter to store custom >>> objects in the metadata map? This page on the wiki says anything >>> Serializable is okay: >>> http://geoserver.org/display/GEOS/Adding+a+Configuration+Option-GS2 ) >>> >> Yeah... this is a issue. The metadata map says it can store anything >> serializable, but the xtream persister assumes it is just a string. To >> the persister should be patched. Do you want to open a seperate jira >> issue for this? >> >> I would also be +1 for a AttributionInfo class, and adding >> LayerInfo.getAttribution(). What do others think? > seems like the cleaner path to me, so +1 > > Gabriel >>> -- >>> David Winslow >>> OpenGeo - http://opengeo.org/ >>> >>>> David Winslow wrote: >>>>> Hi all. I've done some work on adding support for the optional >>>>> Attribution field in the WMS capabilities document. I'd like a review >>>>> on the patch attached to http://jira.codehaus.org/browse/GEOS-3181 . >>>>> The patch uses resource metadata to store title, link, and logo image >>>>> info for attribution on each layer, and provides a panel on the >>>>> Publishing tab for configuring them. If a layer has these properties, >>>>> then they are encoded during a capabilities request (since each >>>>> field is >>>>> optional, my code will render a partial Attribution field if some >>>>> of the >>>>> information is missing, eg, just a title and link if no logo is >>>>> provided.) >>>>> >>>>> Thanks. >>>>> >>>>> -- >>>>> David Winslow OpenGeo - http://opengeo.org/ >>>>> >>>>> >>>>> ------------------------------------------------------------------------------ >>>>> >>>>> >>>>> Are you an open source citizen? Join us for the Open Source Bridge >>>>> conference! >>>>> Portland, OR, June 17-19. Two days of sessions, one day of >>>>> unconference: $250. >>>>> Need another reason to go? 24-hour hacker lounge. Register today! >>>>> http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org >>>>> >>>>> >>>>> _______________________________________________ >>>>> Geoserver-devel mailing list >>>>> [email protected] >>>>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel >> >> > > -- 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
