diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java b/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java index 1931138b4c..db6cb4e899 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModel.java @@ -307,18 +307,46 @@ implements TableModelListener, DataSelectionListener, DataSetListener, OsmPrimit 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 groupRows(int... rows) { + if (rows.length == 0) { + return Collections.emptyList(); + } + List 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,11 +472,27 @@ implements TableModelListener, DataSelectionListener, DataSetListener, OsmPrimit void addMembersAtIndexKeepingOldSelection(final Iterable 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 primitives) { diff --git a/test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java b/test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java index 38f3502310..942c47d57c 100644 --- a/test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java +++ b/test/unit/org/openstreetmap/josm/gui/dialogs/relation/MemberTableModelTest.java @@ -1,20 +1,29 @@ // 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,8 +43,60 @@ class MemberTableModelTest { @Override public Collection getSelection() { - return Collections.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 getSelection() { + return Collections.singleton(relation); + } + + @Override + public void updateTags(List 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") + ); + } }