"building tool" plugin produces error when create a build over a node with "addr:housenumber" tag and member of relation

[oleksii.zagorskyi@…]

What steps will reproduce the problem?

Have a node with "addr:housenumber=16" tag which is a member of a relation (assotiatedStreet), use "building tool" to draw a build over the node.
The existing node being merged with the new build, preserving "addr:housenumber=16" to the new build.

What is the expected result?

It should work without errors in plugin.

What happens instead?

When you do 2nd click to create the new build - an error happens in the "building tool" plugin.
If ignore the error and continue (which is bad, I know), the build is created, but it's not a member of the relation and data is actually broken.

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

NOTE: if draw a build not over the node, but on a side, then move the node over the build and use "Data -> Merge address point" menu - it works great, but it's complicated.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-11-28 01:09:01 +0100 (Wed, 28 Nov 2018)
Build-Date:2018-11-28 00:26:41
Revision:14460
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14460 en) Linux Ubuntu 18.04.1 LTS
Memory Usage: 714 MB / 910 MB (115 MB allocated, but free)
Java version: 1.8.0_121-b13, Oracle Corporation, Java HotSpot(TM) Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080

Dataset consistency test:
[NO DATASET] {Way id=-157355 version=0 VT nodes=[{Node id=-157351 version=0 V lat=50.36605247966937,lon=30.41936695307493}, {Node id=-157354 version=0 V lat=50.36605068553871,lon=30.41953852437232}, {Node id=-157353 version=0 V lat=50.36594631877727,lon=30.41953584216331}, {Node id=-157352 version=0 V lat=50.36594811291188,lon=30.419364270865916}, {Node id=-157351 version=0 V lat=50.36605247966937,lon=30.41936695307493}]} is referenced by {Relation id=6796038 version=3 VT [node 4784243388, way 460398654, node 4784243387, node 4784243383, way 459892042, way 460398678, node 4784243384, node 4784243386, way 460398711, node 4784243385, way 460398714, way 460398653, node 4784243381, way 460398682, node 4784243380, way 460398683, node 4784243379, way 460398656, way 460398720, way 460398685, way -157355, way 460398686, way 170077933]} but not found in dataset


Plugins:
+ BuildingGeneralization (23)
+ DirectDownload (34593)
+ DirectUpload (34502)
+ ElevationProfile (34746)
+ FixAddresses (34511)
+ HouseNumberTaggingTool (34517)
+ ImageWayPoint (34206)
+ InfoMode (34755)
+ PicLayer (34544)
+ ShapeTools (1240)
+ alignways (34489)
+ buildings_tools (34724)
+ editgpx (34751)
+ gpxfilter (34506)
+ imagery_offset_db (34641)
+ log4j (34527)
+ measurement (34529)
+ namemanager (34532)
+ openvisible (34536)
+ poly (34546)
+ reltoolbox (34788)
+ reverter (34552)
+ tageditor (34560)
+ todo (30306)
+ turnrestrictions (34643)
+ undelete (34568)
+ utilsplugin2 (34506)

Tagging presets:
+ https://raw.githubusercontent.com/yopaseopor/traffic_signs_preset_JOSM/master/UA.zip

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
- E: Handled by bug report queue: org.openstreetmap.josm.data.osm.DataIntegrityProblemException: Relation member must be part of the same dataset as relation(relation 6796038, way -155903)
- E: Handled by bug report queue: org.openstreetmap.josm.data.osm.DataIntegrityProblemException: Relation member must be part of the same dataset as relation(relation 6796038, way -157355)

[anonymous]

I've found what is the bug in the plugin code and I've fixed it.

Here is a simple patch:

Index: src/org/openstreetmap/josm/plugins/buildings_tools/Building.java
===================================================================
--- src/org/openstreetmap/josm/plugins/buildings_tools/Building.java    (revision 34797)
+++ src/org/openstreetmap/josm/plugins/buildings_tools/Building.java    (working copy)
@@ -385,13 +385,14 @@
         }
         cmds.add(new AddCommand(ds, w));

-        addAddress(w);
-
         if (snap) {
             snapBuildings(w, nodes, cmds);
         }
         Command c = new SequenceCommand(tr("Create building"), cmds);
         UndoRedoHandler.getInstance().add(c);
+
+        addAddress(w);
+
         return w;
     }

[stoecker]

Seems correct. It would be even better, when in this case addAddress would add its commands to "cmds" instead of creating a separate undo action. Untested, does this work:

src/org/openstreetmap/josm/plugins/buildings_tools/Building.java

@@ -336 +336 @@
         Collections.sort(ways);
         w = ways.get(0);
 
-        addAddress(w);
+        addAddress(w, null);
 
         return w;
     }
@@ -385 +385 @@
         }
         cmds.add(new AddCommand(ds, w));
 
-        addAddress(w);
+        addAddress(w, cmds);
 
         if (snap) {
             snapBuildings(w, nodes, cmds);
@@ -451 +451 @@
         }
     }
 
-    private void addAddress(Way w) {
+    private void addAddress(Way w, Collection<Command> cmdList) {
         if (ToolSettings.PROP_USE_ADDR_NODE.get()) {
             Node addrNode = getAddressNode();
             if (addrNode != null) {
-                Collection<Command> addressCmds = new LinkedList<>();
+                Collection<Command> addressCmds = cmdList != null ? cmdList : new LinkedList<>();
                 for (Entry<String, String> entry : addrNode.getKeys().entrySet()) {
                     w.put(entry.getKey(), entry.getValue());
                 }
@@ -472 +472 @@
                     addressCmds.add(new ChangeCommand(r, rnew));
                 }
                 addressCmds.add(new DeleteCommand(addrNode));
-                Command c = new SequenceCommand(tr("Add address for building"), addressCmds);
-                UndoRedoHandler.getInstance().add(c);
+                if (cmdList == null) {
+                    Command c = new SequenceCommand(tr("Add address for building"), addressCmds);
+                    UndoRedoHandler.getInstance().add(c);
+                }
             }
         }
     }

[stoecker]

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

[stoecker]

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

[oleksii]

Still, don't forget to move "addAddress(w, cmds);" 5 lines down.

[stoecker]

No, that would be wrong. My patch includes the changes into the SequenceCommand and thus the order should be correct.

[stoecker]

In [o34807:34808].

[oleksii.zagorskyi@…]

As for consistency ...
In a function createCircle() the addAddress() is called as very last step before return;

While in the similar (affected) function createRectangle(), the addAddress() was/is called NOT as very last step before return;

That was a way I fixed the issue - call addAddress() as very last before return;

I'm not a programmer, so don't want to argue :), but it looks to me like a not consistent thing.
I've tested your commit and it works good, I'm satisfied.

[stoecker]

;-) You don't need to look at the code positions itself, but on the relevant logic.

Before addAddress() was called before the CommandStack action "Create building" was done, but used the elements of this action. Now addAddress() is part of the same CommandStack and thus in correct order again.

createCircle() is still suboptimal, as it creates more than one Command item, but at least it is in correct order.