Ticket #14410: AddPrimitivesCommand-undo-existing.patch

josm/core/src/org/openstreetmap/josm/command/AddPrimitivesCommand.java

@@ -9 +9 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 import javax.swing.Icon;
 
@@ -27 +28 @@
  */
 public class AddPrimitivesCommand extends Command {
 
-    private List<PrimitiveData> data = new ArrayList<>();
-    private Collection<PrimitiveData> toSelect = new ArrayList<>();
+    private List<PrimitiveData> data;
+    private Collection<PrimitiveData> toSelect;
+    private List<PrimitiveData> preexistinglData;
 
     // only filled on undo
     private List<OsmPrimitive> createdPrimitives;
-    private Collection<OsmPrimitive> createdPrimitivesToSelect;
 
     /**
      * Constructs a new {@code AddPrimitivesCommand} to add data to the current edit layer.
@@ -65 +66 @@
 
     private void init(List<PrimitiveData> data, List<PrimitiveData> toSelect) {
         CheckParameterUtil.ensureParameterNotNull(data, "data");
-        this.data.addAll(data);
-        if (toSelect != null) {
-            this.toSelect.addAll(toSelect);
+        this.data = new ArrayList<>(data);
+        if (toSelect == data) {
+            this.toSelect = this.data;
+        } else if (toSelect != null) {
+            this.toSelect = new ArrayList<>(toSelect);
         }
     }
 
     @Override
     public boolean executeCommand() {
-        Collection<OsmPrimitive> primitivesToSelect;
+        DataSet ds = getAffectedDataSet();
         if (createdPrimitives == null) { // first time execution
             List<OsmPrimitive> newPrimitives = new ArrayList<>(data.size());
-            primitivesToSelect = new ArrayList<>(toSelect.size());
+            preexistinglData = new ArrayList<>();
 
             for (PrimitiveData pd : data) {
-                OsmPrimitive primitive = getAffectedDataSet().getPrimitiveById(pd);
+                OsmPrimitive primitive = ds.getPrimitiveById(pd);
                 boolean created = primitive == null;
-                if (created) {
+                if (primitive == null) {
                     primitive = pd.getType().newInstance(pd.getUniqueId(), true);
+                } else {
+                    preexistinglData.add(primitive.save());
                 }
                 if (pd instanceof NodeData) { // Load nodes immediately because they can't be added to dataset without coordinates
                     primitive.load(pd);
                 }
                 if (created) {
-                    getAffectedDataSet().addPrimitive(primitive);
+                    ds.addPrimitive(primitive);
                 }
                 newPrimitives.add(primitive);
-                if (toSelect.contains(pd)) {
-                    primitivesToSelect.add(primitive);
-                }
             }
 
             // Then load ways and relations
@@ -107 +109 @@
             // When redoing this command, we have to add the same objects, otherwise
             // a subsequent command (e.g. MoveCommand) cannot be redone.
             for (OsmPrimitive osm : createdPrimitives) {
-                getAffectedDataSet().addPrimitive(osm);
+                if (preexistinglData.stream().anyMatch(pd -> pd.getUniqueId() == osm.getUniqueId())) {
+                    osm.load(data.stream().filter(pd -> pd.getUniqueId() == osm.getUniqueId()).findAny().get());
+                } else {
+                    ds.addPrimitive(osm);
+                }
             }
-            primitivesToSelect = createdPrimitivesToSelect;
         }
-
-        getAffectedDataSet().setSelected(primitivesToSelect);
+        if (toSelect != null) {
+            ds.setSelected(toSelect.stream()
+                    .map(pd -> ds.getPrimitiveById(pd))
+                    .collect(Collectors.toList()));
+        }
         return true;
     }
 
     @Override public void undoCommand() {
         DataSet ds = getAffectedDataSet();
-
         if (createdPrimitives == null) {
             createdPrimitives = new ArrayList<>(data.size());
-            createdPrimitivesToSelect = new ArrayList<>(toSelect.size());
-
             for (PrimitiveData pd : data) {
                 OsmPrimitive p = ds.getPrimitiveById(pd);
                 createdPrimitives.add(p);
-                if (toSelect.contains(pd)) {
-                    createdPrimitivesToSelect.add(p);
-                }
             }
             createdPrimitives = PurgeCommand.topoSort(createdPrimitives);
-
-            for (PrimitiveData p : data) {
-                ds.removePrimitive(p);
-            }
-            data = null;
-            toSelect = null;
-
-        } else {
-            for (OsmPrimitive osm : createdPrimitives) {
+        }
+        for (OsmPrimitive osm : createdPrimitives) {
+            Optional<PrimitiveData> previous = preexistinglData.stream().filter(pd -> pd.getUniqueId() == osm.getUniqueId()).findAny();
+            if (previous.isPresent()) {
+                osm.load(previous.get());
+            } else {
                 ds.removePrimitive(osm);
             }
         }
@@ -177 +176 @@
 
     @Override
     public int hashCode() {
-        return Objects.hash(super.hashCode(), data, toSelect, createdPrimitives, createdPrimitivesToSelect);
+        return Objects.hash(super.hashCode(), data, toSelect, preexistinglData, createdPrimitives);
     }
 
     @Override
@@ -188 +187 @@
         AddPrimitivesCommand that = (AddPrimitivesCommand) obj;
         return Objects.equals(data, that.data) &&
                Objects.equals(toSelect, that.toSelect) &&
-               Objects.equals(createdPrimitives, that.createdPrimitives) &&
-               Objects.equals(createdPrimitivesToSelect, that.createdPrimitivesToSelect);
+               Objects.equals(preexistinglData, that.preexistinglData) &&
+               Objects.equals(createdPrimitives, that.createdPrimitives);
     }
 }

josm/core/test/unit/org/openstreetmap/josm/command/AddPrimitivesCommandTest.java

@@ -175 +175 @@
     }
 
     /**
+     * Tests if the undo command does not remove
+     * data ignored by by the add command because they where already existing.
+     */
+    @Test
+    public void testUndoIgnoresExisting() {
+        OsmDataLayer layer1 = new OsmDataLayer(new DataSet(), "l1", null);
+        Main.getLayerManager().addLayer(layer1);
+
+        List<PrimitiveData> testData = createTestData();
+
+        assertTrue(new AddPrimitivesCommand(testData).executeCommand());
+        assertEquals(2, layer1.data.getNodes().size());
+        assertEquals(1, layer1.data.getWays().size());
+
+        testData.set(2, createTestNode(7));
+
+        AddPrimitivesCommand command = new AddPrimitivesCommand(testData);
+
+        assertTrue(command.executeCommand());
+
+        assertEquals(3, layer1.data.getNodes().size());
+        assertEquals(1, layer1.data.getWays().size());
+
+        for (int i = 0; i < 2; i++) {
+            // Needs to work multiple times.
+            command.undoCommand();
+
+            assertEquals(2, layer1.data.getNodes().size());
+            assertEquals(1, layer1.data.getWays().size());
+
+            // redo
+            assertTrue(command.executeCommand());
+
+            assertEquals(3, layer1.data.getNodes().size());
+            assertEquals(1, layer1.data.getWays().size());
+        }
+
+
+    }
+
+    /**
      * Test {@link AddCommand#getParticipatingPrimitives()}
      */
     @Test