Modify

Opened 17 months ago

Last modified 3 weeks ago

#17196 new defect

Undo changes inactive layer

Reported by: GerdP Owned by: team
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 (2)

17196.patch (2.8 KB) - added by GerdP 17 months ago.
Possible solution
17196.2.patch (5.9 KB) - added by GerdP 3 weeks ago.
POC: One instance per DataSet

Download all attachments as: .zip

Change History (8)

comment:1 Changed 17 months 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 17 months 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 17 months ago by GerdP

Attachment: 17196.patch added

Possible solution

comment:3 Changed 17 months 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 3 weeks 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 3 weeks 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 3 weeks ago by GerdP

Attachment: 17196.2.patch added

POC: One instance per DataSet

comment:6 Changed 3 weeks ago by skyper

See #6582 and #13036.

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