On 12/06/2010, at 6:53 PM, Michael Bedward wrote: > Hi Jody, > > A few comments / queries to get started (sorry for the bitty nature of > what follows but I'm multi-mutli-tasking this weekend :)
No worries thanks for the review; I am slated to commit this weekend. I am unsure if I should commit now; and the refractor the renderer class. Or try and do both in one go. I was a bit distracted by the WMSLayer not working; I grabbed a checkout and confirmed that neither of our WMSLab examples are working right now (so it is unrelated to the this refactor. > In the Layer class, are the the methods for firing selected / > deselected events only there to prevent compilation errors in > DefaultMapContext at the moment ? I remember we talked about removing > the notion of 'selected' from layers. Correct; you will also see I want to fire a change event when the user data is changed; and if they change a "selected" entry I may consider firing the selected event? > A small typo with the dispose method name (dispoose :) Joy! The oo version of dispose I suppoose. > In the Map class, why are there separate crs and bounds fields. I > recall (I think) that we decided to hold both the CRS and bounds in a > single ReferencedEnvelope in the existing DefaultMapContext to avoid > error / confusion. That should in fact be done; I was not very aggressive in changing the responsibilities of Map. > Further on this point, I confess I'm still unsure about the merits of > Map objects storing _any_ viewport data. I lean towards that being the > responsibility of display classes. I would like the ability to share > a single Map object between two or more JMapPanes (for example) with > bounds specific to each pane. I had a look at this one - I can see the value of separation: - using the same map for rendering multiple tiles Actually the renderer already provides the separation; all the render methods provide some sort of indication of the view port area being drawn: paint(Graphics2D, Rectangle, AffineTransform) paint(Graphics2D, Rectangle, Envelope) paint(Graphics2D, Rectangle, Envelope, AffineTransform) paint(Graphics2D, Rectangle, ReferencedEnvelope) paint(Graphics2D, Rectangle, ReferencedEnvelope, AffineTransform) Personally I find the above methods confusing; and I think we should provide some assistance in determining these values. So given that there is already a separation; I think we should formalise it. I think you already have a controller of some sort for JMapPane? Could we externalise the model employed by your controller (I mostly don't want to suffer questions of people trying to reproduce the logic to call the above renderer methods themselves). > PS. for little things like dispose method name, would you prefer I > tell you, or just fix them on the branch, or provide a patch ? Just tell me; I am in the correct commit window now and I want to focus on writing next week. Enjoy your weekend and thank you for the feedback. 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 Geotools-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
