Thanks for the feedback Andrea! Some comments inline... On 14/06/2010, at 6:18 AM, Andrea Aime wrote:
> Map -> "really bad name" (tm), we should not clash with > java own classes (yes, packages keep them apart, > but code completion gets confusing) Agreed. Solution - I am thinking of calling it MapLayers or .... MapContext (replacing the current interface). > ... and I see that in the implementation it's actually > a separate class, good (so the class diagram > in the proposal is out of synch) Class diagram was for the proposal to communicate the direction of development; nothing more. > Please don't add a layer.dispose() method, resources to > be disposed are harder to use so let's limit the usage > of dispose/close methods to cases where it's really > necessary to do so (file, streams, database connections > and so on). > Swing does not have a "dispose()" method for components > despites handling lots of listeners, so there's something > to learn there I guess (worst case we can just have the > listeners be dropped in the finalizer?). I remember what you were saying about Swing using weak references for its listeners; currently I am using the CopyOnWriteArrayList implementations. I will move the clean up code to a dispose method and see how we go. > In any case, I'm ready to be convinced that disposal > methods are really necessary, but please let's explore > also this road before adding complexity to the API: > http://www.google.it/search?q=swing+listeners+weak > > Btw, Map also holds a listener list but does not have a > dispose method (it's not like I want one, but things > should be consistent). > So if we did not have a dispose method what would be the correct corse of cleanup for a interactive application? Do we expect them to clean up their feature sources? Probably not since they can clean up their datastrores which they should already be tracking. Do we expect them to clean up after the GridCoverage readers? If so how ... You will noticed I made all the event listener code very very lazy so nothing will be fired or need to be cleaned up unless some client code decides to actually listen. > DirectLayer wise I would really love to see at least one > implementation to make sure we're not just pulling the > interface for it out of... a magician hat ;-) Agreed; I was thinking of one showing CRS bounds. > I believe a direct layer could be both a customized renderer > that does geographic data or a simple decoration over a map, > not sure a single interface can accommodate both (maybe it > does, but better to make sure). The javadocs indicate both uses cases; I am confident it does due to experience in uDig which has the same setup. > A copyright layer (decoration) and wms layer (actual data as far as the > interface is concerned) should cover it. The wms layer implementation is not working right now (on trunk even before this change). I am not sure what is wrong with the WMSLab and/or WMSLab2 tutorial code. > Oh, it is a bit odd to see addLayer and then addMapLayerListener, to keep > things consistent it would be nice to have addLayerListener instead? But that > would break the api or force into another set of wrappers. > Don't know... what do you think? Yeah I am at the turning point; I have done as much as possible without breaking any api. The moment I start breaking api it is tempting to remove the cruft. > Btw, I'm looking at the patch and there are reformats... > were they really necessary??? In general I only reformatted when tabs were being used; I try to be limited in such cases. > - TestUtilites.showRender("testLineLabeling", renderer, timout, env); > + > + TestUtilites.showRender("testLineLabeling", renderer, timout, > env ); > } > > (note, afaik the java coding convention does not ask people to add space > after ( and before ), I personally find that unecessary and distracting). Perhaps the settings we have in our repository for geotools formatting is wrong? I am trying eclipse 3.6 out and they may of messed something up. I will check. > I also see code that has just been commented out like in > FeatureSourceMapLayer, bad practice... well, unless you noted > down all the code that you've commented out and will remove > it later. That will be one of the last things I do; remember up to now is a straight refactor. I will try and make the code clean and consistent today. > MapContext wise I see the interface has been modified and > one method removed. No issue for me, but didn't you say the > old API was not being changed? I did not; the old api was deprecated - it was a setAreaOfInterest( Envenlope bounds ) method that presents a conflict with the also existing ReferencedEnvelope method; forcing people to cast each and every time they call either method in order to be specific. Jody ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
