Don't silently drop locally deleted member primitives from downloaded ways and relation

[mjulius]

JOSM is currently dropping locally deleted way nodes or relation members from downloaded ways and relations.

I believe this is dangerous because it alters the downloaded data without a really good way of restoring that and maybe even without the user noticing. Instead, this should raise a conflict or maybe a warning message with the option to undelete those nodes.

[mjulius]

Ticket #4403 has been marked as a duplicate of this ticket.

[Nakor]

Just copying more explanation from a ticket I opened 2 weeks ago and which was closed as a duplicate of this one. The conflict is actually pointed out but incorrectly silently resolved.

Upload attached file, you will get a conflict that the deleted node still exists in the DB. Choose "Prepare conflict resolution" The resolution window comes up empty Try to upload again (DO NOT actually do it)

JOSM simply deletes the node from the two ways it was in, effectively destroying data without letting the user now.

[mjulius]

Replying to Nakor:

Just copying more explanation from a ticket I opened 2 weeks ago and which was closed as a duplicate of this one. The conflict is actually pointed out but incorrectly silently resolved.

The conflict is pointed out by the API - not by JOSM.

Upload attached file, you will get a conflict that the deleted node still exists in the DB. Choose "Prepare conflict resolution" The resolution window comes up empty Try to upload again (DO NOT actually do it)

JOSM simply deletes the node from the two ways it was in, effectively destroying data without letting the user now.

And this is the issue described in this ticket.

The way it works is this:

• delete a node that is used by a way JOSM doesn't know about
  
• upload => API reports a conflict => JOSM offers to resolve that
  
• to do this JOSM loads the way from API which contains the now deleted node
  
• because your deleted node and the node downloaded from the API have the same version JOSM does not recognize this as a conflict and keeps the deleted node.
• and, because the node is deleted it is removed from the way

In short: JOSM never had a conflict in its data - it only has reported to you that the API has detected a conflict. That's why this is not a matter of conflict resolving but one of conflict detecting.

[Nakor]

Sorry about that. The message in JOSM is confusing then because it says it is going to prepare a conflict resolution.

[mjulius]

(In [2933]) fixes #4467 - Don't silently drop locally deleted member primitives from downloaded ways and relation
conflicts are created instead

[Ldp]

Seems my #3761 is the same as this?

[mjulius]

Replying to Ldp:

Seems my #3761 is the same as this?

It became the same with your last comment there. I guess we can close that one. JOSM should not modify silently downloaded data.

It's not perfect, yet. See #4476. Also. The conflict dialog should mention when there are referring objects.

[jttt]

mjulius, please check r2939. I'm not sure if it's related to this ticket, but it's reverting your code and I couldn't find your private e-mail.

[mjulius]

Thanks for actually doing that. This was a thinking error on my part.

[bastiK]

I tried, but it wasn't fixed:

• delete some nodes that are still used by a way, but this way is not known to JOSM
• upload -> "Upload failed because you tried to delete node ... which is still in use in way ..."
• click "Prepare conflict resolution"
• -> nothing, no conflicts displayed
• Ctrl-a -> error from the measurement plugin (1)

• when trying to upload again, the same api error comes. In earlier versions, JOSM downloaded the conflicting way. And the issue was gone. (For this single node.)

(Maybe connected to the recent API-reimplementation? (2))

(1)

Build-Date: 2010-02-07 18:56:33		
Revision: 2948
Is-Local-Build: true

Memory Usage: 63 MB / 297 MB (22 MB allocated, but free)
Java version: 1.6.0_15, Sun Microsystems Inc., Linux

Dataset consistency test:
[DELETED REFERENCED] {Way id=49794163 version=1 VT nodes=[{Node id=632652219 version=0 IV }, {Node id=632652215 version=0 IV }, {Node id=632652216 version=1 MVD lat=54.0969967,lon=13.3507683}, {Node id=632652211 version=0 IV }, {Node id=632652212 version=0 IV }]} refers to deleted primitive {Node id=632652216 version=1 MVD lat=54.0969967,lon=13.3507683}


Plugins: measurement,osmarender,photo_geotagging,remotecontrol,tageditor,tagging-preset-tester,terracer,validator,wmsplugin
Plugin wmsplugin Version: 19626
Plugin tageditor Version: 19587
Plugin remotecontrol Version: 19471
Plugin validator Version: 19688
Plugin osmarender Version: 19419
Plugin tagging-preset-tester Version: 19481
Plugin terracer Version: 19678
Plugin photo_geotagging Version: 19672
Plugin measurement Version: 19681

java.lang.NullPointerException
	at org.openstreetmap.josm.plugins.measurement.MeasurementLayer.calcDistance(MeasurementLayer.java:151)
	at org.openstreetmap.josm.plugins.measurement.MeasurementDialog$1.selectionChanged(MeasurementDialog.java:130)
	at org.openstreetmap.josm.data.osm.DataSet.fireSelectionChanged(DataSet.java:285)
	at org.openstreetmap.josm.data.osm.DataSet.addSelected(DataSet.java:445)
	at org.openstreetmap.josm.data.osm.DataSet.setSelected(DataSet.java:390)
	at org.openstreetmap.josm.data.osm.DataSet.setSelected(DataSet.java:403)
	at org.openstreetmap.josm.actions.SelectAllAction.actionPerformed(SelectAllAction.java:23)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1636)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2851)
	at javax.swing.KeyboardManager.fireBinding(KeyboardManager.java:267)
	at javax.swing.KeyboardManager.fireKeyboardAction(KeyboardManager.java:216)
	at javax.swing.JComponent.processKeyBindingsForAllComponents(JComponent.java:2928)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2920)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2814)
	at java.awt.Component.processEvent(Component.java:6040)
	at java.awt.Container.processEvent(Container.java:2041)
	at java.awt.Component.dispatchEventImpl(Component.java:4630)
	at java.awt.Container.dispatchEventImpl(Container.java:2099)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1848)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:704)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:969)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:841)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:668)
	at java.awt.Component.dispatchEventImpl(Component.java:4502)
	at java.awt.Container.dispatchEventImpl(Container.java:2099)
	at java.awt.Window.dispatchEventImpl(Window.java:2475)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:599)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

(2)
​http://lists.openstreetmap.org/pipermail/talk-de/2010-February/062703.html

[Ldp]

Replying to bastiK:

• when trying to upload again, the same api error comes. In earlier versions, JOSM downloaded the conflicting way. And the issue was gone. (For this single node.)

That's because earlier it silently deleted the node from the other way as well, when doing that conflict 'resolution' thing. I argued earlier (in #3761) that this is not always the desired behaviour.

As both options seem to be equally valid, but applicable to different circumstances, to me it seems that JOSM should ask

"The node {node} you deleted is still present in way {way} on the server.
[Delete from other way as well] [Leave in other way] [Download way and let me handle this]".

Where in the latter case ('let me handle this') it enters a conflict in the conflict list.

[mjulius]

This is a different issue.

The problem here is that when downloading referrers the API only returns the referrers themselves and not the other primitives the referrers refer to. In your example, JOSM downloads the ways that are referring to the deleted node, but not their other nodes. That's why JOSM does not create a conflict and why it cannot display the way. And if you select such a way (by 'Select all' for example) the Measurement plugin throws a NPE when it encounters a way with incomplete nodes.

There need to be separate tickets created for these issues.

[mjulius]

(In [2950]) when downloading referrers also download way nodes for referring ways
see #4467

[bastiK]

Nice, that's how it should work... :)

[jttt]

This is still not perfect. When my dataset contains deleted node that is referenced by way in their dataset, the result is merged dataset with way that reference deleted node. This produce incorrect rendering and might cause problems in other parts of JOSM.

This get fixed after conflict resolution, but user is not forced to do conflict resolution immediately, so for some time JOSM works with inconsistent dataset.

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

[mjulius]

Replying to jttt:

This is still not perfect. When my dataset contains deleted node that is referenced by way in their dataset, the result is merged dataset with way that reference deleted node. This produce incorrect rendering and might cause problems in other parts of JOSM.

This get fixed after conflict resolution, but user is not forced to do conflict resolution immediately, so for some time JOSM works with inconsistent dataset.

Maybe we should prevent any data modification when there is a conflict in the dataset.

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

and not produce a conflict at all and simply merge the non-deleted primitive from theirs if it is referenced? Otherwise we would need to remember somehow that the primitive was deleted.

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts. The renderer can then display those and they can be made selectable. This would also allow the renderer to easily highlight all primitives with conflicts.

That might be the best way to handle conflicts in deleted state.

[anonymous]

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts.

I think we don't have to introduce a flag.

Each OsmLayer maintains a ConflictCollection. We can ask it whether an OsmPrimitive participates in a conflict. Rendering performance could be an issue, though.

[mjulius]

Replying to anonymous:

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts.

I think we don't have to introduce a flag.

Each OsmLayer maintains a ConflictCollection. We can ask it whether an OsmPrimitive participates in a conflict. Rendering performance could be an issue, though.

Exactly. It might be a bet less so if the renderer first builds a HashSet of OsmPrimitives with conflicts. But, looking up a flag is probably still faster.

A performance penalty would be less significant if we only do that for deleted primitives.

Of course, all this only needs to be done when there are any conflicts. Therefore, this would not effect normal operation. It might serve as an incentive to resolve conflicts early. ;-)

[jttt]

Replying to mjulius:

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

and not produce a conflict at all and simply merge the non-deleted primitive from theirs if it is referenced? Otherwise we would need to remember somehow that the primitive was deleted.

No, there will still be a conflict. Special type of conflict, that will contain reference to primitive and information, that primitive is in fact deleted.

The renderer can then display those and they can be made selectable.

This problem isn't only about rendering, also other parts of JOSM shouldn't be forced to test for deleted nodes/members.

On the other hand, special rendering of primitives in conflict will be useful.

[mjulius]

Replying to jttt:

On the other hand, special rendering of primitives in conflict will be useful.

Really cool would be if JOSM somehow rendered both versions of a conflicting primitive when there was a change in location.

[jttt]

(In [3034]) Fix #4467 Don't silently drop locally deleted member primitives from downloaded ways and relation (fix the issue when deleted primitive is referenced)