From c73c8c28e34bdcac21e89aa89b69e85dad9e0189 Mon Sep 17 00:00:00 2001 From: gmungoc Date: Mon, 19 Sep 2016 11:30:37 +0100 Subject: [PATCH] JAL-2172 test and code updates for readonly selection list --- src/jalview/api/AlignViewportI.java | 12 +++ src/jalview/appletgui/AlignFrame.java | 9 +- src/jalview/bin/JalviewLite.java | 2 +- src/jalview/datamodel/ColumnSelection.java | 93 +++++++++++------ src/jalview/gui/AlignFrame.java | 9 +- src/jalview/gui/AnnotationPanel.java | 9 +- src/jalview/gui/VamsasApplication.java | 10 +- src/jalview/viewmodel/AlignmentViewport.java | 10 +- test/jalview/datamodel/ColumnSelectionTest.java | 125 +++++++++++++++++++---- 9 files changed, 208 insertions(+), 71 deletions(-) diff --git a/src/jalview/api/AlignViewportI.java b/src/jalview/api/AlignViewportI.java index df57cc0..c08527c 100644 --- a/src/jalview/api/AlignViewportI.java +++ b/src/jalview/api/AlignViewportI.java @@ -53,6 +53,18 @@ public interface AlignViewportI extends ViewStyleI */ public int calcPanelHeight(); + /** + * Answers true if the viewport has at least one column selected + * + * @return + */ + boolean hasSelectedColumns(); + + /** + * Answers true if the viewport has at least one hidden column + * + * @return + */ boolean hasHiddenColumns(); boolean isValidCharWidth(); diff --git a/src/jalview/appletgui/AlignFrame.java b/src/jalview/appletgui/AlignFrame.java index 0312015..e36944f 100644 --- a/src/jalview/appletgui/AlignFrame.java +++ b/src/jalview/appletgui/AlignFrame.java @@ -703,9 +703,7 @@ public class AlignFrame extends EmbmenuFrame implements ActionListener, // Hide everything by the current selection - this is a hack - we do the // invert and then hide // first check that there will be visible columns after the invert. - if ((viewport.getColumnSelection() != null - && viewport.getColumnSelection().getSelected() != null && viewport - .getColumnSelection().getSelected().size() > 0) + if (viewport.hasSelectedColumns() || (sg != null && sg.getSize() > 0 && sg.getStartRes() <= sg .getEndRes())) { @@ -733,8 +731,7 @@ public class AlignFrame extends EmbmenuFrame implements ActionListener, hide = true; viewport.hideAllSelectedSeqs(); } - else if (!(toggleCols && viewport.getColumnSelection().getSelected() - .size() > 0)) + else if (!(toggleCols && viewport.hasSelectedColumns())) { viewport.showAllHiddenSeqs(); } @@ -742,7 +739,7 @@ public class AlignFrame extends EmbmenuFrame implements ActionListener, if (toggleCols) { - if (viewport.getColumnSelection().getSelected().size() > 0) + if (viewport.hasSelectedColumns()) { viewport.hideSelectedColumns(); if (!toggleSeqs) diff --git a/src/jalview/bin/JalviewLite.java b/src/jalview/bin/JalviewLite.java index 210a07a..b30ad41 100644 --- a/src/jalview/bin/JalviewLite.java +++ b/src/jalview/bin/JalviewLite.java @@ -466,7 +466,7 @@ public class JalviewLite extends Applet implements SequenceI rs = sel.getSequenceAt(0); start = rs.findIndex(start); end = rs.findIndex(end); - List cs = csel.getSelected(); + List cs = new ArrayList(csel.getSelected()); csel.clear(); for (Integer selectedCol : cs) { diff --git a/src/jalview/datamodel/ColumnSelection.java b/src/jalview/datamodel/ColumnSelection.java index 7ef2a68..8bc8f54 100644 --- a/src/jalview/datamodel/ColumnSelection.java +++ b/src/jalview/datamodel/ColumnSelection.java @@ -45,19 +45,52 @@ public class ColumnSelection /* * list of selected columns (ordered by selection order, not column order) */ - private List order = new ArrayList(); + private List order; + + /* + * an unmodifiable view of the selected columns list + */ + private List _uorder; /** * bitfield for column selection - allows quick lookup */ - private BitSet selected = new BitSet(); + private BitSet selected; + + /** + * Constructor + */ + IntList() + { + order = new ArrayList(); + _uorder = Collections.unmodifiableList(order); + selected = new BitSet(); + } + + /** + * Copy constructor + * + * @param other + */ + IntList(IntList other) + { + this(); + if (other != null) + { + int j = other.size(); + for (int i = 0; i < j; i++) + { + add(other.elementAt(i)); + } + } + } /** * adds a new column i to the selection - only if i is not already selected * * @param i */ - public void add(int i) + void add(int i) { if (!selected.get(i)) { @@ -66,13 +99,13 @@ public class ColumnSelection } } - public void clear() + void clear() { order.clear(); selected.clear(); } - public void remove(int col) + void remove(int col) { Integer colInt = new Integer(col); @@ -87,22 +120,27 @@ public class ColumnSelection } } - public boolean contains(Integer colInt) + boolean contains(Integer colInt) { return selected.get(colInt); } - public boolean isEmpty() + boolean isEmpty() { return order.isEmpty(); } - public List getList() + /** + * Returns a read-only view of the selected columns list + * + * @return + */ + List getList() { - return order; + return _uorder; } - public int size() + int size() { return order.size(); } @@ -113,7 +151,7 @@ public class ColumnSelection * @param i * @return */ - public int elementAt(int i) + int elementAt(int i) { return order.get(i); } @@ -156,7 +194,7 @@ public class ColumnSelection * @param change * - delta for shift */ - public void compensateForEdits(int start, int change) + void compensateForEdits(int start, int change) { BitSet mask = new BitSet(); for (int i = 0; i < order.size(); i++) @@ -175,17 +213,17 @@ public class ColumnSelection selected.or(mask); } - public boolean isSelected(int column) + boolean isSelected(int column) { return selected.get(column); } - public int getMaxColumn() + int getMaxColumn() { return selected.length() - 1; } - public int getMinColumn() + int getMinColumn() { return selected.get(0) ? 0 : selected.nextSetBit(0); } @@ -193,7 +231,7 @@ public class ColumnSelection /** * @return a series of selection intervals along the range */ - public List getRanges() + List getRanges() { List rlist = new ArrayList(); if (selected.isEmpty()) @@ -288,14 +326,18 @@ 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. This returns a copy of the actual list, and changes to the - * copy will not affect the selection. + * Returns a read-only view of the (possibly empty) list of selected columns + *

+ * The list contains no duplicates but is not necessarily ordered. It also may + * include columns hidden from the current view. To modify (for example sort) + * the list, you should first make a copy. + *

+ * The list is not thread-safe: iterating over it could result in + * ConcurrentModificationException if it is modified by another thread. */ public List getSelected() { - return new ArrayList(selection.getList()); + return selection.getList(); } /** @@ -949,14 +991,7 @@ public class ColumnSelection { if (copy != null) { - if (copy.selection != null) - { - selection = new IntList(); - for (int i = 0, j = copy.selection.size(); i < j; i++) - { - selection.add(copy.selection.elementAt(i)); - } - } + selection = new IntList(copy.selection); if (copy.hiddenColumns != null) { hiddenColumns = new Vector(copy.hiddenColumns.size()); diff --git a/src/jalview/gui/AlignFrame.java b/src/jalview/gui/AlignFrame.java index 1848595..49bd138 100644 --- a/src/jalview/gui/AlignFrame.java +++ b/src/jalview/gui/AlignFrame.java @@ -2977,9 +2977,7 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, // Hide everything by the current selection - this is a hack - we do the // invert and then hide // first check that there will be visible columns after the invert. - if ((viewport.getColumnSelection() != null - && viewport.getColumnSelection().getSelected() != null && viewport - .getColumnSelection().getSelected().size() > 0) + if (viewport.hasSelectedColumns() || (sg != null && sg.getSize() > 0 && sg.getStartRes() <= sg .getEndRes())) { @@ -3007,8 +3005,7 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, hideSelSequences_actionPerformed(null); hide = true; } - else if (!(toggleCols && viewport.getColumnSelection().getSelected() - .size() > 0)) + else if (!(toggleCols && viewport.hasSelectedColumns())) { showAllSeqs_actionPerformed(null); } @@ -3016,7 +3013,7 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, if (toggleCols) { - if (viewport.getColumnSelection().getSelected().size() > 0) + if (viewport.hasSelectedColumns()) { hideSelColumns_actionPerformed(null); if (!toggleSeqs) diff --git a/src/jalview/gui/AnnotationPanel.java b/src/jalview/gui/AnnotationPanel.java index bd903b3..7450555 100755 --- a/src/jalview/gui/AnnotationPanel.java +++ b/src/jalview/gui/AnnotationPanel.java @@ -49,6 +49,7 @@ import java.awt.event.MouseMotionListener; import java.awt.event.MouseWheelEvent; import java.awt.event.MouseWheelListener; import java.awt.image.BufferedImage; +import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -445,8 +446,12 @@ public class AnnotationPanel extends JPanel implements AwtRenderPanelI, StringBuilder collatedInput = new StringBuilder(64); String last = ""; ColumnSelection viscols = av.getColumnSelection(); - // TODO: refactor and save av.getColumnSelection for efficiency - List selected = viscols.getSelected(); + + /* + * the selection list (read-only view) is in selection order, not + * column order; make a copy so we can sort it + */ + List selected = new ArrayList(viscols.getSelected()); Collections.sort(selected); for (int index : selected) { diff --git a/src/jalview/gui/VamsasApplication.java b/src/jalview/gui/VamsasApplication.java index 7588e07..afb6df4 100644 --- a/src/jalview/gui/VamsasApplication.java +++ b/src/jalview/gui/VamsasApplication.java @@ -321,6 +321,7 @@ public class VamsasApplication implements SelectionSource, VamsasSource Thread udthread = new Thread(new Runnable() { + @Override public void run() { Cache.log.info("Jalview updating to the Vamsas Session."); @@ -639,6 +640,7 @@ public class VamsasApplication implements SelectionSource, VamsasSource final VamsasApplication client = this; vclient.addDocumentUpdateHandler(new PropertyChangeListener() { + @Override public void propertyChange(PropertyChangeEvent evt) { Cache.log.debug("Dealing with document update event."); @@ -656,6 +658,7 @@ public class VamsasApplication implements SelectionSource, VamsasSource uk.ac.vamsas.client.Events.DOCUMENT_REQUESTTOCLOSE, new PropertyChangeListener() { + @Override public void propertyChange(PropertyChangeEvent evt) { if (client.promptUser) @@ -774,6 +777,7 @@ public class VamsasApplication implements SelectionSource, VamsasSource { String last = null; + @Override public void handleMessage(Message message) { if (vobj2jv == null) @@ -998,6 +1002,7 @@ public class VamsasApplication implements SelectionSource, VamsasSource selecter = new SelectionListener() { + @Override public void selection(SequenceGroup seqsel, ColumnSelection colsel, SelectionSource source) { @@ -1065,11 +1070,10 @@ public class VamsasApplication implements SelectionSource, VamsasSource { // gather selected columns outwith the sequence positions // too - for (Object obj : colsel.getSelected()) + for (Integer ival : colsel.getSelected()) { - int ival = ((Integer) obj).intValue(); Pos p = new Pos(); - p.setI(ival + 1); + p.setI(ival.intValue() + 1); range.addPos(p); } } diff --git a/src/jalview/viewmodel/AlignmentViewport.java b/src/jalview/viewmodel/AlignmentViewport.java index 97f2e1c..47227d5 100644 --- a/src/jalview/viewmodel/AlignmentViewport.java +++ b/src/jalview/viewmodel/AlignmentViewport.java @@ -1107,6 +1107,13 @@ public abstract class AlignmentViewport implements AlignViewportI, } @Override + public boolean hasSelectedColumns() + { + ColumnSelection columnSelection = getColumnSelection(); + return columnSelection != null && columnSelection.hasSelectedColumns(); + } + + @Override public boolean hasHiddenColumns() { return colSel != null && colSel.hasHiddenColumns(); @@ -2763,8 +2770,7 @@ public abstract class AlignmentViewport implements AlignViewportI, if (sg != null && (sgs = sg.getStartRes()) >= 0 && sg.getStartRes() <= (sge = sg.getEndRes()) - && (colSel == null || colSel.getSelected() == null || colSel - .getSelected().size() == 0)) + && !this.hasSelectedColumns()) { if (!wholewidth && alignment.getWidth() == (1 + sge - sgs)) { diff --git a/test/jalview/datamodel/ColumnSelectionTest.java b/test/jalview/datamodel/ColumnSelectionTest.java index e59719c..a943e7c 100644 --- a/test/jalview/datamodel/ColumnSelectionTest.java +++ b/test/jalview/datamodel/ColumnSelectionTest.java @@ -22,12 +22,14 @@ 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; +import static org.testng.AssertJUnit.fail; import java.util.Arrays; import java.util.BitSet; +import java.util.Collections; +import java.util.ConcurrentModificationException; import java.util.List; import org.testng.annotations.Test; @@ -68,8 +70,8 @@ public class ColumnSelectionTest // removing an element in the list removes it cs.removeElement(2); - // ...but not from the copy list! - assertEquals(2, sel.size()); + // ...and also from the read-only view + assertEquals(1, sel.size()); sel = cs.getSelected(); assertEquals(1, sel.size()); assertEquals(new Integer(5), sel.get(0)); @@ -542,7 +544,7 @@ public class ColumnSelectionTest * were added */ @Test(groups = { "Functional" }) - public void testGetSelection() + public void testGetSelected() { ColumnSelection cs = new ColumnSelection(); int[] sel = { 4, 3, 7, 21 }; @@ -551,32 +553,90 @@ public class ColumnSelectionTest cs.addElement(col); } - List selected1 = cs.getSelected(); - assertEquals(4, selected1.size()); + List selected = cs.getSelected(); + assertEquals(4, selected.size()); + assertEquals("[4, 3, 7, 21]", selected.toString()); /* - * getSelected returns a copy, verify the list - * is externally immutable + * getSelected returns a read-only view of the list + * verify the view follows any changes in it */ - selected1.clear(); - List selected2 = cs.getSelected(); - assertNotSame(selected1, selected2); - assertEquals(4, selected2.size()); - int i = 0; - for (int col : sel) + cs.removeElement(7); + cs.addElement(1); + cs.removeElement(4); + assertEquals("[3, 21, 1]", selected.toString()); + } + + /** + * Test to verify that the list returned by getSelection cannot be modified + */ + @Test(groups = { "Functional" }) + public void testGetSelected_isReadOnly() + { + ColumnSelection cs = new ColumnSelection(); + cs.addElement(3); + + List selected = cs.getSelected(); + try + { + selected.clear(); + fail("expected exception"); + } catch (UnsupportedOperationException e) + { + // expected + } + try + { + selected.add(1); + fail("expected exception"); + } catch (UnsupportedOperationException e) + { + // expected + } + try + { + selected.remove(3); + fail("expected exception"); + } catch (UnsupportedOperationException e) { - assertEquals(col, selected2.get(i++).intValue()); + // expected } + try + { + Collections.sort(selected); + fail("expected exception"); + } catch (UnsupportedOperationException e) + { + // expected + } + } - cs.removeElement(7); + /** + * Test that demonstrates a ConcurrentModificationException is thrown if you + * change the selection while iterating over it + */ + @Test( + groups = "Functional", + expectedExceptions = { ConcurrentModificationException.class }) + public void testGetSelected_concurrentModification() + { + ColumnSelection cs = new ColumnSelection(); + cs.addElement(0); cs.addElement(1); - cs.removeElement(4); + cs.addElement(2); - List 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()); + /* + * simulate changing the list under us (e.g. in a separate + * thread) while iterating over it -> ConcurrentModificationException + */ + List selected = cs.getSelected(); + for (Integer col : selected) + { + if (col.intValue() == 0) + { + cs.removeElement(1); + } + } } @Test(groups = "Functional") @@ -669,4 +729,25 @@ public class ColumnSelectionTest assertTrue(selected.contains(6)); assertTrue(selected.contains(10)); } + + @Test(groups = "Functional") + public void testCopyConstructor() + { + ColumnSelection cs = new ColumnSelection(); + cs.addElement(3); + cs.addElement(1); + cs.hideColumns(10, 11); + cs.hideColumns(5, 7); + assertEquals("[5, 7]", Arrays.toString(cs.getHiddenColumns().get(0))); + + ColumnSelection cs2 = new ColumnSelection(cs); + assertTrue(cs2.hasSelectedColumns()); + assertTrue(cs2.hasHiddenColumns()); + // order of column selection is preserved + assertEquals("[3, 1]", cs2.getSelected().toString()); + assertEquals(2, cs2.getHiddenColumns().size()); + // hidden columns are held in column order + assertEquals("[5, 7]", Arrays.toString(cs2.getHiddenColumns().get(0))); + assertEquals("[10, 11]", Arrays.toString(cs2.getHiddenColumns().get(1))); + } } -- 1.7.10.2