Hi,
I'm skimming through the proposal quickly and noting down
the things that do look odd.
Premise: the work looks sound and should clean up things
significantly. I like it, past some issues I'm going to
list in this mail.
Map -> "really bad name" (tm), we should not clash with
java own classes (yes, packages keep them apart,
but code completion gets confusing)
GridCoverageReaderLayer is a separate beast from a
CoverageReader layer, it should not provide a method
to shoot oneself in the foot like getGridCoverage().
Layers based on reader are necessary, whilst the ones
based on grid coverages only will work only in small
data sets (as you cannot ask a coverage to subset itself
or access an overview).
... 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)
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?).
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).
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 ;-)
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).
A copyright layer (decoration) and wms layer (actual data
as far as the interface is concerned) should cover it.
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?
Btw, I'm looking at the patch and there are reformats...
were they really necessary???
I don't mind code cleaning but that also makes it harder to
port back and forth patches between branches.
So in general I think they are good, but would be better to
make them when we're about to cut a new stable branch and give
up maintaining the older one.
Some of these reformats are really gratuitous and even
worsen formatting. Example:
Index:
modules/unsupported/shapefile-renderer/src/test/java/org/geotools/renderer/shape/LabelingTest.java
===================================================================
---
modules/unsupported/shapefile-renderer/src/test/java/org/geotools/renderer/shape/LabelingTest.java
(revision 35694)
+++
modules/unsupported/shapefile-renderer/src/test/java/org/geotools/renderer/shape/LabelingTest.java
(working copy)
@@ -74,7 +74,8 @@
TestUtilites.INTERACTIVE = INTERACTIVE;
env = new Envelope(env.getMinX() - boundary, env.getMaxX() +
boundary,
env.getMinY() - boundary, env.getMaxY() + boundary);
- 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).
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.
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?
Cheers
Andrea
--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.
------------------------------------------------------------------------------
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
