Changeset 18439 in josm

Timestamp:

2022-05-04T21:08:02+02:00 (3 years ago)

Author:

taylor.smock

Message:

Fix #17906, #21889, see #12617 for the original issue

This fixes an issue where a relation member could be deleted accidentally
when a user moved the relation member to its current position. This also
affected multiple members when multiple members were selected. The user
just had to move the selected members to another selected member position.

This patch also improves relation move performance when large numbers of
relation members are moved.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java

@@ -308 +308 @@
         final ListSelectionModel selectionModel = getSelectionModel();
         selectionModel.setValueIsAdjusting(true);
-        for (int row : selectedRows) {
-            row -= offset;
-            if (members.size() > row) {
-                members.remove(row);
-                selectionModel.removeIndexInterval(row, row);
-                offset++;
+        for (int[] row : groupRows(selectedRows)) {
+            if (members.size() > row[0] - offset) {
+                // Remove (inclusive)
+                members.subList(row[0] - offset, row[1] - offset + 1).clear();
+                selectionModel.removeIndexInterval(row[0] - offset, row[1] - offset);
+                offset += row[1] - row[0] + 1;
             }
         }
         selectionModel.setValueIsAdjusting(false);
         fireTableDataChanged();
+    }
+
+    /**
+     * Group rows for use in changing selection intervals, to avoid many small calls on large selections
+     * @param rows The rows to group
+     * @return A list of grouped rows, [lower, higher] (inclusive)
+     */
+    private static List<int[]> groupRows(int... rows) {
+        if (rows.length == 0) {
+            return Collections.emptyList();
+        }
+        List<int[]> groups = new ArrayList<>();
+        int[] current = {Integer.MIN_VALUE, Integer.MIN_VALUE};
+        groups.add(current);
+        for (int row : rows) {
+            if (current[0] == Integer.MIN_VALUE) {
+                current[0] = row;
+                current[1] = row;
+                continue;
+            }
+            if (current[1] == row - 1) {
+                current[1] = row;
+            } else {
+                current = new int[] {row, row};
+                groups.add(current);
+            }
+        }
+        return Collections.unmodifiableList(groups);
     }
 
@@ -445 +473 @@
     void addMembersAtIndexKeepingOldSelection(final Iterable<RelationMember> newMembers, final int index) {
         int idx = index;
+        // Avoid having the inserted rows from being part of the selection. See JOSM #12617, #17906, #21889.
+        int[] originalSelection = null;
+        if (selectedIndices().anyMatch(selectedRow -> selectedRow == index)) {
+            originalSelection = getSelectedIndices();
+        }
         for (RelationMember member : newMembers) {
             members.add(idx++, member);
@@ -450 +483 @@
         invalidateConnectionType();
         fireTableRowsInserted(index, idx - 1);
+        if (originalSelection != null) {
+            final DefaultListSelectionModel model = this.getSelectionModel();
+            model.setValueIsAdjusting(true);
+            model.clearSelection();
+            final int tIdx = idx;
+            // Avoiding many addSelectionInterval calls is critical for performance.
+            for (int[] row : groupRows(IntStream.of(originalSelection).map(i -> i < index ? i : i + tIdx - index).toArray())) {
+                model.addSelectionInterval(row[0], row[1]);
+            }
+            model.setValueIsAdjusting(false);
+        }
     }
 

trunk/test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.dialogs.relation;
 
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.stream.Stream;
 
+import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.Relation;
+import org.openstreetmap.josm.data.osm.RelationMember;
 import org.openstreetmap.josm.data.osm.Tag;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetHandler;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
-
-import org.junit.jupiter.api.Test;
 
 /**
@@ -35 +44 @@
             @Override
             public Collection<OsmPrimitive> getSelection() {
-                return Collections.<OsmPrimitive>singleton(n);
+                return Collections.singleton(n);
             }
         }).getRelationMemberForPrimitive(n));
     }
+
+    /**
+     * Non-regression test for JOSM #12617, #17906, and #21889. Regrettably, it was not easily possible to test using
+     * drag-n-drop methods.
+     */
+    @Test
+    void testTicket12617() {
+        final Node[] nodes = new Node[10];
+        for (int i = 0; i < nodes.length; i++) {
+            nodes[i] = new Node(LatLon.ZERO);
+            // The id has to be > 0
+            nodes[i].setOsmId(i + 1, 1);
+        }
+        final Relation relation = TestUtils.newRelation("", Stream.of(nodes).map(node -> new RelationMember("", node))
+                .toArray(RelationMember[]::new));
+        final OsmDataLayer osmDataLayer = new OsmDataLayer(new DataSet(), "testTicket12617", null);
+        osmDataLayer.getDataSet().addPrimitiveRecursive(relation);
+        final MemberTableModel model = new MemberTableModel(relation, osmDataLayer, new TaggingPresetHandler() {
+            @Override
+            public Collection<OsmPrimitive> getSelection() {
+                return Collections.singleton(relation);
+            }
+
+            @Override
+            public void updateTags(List<Tag> tags) {
+                // Do nothing
+            }
+        });
+
+        model.populate(relation);
+        // Select the members to move
+        model.setSelectedMembersIdx(Arrays.asList(2, 3, 5, 9));
+        // Move the members (this is similar to what the drag-n-drop code is doing)
+        model.addMembersAtIndexKeepingOldSelection(model.getSelectedMembers(), 2);
+        model.remove(model.getSelectedIndices());
+        // Apply the changes
+        model.applyToRelation(relation);
+
+        // Perform the tests
+        assertAll(() -> assertEquals(10, relation.getMembersCount(), "There should be no changes to the member count"),
+                () -> assertEquals(nodes[0], relation.getMember(0).getMember()),
+                () -> assertEquals(nodes[1], relation.getMember(1).getMember()),
+                () -> assertEquals(nodes[2], relation.getMember(2).getMember(), "Node 2 should not have moved"),
+                () -> assertEquals(nodes[3], relation.getMember(3).getMember(), "Node 3 should not have moved"),
+                () -> assertEquals(nodes[4], relation.getMember(6).getMember(), "Node 4 should be in position 5"),
+                () -> assertEquals(nodes[5], relation.getMember(4).getMember(), "Node 5 should be in position 4"),
+                () -> assertEquals(nodes[6], relation.getMember(7).getMember(), "Node 6 should not have moved"),
+                () -> assertEquals(nodes[7], relation.getMember(8).getMember()),
+                () -> assertEquals(nodes[8], relation.getMember(9).getMember()),
+                () -> assertEquals(nodes[9], relation.getMember(5).getMember(), "Node 9 should have moved")
+                );
+    }
 }