Modify

Opened 2 years ago

Last modified 7 weeks ago

#17196 reopened defect

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

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone:
Component: Core Version:
Keywords: template_report Cc: simon04

Description

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

Attachments (3)

17196.patch (2.8 KB) - added by GerdP 2 years ago.
Possible solution
17196.2.patch (5.9 KB) - added by GerdP 9 months ago.
POC: One instance per DataSet
17196.3.patch (5.4 KB) - added by GerdP 6 months ago.
updated patch, shows empty command stack when active layer is not an edit layer

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years ago by stoecker

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

comment:2 in reply to:  1 Changed 2 years ago by 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?

Changed 2 years ago by GerdP

Attachment: 17196.patch added

Possible solution

comment:3 Changed 2 years ago by 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.

comment:4 Changed 9 months ago by 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

     
    2222public final class UndoRedoHandler {
    2323
    2424    /**
    25      * All commands that were made on the dataset. Don't write from outside!
     25     * All commands that were made on the dataset.
    2626     *
    27      * @see #getLastCommand()
    28      * @see #getUndoCommands()
    2927     */
    30     public final LinkedList<Command> commands = new LinkedList<>();
     28    private final LinkedList<Command> commands = new LinkedList<>();
    3129
    3230    /**
    3331     * The stack for redoing commands
    34 
    35      * @see #getRedoCommands()
    3632     */
    37     public final LinkedList<Command> redoCommands = new LinkedList<>();
     33    private final LinkedList<Command> redoCommands = new LinkedList<>();
    3834
    3935    private final LinkedList<CommandQueueListener> listenerCommands = new LinkedList<>();
    4036    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.

comment:5 Changed 9 months ago by GerdP

Cc: simon04 added

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?

Changed 9 months ago by GerdP

Attachment: 17196.2.patch added

POC: One instance per DataSet

comment:6 Changed 9 months ago by skyper

See #6582 and #13036.

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

Changed 6 months ago by GerdP

Attachment: 17196.3.patch added

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

comment:8 Changed 3 months ago by GerdP

Milestone: 20.11
Owner: changed from team to GerdP
Status: newassigned
Summary: Undo changes inactive layer[RFC] [Patch] Undo/Redo may change data in inactive layer

comment:9 Changed 2 months ago by Don-vip

Milestone: 20.1120.12

Milestone renamed

comment:10 Changed 2 months ago by GerdP

Resolution: fixed
Status: assignedclosed

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

comment:11 Changed 2 months ago by skyper

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

comment:12 Changed 2 months ago by GerdP

Resolution: fixed
Status: closedreopened

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

comment:13 Changed 2 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

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.

comment:14 Changed 2 months ago by GerdP

In 17379/josm:

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

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

comment:15 Changed 8 weeks ago by skyper

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

comment:16 Changed 7 weeks ago by skyper

See #20213 for a remaining problem.

comment:17 Changed 7 weeks ago by GerdP

Milestone: 20.12
Resolution: fixed
Status: closedreopened

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.

Modify Ticket

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

Add Comment


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

 
Note: See TracTickets for help on using tickets.