Ticket #17906: 17906.patch

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

@@ -307 +307 @@
         int offset = 0;
         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 = new int[] {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 groups;
+    }
+
     /**
      * Checks that a range of rows can be removed.
      * @param rows indexes of rows to remove
@@ -444 +472 @@
 
     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);
         }
         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);
+        }
     }
 
     public void addMembersAtBeginning(List<? extends OsmPrimitive> primitives) {

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

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 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;
-
 /**
  * Unit tests of {@link MemberTableModel} class.
  */
@@ -34 +43 @@
 
             @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")
+                );
+    }
 }