JAL-2001 return copy of selection in getSelected()

diff --git a/src/jalview/bin/JalviewLite.java b/src/jalview/bin/JalviewLite.java
index 13e4b7e..210a07a 100644 (file)
--- a/src/jalview/bin/JalviewLite.java
+++ b/src/jalview/bin/JalviewLite.java
@@ -466,17 +466,11 @@ public class JalviewLite extends Applet implements
         SequenceI rs = sel.getSequenceAt(0);
         start = rs.findIndex(start);
         end = rs.findIndex(end);
-        if (csel != null)
-        {
-          List<Integer> cs = csel.getSelected();
-          // note - the following actually clears cs as well, since
-          // csel.getSelected returns a reference. Need to check if we need to
-          // have a concurrentModification exception thrown here
-          csel.clear();
-          for (Integer selectedCol : cs)
-          {
-            csel.addElement(rs.findIndex(selectedCol));
-          }
+        List<Integer> cs = csel.getSelected();
+        csel.clear();
+        for (Integer selectedCol : cs)
+        {
+          csel.addElement(rs.findIndex(selectedCol));
         }
       }
       sel.setStartRes(start);

diff --git a/src/jalview/datamodel/ColumnSelection.java b/src/jalview/datamodel/ColumnSelection.java
index aaf70b8..47acbd0 100644 (file)
--- a/src/jalview/datamodel/ColumnSelection.java
+++ b/src/jalview/datamodel/ColumnSelection.java
@@ -290,11 +290,12 @@ public class ColumnSelection
   /**
    * Returns a list of selected columns. The list contains no duplicates but is
    * not necessarily ordered. It also may include columns hidden from the
-   * current view
+   * current view. This returns a copy of the actual list, and changes to the
+   * copy will not affect the selection.
    */
   public List<Integer> getSelected()
   {
-    return selection.getList();
+    return new ArrayList<Integer>(selection.getList());
   }
 
   /**

diff --git a/test/jalview/datamodel/ColumnSelectionTest.java b/test/jalview/datamodel/ColumnSelectionTest.java
index 04af3ce..2deb1a7 100644 (file)
--- a/test/jalview/datamodel/ColumnSelectionTest.java
+++ b/test/jalview/datamodel/ColumnSelectionTest.java
@@ -22,6 +22,7 @@ package jalview.datamodel;
 
 import static org.testng.AssertJUnit.assertEquals;
 import static org.testng.AssertJUnit.assertFalse;
+import static org.testng.AssertJUnit.assertNotSame;
 import static org.testng.AssertJUnit.assertSame;
 import static org.testng.AssertJUnit.assertTrue;
 
@@ -66,6 +67,9 @@ public class ColumnSelectionTest
 
     // removing an element in the list removes it
     cs.removeElement(2);
+    // ...but not from the copy list!
+    assertEquals(2, sel.size());
+    sel = cs.getSelected();
     assertEquals(1, sel.size());
     assertEquals(new Integer(5), sel.get(0));
   }
@@ -545,21 +549,32 @@ public class ColumnSelectionTest
     {
       cs.addElement(col);
     }
-    List<Integer> selected = cs.getSelected();
-    assertEquals(4, selected.size());
+
+    List<Integer> selected1 = cs.getSelected();
+    assertEquals(4, selected1.size());
+
+    /*
+     * getSelected returns a copy, verify the list
+     * is externally immutable
+     */
+    selected1.clear();
+    List<Integer> selected2 = cs.getSelected();
+    assertNotSame(selected1, selected2);
+    assertEquals(4, selected2.size());
     int i = 0;
     for (int col : sel)
     {
-      assertEquals(col, selected.get(i++).intValue());
+      assertEquals(col, selected2.get(i++).intValue());
     }
 
     cs.removeElement(7);
     cs.addElement(1);
     cs.removeElement(4);
-    selected = cs.getSelected();
-    assertEquals(3, selected.size());
-    assertEquals(3, selected.get(0).intValue());
-    assertEquals(21, selected.get(1).intValue());
-    assertEquals(1, selected.get(2).intValue());
+
+    List<Integer> selected3 = cs.getSelected();
+    assertEquals(3, selected3.size());
+    assertEquals(3, selected3.get(0).intValue());
+    assertEquals(21, selected3.get(1).intValue());
+    assertEquals(1, selected3.get(2).intValue());
   }
 }