Fix #23104: Mark Selected should account for items not in the list

Signed-off-by: Taylor Smock <[email protected]>
JOSM · Aug 7, 2023 · bfe94e7 · bfe94e7
1 parent b3a12d2
commit bfe94e7
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 6 deletions.
diff --git a/src/org/openstreetmap/josm/plugins/todo/TodoListModel.java b/src/org/openstreetmap/josm/plugins/todo/TodoListModel.java
@@ -227,11 +227,13 @@ void markItems(Collection<TodoListItem> items) {
         final var tempDoneList = new ArrayList<TodoListItem>(indices.length);
         for (var i = indices.length - 1; i >= 0; i--) {
             final var index = indices[i];
-            final var item = todoList.get(index);
-            todoList.remove(index);
-            tempDoneList.add(item);
-            if (sel > index)
-                sel--;
+            if (index >= 0) {
+                final var item = todoList.get(index);
+                todoList.remove(index);
+                tempDoneList.add(item);
+                if (sel > index)
+                    sel--;
+            }
         }
         doneList.addAll(tempDoneList);
         if (sel >= getSize())

diff --git a/test/unit/org/openstreetmap/josm/plugins/todo/TodoDialogTest.java b/test/unit/org/openstreetmap/josm/plugins/todo/TodoDialogTest.java
@@ -1,6 +1,7 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.todo;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertSame;
@@ -107,7 +108,24 @@ void testClear() {
         assertTrue(this.model.getTodoList().isEmpty());
     }
 
+    /**
+     * Non-regression test for #23104: `Mark Selected` should account for items not in the list
+     */
     @Test
-    void testNonRegression23092() {
+    void testNonRegression23104() throws Exception {
+        // Sort the list for stability in tests
+        final var list = new ArrayList<>(this.ds.allPrimitives());
+        list.sort(Comparator.comparing(prim -> prim.get("access")));
+        this.ds.setSelected(list.get(0));
+        this.actAdd.actionPerformed(null);
+        final var actMarkSelected = (JosmAction) tryToReadFieldValue("actMarkSelected");
+        assertEquals(1, this.model.getSize());
+        assertSame(list.get(0), this.model.getElementAt(0).primitive());
+        this.ds.setSelected(list.get(1));
+        assertDoesNotThrow(() -> actMarkSelected.actionPerformed(null));
+        this.ds.setSelected(list.get(1), list.get(2));
+        assertDoesNotThrow(() -> actMarkSelected.actionPerformed(null));
+        this.ds.setSelected(list);
+        assertDoesNotThrow(() -> actMarkSelected.actionPerformed(null));
     }
 }