[RFC] [Patch] Undo/Redo may change data in inactive layer

[GerdP]

What steps will reproduce the problem?

1. Create new data layer and add some objects
2. Create 2nd data layer and pan so that the data in the other layer is no longer visible
3. create way with two nodes
4. Press Ctrl+Z (Undo) tree or more times

What is the expected result?

Only the way in the current (newer) data layer should be undone

What happens instead?

Changes in the inactive layer are also undone. This is probably never wanted.

Please provide any additional information below. Attach a screenshot if possible.

I am not sure if this is intended since we have only one Undo/Redo tree instance. I'd prefer to have a separate Undo/Redo tree for each layer, else we should not allow to undo commands for a dataset that is not active.

Build-Date:2019-01-11 07:22:54
Revision:14672
Is-Local-Build:true

Identification: JOSM/1.5 (14672 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 456 MB / 1820 MB (68 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:53371, -ea, -Dfile.encoding=UTF-8]
Program arguments: [--debug]

Plugins:
+ OpeningHoursEditor (34535)
+ apache-commons (34506)
+ buildings_tools (34807)
+ download_along (34503)
+ ejml (34389)
+ geotools (34513)
+ jaxb (34506)
+ jts (34524)
+ o5m (34405)
+ opendata (34805)
+ pbf (34576)
+ poly (34546)
+ reltoolbox (34788)
+ reverter (34552)
+ utilsplugin2 (34793)

Last errors/warnings:
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet

[stoecker]

What about a warning "Undoing/Redoing changes for inactive layer. Do you want to continue? Yes, No, This session, Always"

[GerdP]

Replying to stoecker:

What about a warning "Undoing/Redoing changes for inactive layer. Do you want to continue? Yes, No, This session, Always"

Either that or maybe a popup message that would always appear?

[GerdP]

Possible solution

[GerdP]

The attached patch refuses to do undo/redo changes inactive layer. After upload I noticed that I mixed warning/info level, I guess it should always be warning.
Still, this is not more than a work around. I think it would be much cleaner to have a single UndoRedoHandler for each OsmDataLayer instance. Even the code for UndoRedoHandler.undo() seems to expect that all commands are for a single OsmDataset as it uses the ds.beginUpdate() ... ds.endUpdate() sequence.

[GerdP]

When switching to a different edit layer we should update the CommandStack so that only the commands which are related to the current edit layer are visible. This requires some changes but should improve stability.
see [o35450:35453]
I see no reason to have the fields UndoRedoHandler.commands or UndoRedoHandler.redoCommands public. I plan to make them private:

src/org/openstreetmap/josm/data/UndoRedoHandler.java

@@ -22 +22 @@
 public final class UndoRedoHandler {
 
     /**
-     * All commands that were made on the dataset. Don't write from outside!
+     * All commands that were made on the dataset.
      *
-     * @see #getLastCommand()
-     * @see #getUndoCommands()
      */
-    public final LinkedList<Command> commands = new LinkedList<>();
+    private final LinkedList<Command> commands = new LinkedList<>();
 
     /**
      * The stack for redoing commands
-
-     * @see #getRedoCommands()
      */
-    public final LinkedList<Command> redoCommands = new LinkedList<>();
+    private final LinkedList<Command> redoCommands = new LinkedList<>();
 
     private final LinkedList<CommandQueueListener> listenerCommands = new LinkedList<>();
     private final LinkedList<CommandQueuePreciseListener> preciseListenerCommands = new LinkedList<>();

Next step is to decide if we need distinct instances of UndoRedoHandler for each EditLayer or if we can filter the existing lists.

[GerdP]

I wonder why UndoRedoHandler is a singleton. Wouldn't it be much better to have one instance per DataSet?
Or would it be too confusing for existing users when CommandStackDialog shows different/empty undo/redo trees when layers are changed?

[GerdP]

POC: One instance per DataSet

[skyper]

See #6582 and #13036.

[GerdP]

If we use one undo/redo stack for each data layer what should be displayed in the Command Stack window when the active layer is not a data layer? I wonder if it makes sense that I can activate an image layer like Bing aerial image. Is that really intended?

[GerdP]

updated patch, shows empty command stack when active layer is not an edit layer

[Don-vip]

Milestone renamed

[GerdP]

In 17347/josm:

fix #17196: Undo/Redo may change data in inactive layer

• use separate stacks for each edit layer
• show empty undo/redo stack when active layer isn't an edit layer

[skyper]

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

[GerdP]

Doesn't work when there are two layers with changes and one layer is closed. The undo stack for the other layer disappears.

[GerdP]

In 17378/josm:

fix #17196 Undo/Redo may change data in inactive layer

• fix case when there are two layers with changes and one layer is closed. The undo stack for the other layer disappeared.

[GerdP]

In 17379/josm:

see #17196 Undo/Redo may change data in inactive layer

• fix sonar issue: Make UndoRedoHandler.clean(Dataset dataset) static

[skyper]

See my comment 5 on #20041 for a possible problem.

[skyper]

See #20213 for a remaining problem.

[GerdP]

This patch cannot work with the concept of Relation Editor as it might willingly(?) change an inactive layer. Plugins might as well implement non-modal dialogs like this.

[skyper]

See #23987 which suggest to at least show a warning if undoing modifies an inactive layer.