Modify

Opened 17 months ago

Last modified 3 weeks ago

#13036 new enhancement

More validation for commands

Reported by: michael2402 Owned by: team
Priority: normal Milestone: 17.11
Component: Core Version:
Keywords: gsoc-core command Cc: Don-vip, bastiK, stoecker

Description (last modified by 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.

Attachments (2)

commands-layer.patch (70.3 KB) - added by Don-vip 3 months ago.
commands-layerv2.patch (73.8 KB) - added by Don-vip 3 months ago.

Download all attachments as: .zip

Change History (50)

comment:1 Changed 16 months ago by Don-vip

Milestone: 16.0716.08

comment:2 Changed 15 months ago by Don-vip

Milestone: 16.0816.09

comment:3 Changed 14 months ago by Don-vip

Milestone: 16.0916.10

comment:4 Changed 14 months ago by simon04

Milestone: 16.1016.11

Milestone renamed

comment:5 Changed 11 months ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

comment:6 Changed 11 months ago by Don-vip

Milestone: 16.12

comment:7 Changed 5 months ago by michael2402

In 12348/josm:

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

comment:8 Changed 5 months ago by michael2402

In 12350/josm:

See #13036: Add data set tests to SelectCommand

comment:9 in reply to:  description Changed 5 months ago by 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.

comment:10 Changed 5 months ago by Don-vip

Milestone: 17.06

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.

comment:11 in reply to:  10 Changed 5 months ago by 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.

Last edited 5 months ago by michael2402 (previous) (diff)

comment:12 Changed 5 months ago by michael2402

Description: modified (diff)

comment:13 Changed 5 months ago by Don-vip

is this ticket fixed?

comment:14 Changed 5 months ago by michael2402

Milestone: 17.0617.07

I have not looked at all commands yet.

comment:15 Changed 4 months ago by Don-vip

Milestone: 17.0717.08

comment:16 Changed 3 months ago by Don-vip

Milestone: 17.0817.09

comment:17 Changed 3 months ago by Don-vip

Keywords: command added

comment:18 Changed 3 months ago by 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.

comment:19 Changed 3 months ago by 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.

Changed 3 months ago by Don-vip

Attachment: commands-layer.patch added

comment:20 in reply to:  19 Changed 3 months ago by 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();

Changed 3 months ago by Don-vip

Attachment: commands-layerv2.patch added

comment:21 Changed 3 months ago by Don-vip

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

comment:22 Changed 3 months ago by 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.

Last edited 3 months ago by michael2402 (previous) (diff)

comment:23 in reply to:  22 ; Changed 3 months ago by 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.

comment:24 in reply to:  23 Changed 3 months ago by 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.

comment:25 Changed 3 months ago by 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

comment:26 Changed 3 months ago by 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

comment:27 Changed 3 months ago by Don-vip

In 12726/josm:

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

comment:28 Changed 3 months ago by Don-vip

In 12727/josm:

see #13036 - forgot two unit tests

comment:29 Changed 3 months ago by Don-vip

In 12728/josm:

see #13036 - fix more unit tests

comment:30 Changed 3 months ago by Don-vip

In 12730/josm:

see #13036 - fix TagInfo integration test

comment:31 Changed 3 months ago by 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

comment:32 Changed 3 months ago by 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

comment:33 Changed 3 months ago by Don-vip

In 12749/josm:

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

comment:34 Changed 3 months ago by Don-vip

In 12751/josm:

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

comment:35 Changed 2 months ago by Don-vip

In 12752/josm:

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

comment:36 Changed 2 months ago by Don-vip

In 12754/josm:

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

comment:37 Changed 2 months ago by Don-vip

In 12757/josm:

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

comment:38 Changed 2 months ago by Don-vip

In 12759/josm:

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

comment:39 Changed 2 months ago by Don-vip

In 12760/josm:

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

comment:40 Changed 2 months ago by Don-vip

In 12761/josm:

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

comment:41 Changed 2 months ago by Don-vip

In 12762/josm:

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

comment:42 Changed 2 months ago by Don-vip

In 12763/josm:

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

comment:43 Changed 2 months ago by Don-vip

In 12769/josm:

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

comment:44 Changed 2 months ago by Don-vip

In 12785/josm:

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

comment:45 Changed 2 months ago by Don-vip

In 12828/josm:

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

comment:46 Changed 8 weeks ago by Don-vip

Milestone: 17.0917.10

comment:47 Changed 6 weeks ago by Don-vip

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

comment:48 Changed 3 weeks ago by Don-vip

Milestone: 17.1017.11

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