Modify

Opened 6 years ago

Last modified 3 years ago

#7489 new enhancement

[patch needs rework] Undo merge and download actions

Reported by: joshdoe Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: redo undo merge download command Cc:

Description

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.

Attachments (6)

add_merge_command.patch (17.1 KB) - added by joshdoe 6 years ago.
adds MergeCommand and used with MergeSelectionAction
add_merge_command2.patch (25.9 KB) - added by joshdoe 6 years ago.
slight changes, and add undo for download actions
7489_patches.zip (10.8 KB) - added by joshdoe 6 years ago.
four patches
7489_unified.diff (34.7 KB) - added by joshdoe 6 years ago.
git diff -U upstream/mirror origin/mergeundo > 7489_unified.diff
7489_unified2.diff (34.7 KB) - added by joshdoe 6 years ago.
fix NPE when coords are null
7489_unified2_updated.diff (33.1 KB) - added by bastiK 6 years ago.
patch updated for [5471]

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by 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.

Changed 6 years ago by joshdoe

Attachment: add_merge_command.patch added

adds MergeCommand and used with MergeSelectionAction

comment:2 Changed 6 years ago by joshdoe

Summary: Undo/redo merge selection action[patch] Undo/redo merge selection action

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).

Changed 6 years ago by joshdoe

Attachment: add_merge_command2.patch added

slight changes, and add undo for download actions

comment:3 Changed 6 years ago by joshdoe

Keywords: download added
Summary: [patch] Undo/redo merge selection action[patch] Undo merge and download actions

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...

Before landing this, it would be good to run it through some tests. Any ideas on how best to do this?

Last edited 6 years ago by joshdoe (previous) (diff)

comment:4 Changed 6 years ago by stoecker

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

comment:5 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by 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

comment:7 Changed 6 years ago by simon04

Owner: changed from team to joshdoe
Status: newneedinfo

Please attach DownloadOsmCommand.java

comment:8 Changed 6 years ago by 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.

Changed 6 years ago by joshdoe

Attachment: 7489_patches.zip added

four patches

comment:9 Changed 6 years ago by bastiK

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

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

Changed 6 years ago by joshdoe

Attachment: 7489_unified.diff added

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

comment:10 Changed 6 years ago by joshdoe

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

comment:11 Changed 6 years ago by 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)

comment:12 Changed 6 years ago by 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.

Changed 6 years ago by joshdoe

Attachment: 7489_unified2.diff added

fix NPE when coords are null

comment:13 in reply to:  11 Changed 6 years ago by joshdoe

Owner: changed from joshdoe to team
Status: needinfonew

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.

comment:14 Changed 6 years ago by stoecker

@bastiK: What's the status of this?

comment:15 Changed 6 years ago by 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.)

Changed 6 years ago by bastiK

Attachment: 7489_unified2_updated.diff added

patch updated for [5471]

comment:16 Changed 6 years ago by bastiK

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

comment:17 Changed 6 years ago by stoecker

Status?

comment:18 Changed 6 years ago by bastiK

There are still bugs, as reported in comment 15.

comment:19 Changed 3 years ago by stoecker

Summary: [patch] Undo merge and download actions[patch neeeds rework] Undo merge and download actions

comment:20 Changed 3 years ago by skyper

Summary: [patch neeeds rework] Undo merge and download actions[patch needs rework] Undo merge and download actions

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to joshdoe
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.