More validation for commands

[michael2402]

Currently, the command class does only few validation of the layer the command works for.

I suggest:

• Enforce that layer may not be null. Make command construction fail if it is.
• Always set the layer/dataset the command is working on.
• Check that all primitives of that command belong to the layer layer (for add/move/delete/... commands).
  • if plugins want to change this, we can provide them with the means to extend the commands. They have to take care of the problems that occur then, e.g. handle data integrity.

[simon04]

Milestone renamed

[Don-vip]

Milestone renamed

[michael2402]

In 12348/josm:

See #13036: Add more consistency checks to move / delete command.

[michael2402]

In 12350/josm:

See #13036: Add data set tests to SelectCommand

[Don-vip]

Replying to michael2402:

• Enforce that layer may not be null. Make command construction fail if it is.

If I remember correctly I have put a great effort to make sure a null layer is allowed, in order to work only with a dataset. Plugins rely on it.

[Don-vip]

If dataset is forced now, all command constructors must be checked/updated. I have fixed in r12386 the problem that caused opendata plugin unit test to fail, but I think all constructors from DeleteCommand that don't take a Layer or a DataSet as parameter will cause errors when executed.

[michael2402]

Replying to Don-vip:

If dataset is forced now, all command constructors must be checked/updated. I have fixed in r12386 the problem that caused opendata plugin unit test to fail, but I think all constructors from DeleteCommand that don't take a Layer or a DataSet as parameter will cause errors when executed.

Not necessarily. All commands that modify the edit data set (the one while they are created) are fine. Undo/Redo respects the layer that was used during the command construction.

The problem with unit tests is that they do not correctly set the edit layer during the test - this is why they fail. If we get errors in plugins, the plugin is using commands to modify a layer that is not the edit layer.

... but I agree: We should try to always add dataset information to our utility functions - it will make them more universal and allow them to be used on a data set that is not in any layer. I fixed this for the right/left hand traffic computations.

[Don-vip]

is this ticket fixed?

[michael2402]

I have not looked at all commands yet.

[Don-vip]

In the course of #15182 it appears problematic to have commands depending on layers, because layers are GUI stuff, and commands should not depend on GUI. They should only affect a DataSet, and I'd like to get rid of all layer references. This way we could move forward to have all commands, plus UndoRedoHandler, to not depend on GUI.

[michael2402]

+1

Should not be hard, since most of them only depend on the DataSet internally. We only need to change most constructor calls.

We should also enforce that the data set needs to be set with the constructor.

You should never use the current edit layer during command execution/undo. I already fixed that, but it is not documented yet why this should not be the case.

[Don-vip]

Replying to michael2402:

Should not be hard

Well it's quite a big patch, see attachment. Not tested a lot, and not completely finished, I have these warnings I don't know how to fix yet. Any idea?

    [javac] org\openstreetmap\josm\data\UndoRedoHandler.java:100: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]         c.invalidateAffectedLayers();
    [javac]          ^
    [javac] org\openstreetmap\josm\data\UndoRedoHandler.java:133: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]                 c.invalidateAffectedLayers();
    [javac]                  ^
    [javac] org\openstreetmap\josm\data\UndoRedoHandler.java:169: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]             c.invalidateAffectedLayers();
    [javac]              ^
    [javac] org\openstreetmap\josm\command\SequenceCommand.java:139: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]     public void invalidateAffectedLayers() {
    [javac]                 ^
    [javac] org\openstreetmap\josm\command\SequenceCommand.java:140: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]         super.invalidateAffectedLayers();
    [javac]              ^
    [javac] org\openstreetmap\josm\command\SequenceCommand.java:142: warning: [deprecation] invalidateAffectedLayers() in Command has been deprecated
    [javac]             c.invalidateAffectedLayers();

[Don-vip]

Warnings fixed. I tested it quickly and it seems ready for integration in next release. Feedback welcome :)

[michael2402]

(1)

I'm no fan of invalidateDataSet(). We don't want to tell components when they need to repaint data sets.

We have a change listener on the data set. We should fix those and see that they fire every time there was a change. Comonents can listen to those changes and repaint accordingly.

(2)
JoinAreasAction/SplitWayAction: There was a reason for the layer there. I don't exactly remember what, but I tried to remove it some time ago, too. Have you tested if re-creating the left/right traffic works?

(3)
While we are on it: Should we switch to a per-dataset command queue? It would make many things easier and Undo/Redo does not need to depend on the main layer manager any more. We should deprecate the Command() constructor for the same reason.

We can simply bind the Undo/Redo manager to each layer. So you can do:

dataSet.getCommandManager().doCommand(new AddPrimitivesCommand(...))

Or we can simplify this for the caller:

addCommand.doCommand()
-> call dataSet.getCommandManager().doCommand(this: AddPrimitivesCommand)
-> call addCommand.run() throws CommandFailedException (good to make users aware that this method should not be called directly)

It might be a plus for users as well if we don't change things in hidden layers on undo but only in the active, visible layer.

While we are on it: Make that undo/redo queue implement an interface (e.g. UndoRedo). then add a UndoRedo getUndoRedo() to the Layer class. It will make the code much more universal and we can add undo/redo to other layers later without changing the UI code.

(4)
Dataset names are a nice-to-have feature. So even if we change to DS command queues, we can keep them to make debugging easier.

[bastiK]

Replying to michael2402:

(1)

I'm no fan of invalidateDataSet(). We don't want to tell components when they need to repaint data sets.

We have a change listener on the data set. We should fix those and see that they fire every time there was a change. Comonents can listen to those changes and repaint accordingly.

Yes, any change to the dataset should trigger OsmDataLayer.processDatasetEvent, which invalidates the layer. So without testing, I would think the invalidation wasn't really necessary in the first place.

(2)
JoinAreasAction/SplitWayAction: There was a reason for the layer there. I don't exactly remember what, but I tried to remove it some time ago, too. Have you tested if re-creating the left/right traffic works?

If you don't remember, then there is no choice but to try and find out ... ;)

(3)
While we are on it: Should we switch to a per-dataset command queue?

Alright, but this is not really the scope of the patch.

[Don-vip]

(1)

Yes, any change to the dataset should trigger OsmDataLayer.processDatasetEvent, which invalidates the layer. So without testing, I would think the invalidation wasn't really necessary in the first place.

You're right, I don't see any problem after removing it :)

(2)
JoinAreasAction/SplitWayAction: There was a reason for the layer there. I don't exactly remember what, but I tried to remove it some time ago, too. Have you tested if re-creating the left/right traffic works?

If you don't remember, then there is no choice but to try and find out ... ;)

I tried and didn't see anything suspicious. the left/right traffic file is correctly recreated after cache cleanup, and the roundabout validator test works.

(3)
While we are on it: Should we switch to a per-dataset command queue?

Alright, but this is not really the scope of the patch.

I'll think about it when I'll work on UndoRedoHandler.

[Don-vip]

In 12718/josm:

see #13036 - see #15229 - see #15182 - make Commands depends only on a DataSet, not a Layer. This removes a lot of GUI dependencies

[Don-vip]

In 12721/josm:

see #13036 - Do not call default Command() constructor from SequenceCommand(), as it requires Main.main to be set, which is not the case in Taginfo groovy script

[Don-vip]

In 12726/josm:

see #13036 - deprecate Command() default constructor, fix unit tests and java warnings

[Don-vip]

In 12727/josm:

see #13036 - forgot two unit tests

[Don-vip]

In 12728/josm:

see #13036 - fix more unit tests

[Don-vip]

In 12730/josm:

see #13036 - fix TagInfo integration test

[Don-vip]

In 12731/josm:

see #13036 - execute commands in CreateMultipolygonActionTest when relation is reused. Does not fix one of the tests, don't know why

[Don-vip]

In 12740/josm:

see #13036 - fix CreateMultipolygonActionTest - tags from outer ways are moved to the multipolygon relation, test has not been designed with this in mind

[Don-vip]

In 12749/josm:

see #15229 - see #15182 - see #13036 - remove GUI stuff from Command and DeleteCommand

[Don-vip]

In 12751/josm:

see #15229 - see #15182 - see #13036 - update unit tests and groovy script

[Don-vip]

In 12752/josm:

see #15229 - see #15182 - see #13036 - fix groovy script

[Don-vip]

In 12754/josm:

see #15229 - see #15182 - see #13036 - update unit tests

[Don-vip]

In 12757/josm:

see #15229 - see #15182 - see #13036 - update fixture for performance tests

[Don-vip]

In 12759/josm:

fix #15252 - see #13036 - MoveCommand created without data set

[Don-vip]

In 12760/josm:

see #15229 - see #15182 - see #13036 - remove more GUI stuff from DeleteCommand

[Don-vip]

In 12761/josm:

see #15229 - see #15182 - see #13036 - fix stupid mistake

[Don-vip]

In 12762/josm:

fix #15254 - see #13036 - do not create empty SequenceCommand

[Don-vip]

In 12763/josm:

see #15229 - see #15182 - see #13036 - remove last GUI stuff from DeleteCommand

[Don-vip]

In 12769/josm:

fix #15262 - see #13036 - fix regression in ChangePropertyCommand + optimize commands creation when pasting tags

[Don-vip]

In 12785/josm:

fix #15268 - see #13036 - do not create empty MoveCommand in SelectAction

[Don-vip]

In 12828/josm:

see #15229 - see #15182 - see #13036 - convert SplitWayAction to SplitWayCommand to remove dependence of DeleteCommand on actions package

[Don-vip]

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

[skyper]

Replying to michael2402:

(3)
While we are on it: Should we switch to a per-dataset command queue?

See #6582.