[patch needs rework] Undo merge and download actions

[joshdoe]

I'm just starting to look at the code for this, and I think I'm seeing why a command isn't used for the replace action; it's complicated. However I'd like to know if this is the only reason a command isn't used for this, or if there's some factor which makes it nearly impossible to implement. For anyone interested start looking in josm/trunk/src/org/openstreetmap/josm/actions/MergeSelectionAction.java​, josm/trunk/src/org/openstreetmap/josm/data/osm/visitor/MergeSourceBuildingVisitor.java​, and dive from there.

[joshdoe]

I started creating MergeCommand, but I'm not sure how best to implement it. Should I make it a SequenceCommand​ which just uses AddPrimitivesCommmand​, DeleteCommand​, and ChangeCommand​? Since DataSetMerger​ is where all the magic happens, I'd need to either have that provide an undo method, or provide a list of objects that are deleted, modified, and added. In some cases, like with OsmDataLayer.mergeFrom()​, a subclass of MergeCommand can handle undoing data source rectangle and conflict changes.

[joshdoe]

adds MergeCommand and used with MergeSelectionAction

[joshdoe]

The attached patch is my attempt at adding MergeCommand, and have used it with MergeSelectionAction. It seems to work for both undo and redo, but haven't thoroughly tested it yet, as I'd like to get feedback as to if this is a good approach (e.g. not rife with potential problems).

[joshdoe]

slight changes, and add undo for download actions

[joshdoe]

This updated patch adds a download command so any download operation can be undone. Not sure how to handle undoing of merge layers, whether to recreate layer on redo...

[stoecker]

I have not read the patch, but in principle it sounds good to me to add more functions to the commandstack.

[bastiK]

Hi, have you forgotten to include DownloadOsmCommand class in the patch?

I also like this problem to get fixed, but try to keep the additional memory footprint per primitive low.

[skyper]

I would really appreciate this. Years ago I ask for the copy layer action cause after update data I often got exceptions or way to many conflicts to solve.

To keep the memory footprint low you could use caches. Think there already exists one which is saved in order to restore after a kill. Maybe you can download into a virtual saved layer (timestamp), merge layers and only delete the saved layers on clean exit or deleting of the command stack.
I know these are only thoughts of a user but as I do not see any improvement with #4608 and #6582 it might be an easy solution.

As for sample data it might be useful to create some test file which can also be used for problems with conflicts and maybe validator. Simple files would look similar to the attachments of #7481.

Cheers

[simon04]

Please attach DownloadOsmCommand.java …

[joshdoe]

Sorry about the delay, I forgot about this. If you use Git you can pull ​my GitHub mergeundo branch. For those unable to do that, I'll attach a patch serial.

[joshdoe]

four patches

[bastiK]

A unified patch is preferred, git can easily create one for you.

​https://github.com/joshdoe/josm/compare/e408ddcee...mergeundo

[joshdoe]

git diff -U upstream/mirror origin/mergeundo > 7489_unified.diff

[joshdoe]

Try that one, I believe using patch -p1 7489_unified.diff should do the trick.

[bastiK]

I get NPE after second download:

GET http://api.openstreetmap.org/api/0.6/map?bbox=-1.4436293,52.553602399999995,-1.4384365,52.5573075
GET http://api.openstreetmap.org/api/0.6/map?bbox=-1.4416073,52.552627199999996,-1.4353395,52.5553694
java.lang.reflect.InvocationTargetException
	at java.awt.EventQueue.invokeAndWait(EventQueue.java:1045)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(PleaseWaitRunnable.java:87)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.run(PleaseWaitRunnable.java:145)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
	at java.util.concurrent.FutureTask.run(FutureTask.java:166)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
	at java.lang.Thread.run(Thread.java:679)
Caused by: java.lang.NullPointerException
	at org.openstreetmap.josm.data.osm.Node.mergeFrom(Node.java:220)
	at org.openstreetmap.josm.data.osm.DataSetMerger.mergePrimitive(DataSetMerger.java:137)
	at org.openstreetmap.josm.data.osm.DataSetMerger.merge(DataSetMerger.java:420)
	at org.openstreetmap.josm.command.MergeCommand.executeCommand(MergeCommand.java:88)
	at org.openstreetmap.josm.command.DownloadOsmCommand.executeCommand(DownloadOsmCommand.java:37)
	at org.openstreetmap.josm.data.UndoRedoHandler.addNoRedraw(UndoRedoHandler.java:36)
	at org.openstreetmap.josm.data.UndoRedoHandler.add(UndoRedoHandler.java:58)
	at org.openstreetmap.josm.actions.downloadtasks.DownloadOsmTask$DownloadTask.finish(DownloadOsmTask.java:212)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable$1.run(PleaseWaitRunnable.java:89)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:216)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:647)
	at java.awt.EventQueue.access$000(EventQueue.java:96)

[joshdoe]

A quick look indicates getCoor() on line 220 is returning null (because it's a newly instantiated object and lat/lon are NaN), so the getCoor().equals() throws the NPE. An easy fix, but I can't test it until tonight.

[joshdoe]

fix NPE when coords are null

[joshdoe]

Replying to bastiK:

I get NPE after second download:

I swapped the order of the coords for the comparison, which seems to fix this at least for your download bounds. Updated patch above.

[stoecker]

@bastiK: What's the status of this?

[bastiK]

Thanks for reminder. Here is another exception I got while reviewing the patch:

GET http://api.openstreetmap.org/api/0.6/map?bbox=7.2558717,51.1175345,7.257454,51.1183249
org.openstreetmap.josm.data.osm.DataIntegrityProblemException: Relation member must be part of the same dataset as relation(relation 99098, relation 138306)
	at org.openstreetmap.josm.data.osm.Relation.checkMembers(Relation.java:473)
	at org.openstreetmap.josm.data.osm.Relation.setDataset(Relation.java:463)
	at org.openstreetmap.josm.data.osm.DataSet.addPrimitive(DataSet.java:371)
	at org.openstreetmap.josm.data.osm.DataSetMerger.remerge(DataSetMerger.java:515)
	at org.openstreetmap.josm.command.MergeCommand.executeCommand(MergeCommand.java:133)
	at org.openstreetmap.josm.command.DownloadOsmCommand.executeCommand(DownloadOsmCommand.java:37)
	at org.openstreetmap.josm.data.UndoRedoHandler.redo(UndoRedoHandler.java:113)
	at org.openstreetmap.josm.data.UndoRedoHandler.redo(UndoRedoHandler.java:101)
	at org.openstreetmap.josm.actions.RedoAction.actionPerformed(RedoAction.java:35)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2012)
	at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2335)
	at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:404)
	at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
	at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:252)
	at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
	at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
	at java.awt.Component.processMouseEvent(Component.java:6268)
	at javax.swing.JComponent.processMouseEvent(JComponent.java:3267)
	at java.awt.Component.processEvent(Component.java:6033)
	at java.awt.Container.processEvent(Container.java:2045)
	at java.awt.Component.dispatchEventImpl(Component.java:4629)
	at java.awt.Container.dispatchEventImpl(Container.java:2103)
	at java.awt.Component.dispatchEvent(Component.java:4455)
	at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4633)
	at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4297)
	at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4227)
	at java.awt.Container.dispatchEventImpl(Container.java:2089)
	at java.awt.Window.dispatchEventImpl(Window.java:2517)
	at java.awt.Component.dispatchEvent(Component.java:4455)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:649)
	at java.awt.EventQueue.access$000(EventQueue.java:96)
	at java.awt.EventQueue$1.run(EventQueue.java:608)
	at java.awt.EventQueue$1.run(EventQueue.java:606)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:105)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:116)
	at java.awt.EventQueue$2.run(EventQueue.java:622)
	at java.awt.EventQueue$2.run(EventQueue.java:620)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:105)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:619)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:275)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:200)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:190)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:185)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:177)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:138)

(Add a new empty layer, download a region. Then undo and redo.)

[bastiK]

patch updated for [5471]

[bastiK]

From what I can tell, the patch is good in general. I hope we get it merged soon.

[stoecker]

Status?

[bastiK]

There are still bugs, as reported in comment 15.

[skyper]

Merge selection should be the easiest one, I guess.

[skyper]

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

[skyper]

See also #5846, #11333 and #19331.

[skyper]

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