Modify

Opened 14 years ago

Closed 14 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)

CopyIncompleteNodesException.patch (4.9 KB ) - added by mjulius 14 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by mjulius, 14 years ago

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:

  • it doesn't try to copy coordinates from incomplete nodes
  • it copies the incomplete status in PrimitiveData when copying nodes
  • it changes the implementation of the incomplete flag to be in line with the other flags (deleted, modified, visible, ...). There are now OsmPrimitive.setIncomplete() and OsmPrimitive.isIncomplete() instead of OsmPrimitive.incomplete

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.

comment:2 by mjulius, 14 years ago

Summary: Problem with copy all[Patch] Exception when trying to copy relation with incomplete node

comment:3 by mjulius, 14 years ago

Version: latest

comment:4 by stoecker, 14 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 anonymous, 14 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 stoecker, 14 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 stoecker, 14 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 mjulius, 14 years ago

OK, that reduces my patch to the essential for this ticket. It is updated and much shorter now.

comment:9 by jttt, 14 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 jttt, 14 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 jttt, 14 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.

comment:12 by mjulius, 14 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.

in reply to:  12 ; comment:13 by jttt, 14 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).

in reply to:  13 comment:14 by mjulius, 14 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 mjulius, 14 years ago

comment:15 by mjulius, 14 years ago

Resolution: fixed
Status: newclosed

(In [2612]) Fixes #4083 - exception when copying relation with incomplete node
Fixes #4141 - relax conflict detection for coordinates; API only returns 7 decimals

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.