[josm-dev] JOSM Jumbo patch v8 (JOSM and validator enhancements)
Gabriel Ebner
ge at gabrielebner.at
Wed Jan 30 12:17:54 GMT 2008
On Tue, Jan 29, 2008 at 12:46:36PM -0800, Dave Hansen wrote:
> My basic motivation here is to have the validator detect and fix all of
> the things that I've been doing manually, especially with the TIGER
> data.
Thank you for your efforts.
> I'd love to get this merged into JOSM, but JOSM development is pretty
> slow these days, and it turns out not to be too much trouble to keep my
> patches merged up to date with the latest JOSM.
I think the following things are pretty much uncontroversial and I'll merge
them later today if nobody objects. These features are great but they were
sadly drowned in the debate over the reverse lookup changes.
> * remove unused imports
> * add Main.debug() function for messages
TODO: Add a --debug flag for that.
> * teach Way/Node merging about TIGER tags (this allows good merging at
> TIGER county borders)
> * add retries to uploads
> * give better progress in upload dialog
> * add Bounds::contains() function
> * MotorwayLinksMustTouchMotorways, don't let motorway_links exist
> unless touching a motorway (lots of these in TIGER)
> * MotorwayIntersections: motorways may only touch motorway_links
> and other motorways, except at their endpoints (fixable)
> * DuplicateSegment: for ways that re-use the same node pair
TODO: Only highlight the duplicated way segments instead of the complete way.
This should be just a matter of passing a List containing the WaySegment to
the TestError constructor.
> * Rewrote coastlines checks:
> * check for "forks" in coastline
> * ensure coastline is ordered (these are fixable now)
> * check for "ends" of coastline inside the current layer
> * Make OverLapping ways fixable
> * Take different types of ways into account in CrossingWays
> * consider TIGER tags in UntaggedNodes
=====
> * Basically rewrote DuplicatedNodes to allow multiple nodes at the same
> spot if they are different types (highway, power, railway, etc...)
DuplicateNodes is missing from plugins-10.diff
> * Empty ways (ways with 0 nodes) (fixable)
We already check that in UntaggedWay (along with one-node ways and unnamed
ways).
== Granular commands ==
> --- src/org/openstreetmap/josm/command/RemoveNodeInWayCommand.java (revision 0)
> + @Override public void undoCommand() {
> + way.addNodeNr(location, node);
> + way.modified = false;
> + }
> --- src/org/openstreetmap/josm/command/AddNodeToWayCommand.java (revision 0)
> + @Override public void undoCommand() {
> + super.undoCommand();
> + Node removed = way.removeNode(location);
> + if (removed != node)
> + Main.debug("removed wrong node");
> + way.modified = false;
> + }
> --- src/org/openstreetmap/josm/command/ReplaceNodeInWayCommand.java (revision 0)
> + @Override public void undoCommand() {
> + way.replaceNode(newNode, node, replaced_at);
> + //stem.out.println("undo ReplaceNode at: " + replaced_at);
> + way.modified = false;
> + }
> --- src/org/openstreetmap/josm/command/ReverseWayCommand.java (revision 0)
> + @Override public void undoCommand() {
> + way.reverseNodes();
> + way.modified = false;
> + }
Setting the modified flag to false will break uploads after undo. If the user
modifies a way, then reverses it, and then hits undo, then the way will not be
uploaded.
> --- src/org/openstreetmap/josm/command/ConditionalDeleteCommand.java (revision 0)
> + @Override public void fillModifiedData(Collection<OsmPrimitive> modified, Collection<OsmPrimitive> deleted, Collection<OsmPrimitive> added) {}
You'll need to implement either this or undoCommand, else undo won't work.
== Reverse lookup ==
> * uses preference: reverse-lookup=always/never/cache
> * 'always' will create the cache from the start, and keep it up to
> date always.
> * 'never' will always use the slow back-reference collector
> * 'cache' will use the slow one the first time, then the cache after
> that. (this is the default)
Good idea.
However, I still think that this your proposal is (partly) the wrong way to
go. IMHO there should be one ReverseLookup instance per DataSet, and the
bookkeeping should be done in the Commands. This would fix pretty much all of
the problems I currently see:
1) Now that ReverseLookup is static, it accumulates a lot of dead entries.
E.g. load an osm file with reverse-lookup=always and delete the layer
afterwards; ReverseLookup will still contain backreferences for all the nodes.
This can possibly be fixed by a) pruning the cache after layer manipulations,
or b) keeping the cache local to the data set (so that it can be garbage
collected).
2) Deleting a way doesn't remove it from the cache. E.g. add two nodes and a
way, then remove the way; ReverseLookup.waysUsingNode(<one of the two nodes>)
will still contain the way.
3) ChangeCommand will produce entries to ways that are not in the data set.
E.g. change the tags of a way; ReverseLookup.waysUsingNode() will contain both
the way that is in the data set and the way that is stored in
ChangeCommand.newOsm.
== Fallible commands ==
> * change Commands to be able to return success/failure, which will
> * allow SequenceCommand() to detect errors, and roll back all operations
> which were performed before that one errored out
Is there a use case for this? I have been searching the list archives and
couldn't find any. (The changes are fine, I'd just like to know what problem
they were meant to solve.)
AFAICT only RemoveNodeInWayCommand and ReplaceNodeInWayCommand can actually
fail. And I suppose that failure means that there's a bug in the calling
code. If that's the case, wouldn't it be better to fail loudly instead of
silently rolling back the current command?
Gabriel.
More information about the josm-dev
mailing list