Ahh, the curse of the "willing to do it". Stefan, I've looked at your suggestions, and here are my thoughts:
1) I'm not sure I understand the changes well enough on this one, but if you could provide a patch using "svn diff" on the PPPFSV.java file, then I can review and give you the ok on this one. 2) You've hit a recurring problem with PPPFSV. FilterCapabilities have two 'styles'. One is the "old style" with FilterCapabilities.INTEGER_CONSTANT as the definition of the capability. The other is the "new style" with Capability.class as the definition of the capability. Your patch which includes BOTH checks looks good. Leaving one off will work for you, but break other datastores that use the new filter capabilities style exclusively. Sorry it's not a cleaner answer! For both patches, you'll need to do the following: a. Make your changes to your local subversion working copy. b. Run '$ mvn clean install to make sure all tests are still passing. c. create an "svn diff" of your patch and send it to the list for review (attaching it to the JIRA issue would be just as good, if not better) d. Port your patch FORWARD to geotools-trunk. the PPPFSV class hasn't changed much between the two branches, so probably just 'svn diff' and 'patch -p0 < your-patch-file.patch' should do it. If not, eclipse and 'compare with each other...' will do you wonders. e. Make sure that the patched geotools-trunk also is passing all tests (or at least those tests which you probably didn't break) f. Commit! If you don't have the time/interest in doing steps a-e, please don't do step f...but please *do* create patches and then I can do all 6 steps just to make sure 2.4.x and trunk get the benefit of the patches (and stay stable). If you're really ambition, before step a make your own test-case that tests exactly the bug you're fixing, and add it to the appropriate testing classes. That way this bug won't ever come back (as long as people do all 6 of the above steps!) Thanks much for finding these bugs and taking the time to report them in such detail to the list! We really appreciate it. Go Sox, --saul On Sun, 2007-10-28 at 22:59 -0700, Justin Deoliveira wrote: > Also it is always nice to have the patch in a diff format that can be > readily applied. As for the patches they look reasonable to me. I > believe Saul has become the defacto maintainer for the filter > capabilities stuff on main so you may want to get his review before > committing. > > -Justin > > Cameron Shorter wrote: > > Stefan, > > For these 2 issues you have found, I suggest you create an issue in the > > Geoserver issue tracker. > > I understand that you have fixed these issues in your branch. I suggest > > you reference the svn version you committed which makes it easier to review. > > Be specific about what you would like, and how you can help. In this > > case, ask if it is ok if you can commit this patch back to the trunk, or > > if someone else can do it for you. > > > > Geotools, > > I suggest requesting codehaus track geotools with > > http://fisheye.codehaus.org/ > > It provides lots of useful metics and web based svn diffs. > > > > Stefan Hansen wrote: > >> Dear list! > >> > >> I discovered a small problem in the function visit(GeometryFilter > >> filter) of > >> org.geotools.filter.visitor.PostPreProcessFilterSplittingVisitor.java. > >> > >> If the given filter is a FilterType.GEOMETRY_BBOX containing a BBOX that > >> lies completely outside the maxbox, the BBOX in the filter gets altered. > >> Because of this, sometimes features are returned, that are not in the > >> requested BBOX. > >> > >> Here is the code snippet of which I think causes the problem: > >> > >> if(bbox!=null){ > >> boolean changed = false; > >> double minx,miny,maxx,maxy; > >> minx = bbox.getMinX(); > >> miny = bbox.getMinY(); > >> maxx = bbox.getMaxX(); > >> maxy = bbox.getMaxY(); > >> if(minx < maxbbox.getMinX()){ > >> minx = maxbbox.getMinX(); > >> changed = true; > >> } > >> if(maxx > maxbbox.getMaxX()){ > >> maxx = maxbbox.getMaxX(); > >> changed = true; > >> } > >> if(miny < maxbbox.getMinY()){ > >> miny = maxbbox.getMinY(); > >> changed = true; > >> } > >> if(maxy > maxbbox.getMaxY()){ > >> maxy = maxbbox.getMaxY(); > >> changed = true; > >> } > >> if(changed){ > >> Envelope tmp = new > >> Envelope(minx,maxx,miny,maxy); > >> try { > >> le.setLiteral((new > >> GeometryFactory()).toGeometry(tmp)); > >> } catch (IllegalFilterException e) > >> { > >> logger.warning(e.toString()); > >> } > >> } > >> > >> And here is my solution, which is supposed to alter the BBOX only if it > >> overlaps the maxbox: > >> > >> if(bbox!=null){ > >> logger.warning("sh: GeometryFilter: > >> BBOX!=null"); > >> boolean changed = false; > >> double minx,miny,maxx,maxy; > >> minx = bbox.getMinX(); > >> miny = bbox.getMinY(); > >> maxx = bbox.getMaxX(); > >> maxy = bbox.getMaxY(); > >> if(minx < maxbbox.getMinX() && maxx > >> > maxbbox.getMinX()){ > >> minx = maxbbox.getMinX(); > >> changed = true; > >> } > >> if(maxx > maxbbox.getMaxX() && minx > >> < maxbbox.getMaxX()){ > >> maxx = maxbbox.getMaxX(); > >> changed = true; > >> } > >> if(miny < maxbbox.getMinY() && maxy > >> > maxbbox.getMinY()){ > >> miny = maxbbox.getMinY(); > >> changed = true; > >> } > >> if(maxy > maxbbox.getMaxY() && miny > >> > maxbbox.getMaxY()){ > >> maxy = maxbbox.getMaxY(); > >> changed = true; > >> } > >> if(changed){ > >> logger.warning("sh: > >> GeometryFilter: Changed true"); > >> Envelope tmp = new > >> Envelope(minx,maxx,miny,maxy); > >> try { > >> le.setLiteral((new > >> GeometryFactory()).toGeometry(tmp)); > >> } catch (IllegalFilterException e) > >> { > >> logger.warning(e.toString()); > >> } > >> } > >> > >> cheers, > >> stefan > >> > >> ------------------------------------------------------------------------- > >> This SF.net email is sponsored by: Splunk Inc. > >> Still grepping through log files to find problems? Stop. > >> Now Search log events and configuration files using AJAX and a browser. > >> Download your FREE copy of Splunk now >> http://get.splunk.com/ > >> _______________________________________________ > >> Geotools-devel mailing list > >> [email protected] > >> https://lists.sourceforge.net/lists/listinfo/geotools-devel > >> > >> > > > > > > ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
