Opened 15 years ago
Closed 15 years ago
#4083 closed defect (fixed)
[Patch] Exception when trying to copy relation with incomplete node
Reported by: | anonymous | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Core | Version: | latest |
Keywords: | Cc: |
Description
I use the coby all
Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2009-11-30 13:18:06
Last Changed Author: stoecker
Revision: 2554
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2009-11-30 13:48:36 +0100 (Mon, 30 Nov 2009)
Last Changed Rev: 2554
Memory Usage: 35 MB / 508 MB (7 MB allocated, but free)
Java version: 1.6.0_17
Dataset consistency test:
No problems found
Plugins: AgPifoJ,openstreetbugs,remotecontrol,validator,wmsplugin
Plugin AgPifoJ Version: 18593
Plugin openstreetbugs Version: 18595
Plugin remotecontrol Version: 18678
Plugin validator Version: 18848
Plugin wmsplugin Version: 18762
java.lang.NullPointerException
at org.openstreetmap.josm.data.coor.CachedLatLon.setCoor(CachedLatLon.java:27)
at org.openstreetmap.josm.data.osm.NodeData.setCoor(NodeData.java:35)
at org.openstreetmap.josm.data.osm.Node.save(Node.java:138)
at org.openstreetmap.josm.data.osm.PrimitiveDeepCopy$1.visit(PrimitiveDeepCopy.java:46)
at org.openstreetmap.josm.data.osm.Node.visit(Node.java:103)
at org.openstreetmap.josm.data.osm.PrimitiveDeepCopy$1.visit(PrimitiveDeepCopy.java:63)
at org.openstreetmap.josm.data.osm.Relation.visit(Relation.java:133)
at org.openstreetmap.josm.data.osm.PrimitiveDeepCopy$1.visitAll(PrimitiveDeepCopy.java:70)
at org.openstreetmap.josm.data.osm.PrimitiveDeepCopy.makeCopy(PrimitiveDeepCopy.java:40)
at org.openstreetmap.josm.actions.CopyAction.actionPerformed(CopyAction.java:38)
at javax.swing.AbstractButton.fireActionPerformed(Unknown Source)
at javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source)
at javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source)
at javax.swing.DefaultButtonModel.setPressed(Unknown Source)
at javax.swing.AbstractButton.doClick(Unknown Source)
at javax.swing.plaf.basic.BasicMenuItemUI.doClick(Unknown Source)
at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(Unknown Source)
at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
at java.awt.Component.processMouseEvent(Unknown Source)
at javax.swing.JComponent.processMouseEvent(Unknown Source)
at java.awt.Component.processEvent(Unknown Source)
at java.awt.Container.processEvent(Unknown Source)
at java.awt.Component.dispatchEventImpl(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
at java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
at java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Window.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.EventQueue.dispatchEvent(Unknown Source)
at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.run(Unknown Source)
Attachments (1)
Change History (16)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
Summary: | Problem with copy all → [Patch] Exception when trying to copy relation with incomplete node |
---|
comment:3 by , 15 years ago
Version: | → latest |
---|
comment:4 by , 15 years ago
Lots of isIncomplete() calls actually refer to isUsable(). Could you please use this instead of isIncomplete(), as you do now.
comment:5 by , 15 years ago
I don't completely understand what you are asking me to do. Do you mean I should go through all the isIncomplete() calls and check whether they really should be calls to isUsable()? I thought that would be even more outside of the scope of this patch. But, if you insist ...
isUsable() currently checks for isDeleted(), isIncomplete(), and isDisabled(). Shouldn't it also check for isVisible()?
comment:6 by , 15 years ago
The (!isDeleted() && !isIncomplete()) in most places are isUsable() in reality. These should be fixed. When in doubt, leave isIncomplete() in code :-)
Also other common combinations of flags should be a isWhatever() and the combination moved into OsmPrimitive. I.E. The flags should not be exposed into outside world as much as they are now. The current situation makes it very hard to fix code, whenever an additional flags needs to be tested.
comment:7 by , 15 years ago
Seems Jiri did do incomplete changes independently as well. And he also did not use proper isUsable(). So even if the major part of your patch is already checked in, the points I said are still valid and it would be nice, when they could be fixed.
comment:8 by , 15 years ago
OK, that reduces my patch to the essential for this ticket. It is updated and much shorter now.
comment:9 by , 15 years ago
I've changed OsmPrimitive.incomplete to OsmPrimitive.isIncomplete because I want to be able to fire Dataset.primitivesAdded event when primitive is downloaded. Another thing is that I want to get rid of code that sets Way.incomplete to true if one of the nodes is incomplete. That should be done automatically so I need to know when incomplete is set on Node.
Replacing !incomplete && !isDeleted() with isUsable() is another thing that should be also done, but it doesn't matter if incomplete or isIncomplete is being replaced.
comment:10 by , 15 years ago
incomplete is still not handled correctly in the patch. If there is relation with incomplete members then copy of incomplete member will be made - it's new primitive, so id < 0 but it's also incomplete. That will probably break uploading. And even if uploading worked, the relation will point to empty primitive, because reference to existing primitive is lost.
comment:11 by , 15 years ago
Maybe incomplete primitives should not be copied at all. If there is incomplete primitive added either directly or because it's referenced by Way or Relation dialog should be shown allowing to either download the primitive or remove it from selection (including all primitives that reference incomplete primitive). That should be part of CopyAction, PasteAction will get only complete primitives.
follow-up: 13 comment:12 by , 15 years ago
Well, I actually did not think about what happens after the relation with incomplete members has been copied.
What is supposed to happen if one copies a relation? Should all members really be copied? This all depends on the context when pasting. When pasting it into a new layer I would expect to get an exact copy of all the data. This would be equivalent to merge selection. Then, copying incomplete members would make sense.
When pasting into the same layer it might make more sense to just copy the relation itself.
Anyway, at the time of copy I would just make an exact copy of all selected and referenced data, probably with a flag that indicates whether a primitive belonged to the selection or not. Then, at the time of paste the user can bi given the option to do different things with the data.
follow-up: 14 comment:13 by , 15 years ago
Replying to mjulius:
What is supposed to happen if one copies a relation? Should all members really be copied? This all depends on the context when pasting. When pasting it into a new layer I would expect to get an exact copy of all the data. This would be equivalent to merge selection. Then, copying incomplete members would make sense.
I think we don't need to duplicate MergeSelectionAction in PasteAction, paste should do the same thing independenly of layer.
When pasting into the same layer it might make more sense to just copy the relation itself.
Usually. But for example for multipolygon I would expect deep copy to be made.
Anyway, at the time of copy I would just make an exact copy of all selected and referenced data, probably with a flag that indicates whether a primitive belonged to the selection or not. Then, at the time of paste the user can bi given the option to do different things with the data.
Ok, that's probably better. PasteAction will have to for every incomplete primitive check dataset to make sure primitive is still incomplete and for really incomplete primitives show dialog asking what to do (skip/download).
comment:14 by , 15 years ago
Replying to jttt:
Replying to mjulius:
Anyway, at the time of copy I would just make an exact copy of all selected and referenced data, probably with a flag that indicates whether a primitive belonged to the selection or not. Then, at the time of paste the user can bi given the option to do different things with the data.
Ok, that's probably better. PasteAction will have to for every incomplete primitive check dataset to make sure primitive is still incomplete and for really incomplete primitives show dialog asking what to do (skip/download).
I think that's the way to go. My patch above should then probably be applied.
by , 15 years ago
Attachment: | CopyIncompleteNodesException.patch added |
---|
comment:15 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This exception comes up when one tries to copy a relation which has an incomplete node as member. JOSM then wants to store null as coordinates.
The attached patch fixes a few things:
Of course, the last point wasn't strictly necessary. I only realized during the process how often this is used. Unfortunately, this has turned my little patch into a monster. If that is not acceptable I will take that part out.
On the upside, it will save a byte (I guess, don't know how java stores a boolean internally) of memory per primitive loaded.