JAL-2674 Simplify locateVisibleBoundsOfSequence

Take advantage of sequence cursor changes where possible

diff --git a/src/jalview/datamodel/HiddenColumns.java b/src/jalview/datamodel/HiddenColumns.java
index a6b4f82..09f2ec5 100644 (file)
--- a/src/jalview/datamodel/HiddenColumns.java
+++ b/src/jalview/datamodel/HiddenColumns.java
@@ -693,10 +693,9 @@ public class HiddenColumns
    * extent of the sequence
    * 
    * @param seq
-   * @return int[] { visible start, visible end, first seqpos, last seqpos,
-   *         alignment index for seq start, alignment index for seq end }
+   * @return int[] { visible start, first seqpos, last seqpos }
    */
-  public int[] locateVisibleBoundsOfSequence(SequenceI seq)
+  public int locateVisibleBoundsOfSequence(SequenceI seq)
   {
     try
     {
@@ -705,9 +704,7 @@ public class HiddenColumns
 
       if (hiddenColumns == null || hiddenColumns.size() == 0)
       {
-        int ifpos = seq.findIndex(seq.getStart()) - 1;
-        int ilpos = seq.findIndex(seq.getEnd()) - 1;
-        return new int[] { ifpos, ifpos, ilpos };
+        return seq.findIndex(seq.getStart()) - 1;
       }
 
       // Simply walk along the sequence whilst watching for hidden column
@@ -718,20 +715,12 @@ public class HiddenColumns
       int hideEnd = -1;
       int visPrev = 0;
       int visNext = 0;
-      int firstP = -1;
-      int lastP = -1;
+
       boolean foundStart = false;
       for (int p = 0; spos <= seq.getEnd() && p < seq.getLength(); p++)
       {
         if (!Comparison.isGap(seq.getCharAt(p)))
         {
-          // keep track of first/last column
-          // containing sequence data regardless of visibility
-          if (firstP == -1)
-          {
-            firstP = p;
-          }
-          lastP = p;
           // update hidden region start/end
           while (hideEnd < p && regions.hasNext())
           {
@@ -746,13 +735,10 @@ public class HiddenColumns
             hideStart = seq.getLength();
           }
           // update visible boundary for sequence
-          if (p < hideStart)
+          if ((p < hideStart) && (!foundStart))
           {
-            if (!foundStart)
-            {
               start = p;
               foundStart = true;
-            }
           }
           // look for next sequence position
           spos++;
@@ -760,10 +746,10 @@ public class HiddenColumns
       }
       if (foundStart)
       {
-        return new int[] { findColumnPosition(start), firstP, lastP };
+        return findColumnPosition(start);
       }
       // otherwise, sequence was completely hidden
-      return new int[] { visPrev, firstP, lastP };
+      return visPrev;
     } finally
     {
       LOCK.readLock().unlock();
@@ -1916,8 +1902,10 @@ public class HiddenColumns
   }
 
   /**
-   * An iterator which iterates over visible regions in a range as visible
-   * column positions.
+   * An iterator which iterates over visible regions in a range. The range is
+   * specified in terms of visible column positions. Provides a special
+   * "endsAtHidden" indicator to allow callers to determine if the final visible
+   * column is adjacent to a hidden region.
    */
   public class VisibleBlocksVisBoundsIterator implements Iterator<int[]>
   {
@@ -1927,8 +1915,19 @@ public class HiddenColumns
 
     private boolean endsAtHidden = false;
 
+    /**
+     * Constructor for iterator over visible regions in a range.
+     * 
+     * @param start
+     *          start position in terms of visible column position
+     * @param end
+     *          end position in terms of visible column position
+     * @param usecopy
+     *          whether to use a local copy of hidden columns
+     */
     VisibleBlocksVisBoundsIterator(int start, int end, boolean usecopy)
     {
+      /* actually this implementation always uses a local copy but this may change in future */
       try
       {
         if (usecopy)
@@ -1942,7 +1941,6 @@ public class HiddenColumns
           int blockEnd = end;
           int hiddenSoFar = 0;
           int visSoFar = 0;
-          endsAtHidden = false;
 
           // iterate until a region begins within (start,end]
           int i = 0;
@@ -1955,34 +1953,47 @@ public class HiddenColumns
           }
 
           blockStart += hiddenSoFar; // convert start to absolute position
-          blockEnd += hiddenSoFar; // convert end too in
+          blockEnd += hiddenSoFar; // convert end to absolute position
 
-          // iterate from start to end, adding each hidden region. Positions are
-          // absolute, and all regions which *overlap* [start,end] are used.
+          // iterate from start to end, adding each visible region. Positions
+          // are
+          // absolute, and all hidden regions which overlap [start,end] are
+          // used.
           while (i < hiddenColumns.size()
                   && (hiddenColumns.get(i)[0] <= blockEnd))
           {
             int[] region = hiddenColumns.get(i);
 
+            // end position of this visible region is either just before the
+            // start of the next hidden region, or the absolute position of
+            // 'end', whichever is lowest
             blockEnd = Math.min(blockEnd, region[0] - 1);
 
-            int[] contig = new int[] { blockStart, blockEnd };
-            vcontigs.add(contig);
+            vcontigs.add(new int[] { blockStart, blockEnd });
 
             visSoFar += blockEnd - blockStart + 1;
+
+            // next visible region starts after this hidden region
             blockStart = region[1] + 1;
 
             hiddenSoFar += region[1] - region[0] + 1;
+
+            // reset blockEnd to absolute position of 'end', assuming we've now
+            // passed all hidden regions before end
             blockEnd = end + hiddenSoFar;
 
             i++;
           }
           if (visSoFar < end - start)
           {
+            // the number of visible columns we've accounted for is less than
+            // the number specified by end-start; work out the end position of
+            // the last visible region
             blockEnd = blockStart + end - start - visSoFar;
-            int[] contig = new int[] { blockStart, blockEnd };
-            vcontigs.add(contig);
+            vcontigs.add(new int[] { blockStart, blockEnd });
 
+            // if the last visible region ends at the next hidden region, set
+            // endsAtHidden=true
             if (i < hiddenColumns.size()
                     && hiddenColumns.get(i)[0] - 1 == blockEnd)
             {
@@ -1993,8 +2004,7 @@ public class HiddenColumns
         else
         {
           // there are no hidden columns, return a single visible contig
-          int[] contig = new int[] { start, end };
-          vcontigs.add(contig);
+          vcontigs.add(new int[] { start, end });
           endsAtHidden = false;
         }
       } finally

diff --git a/src/jalview/renderer/ScaleRenderer.java b/src/jalview/renderer/ScaleRenderer.java
index d310705..6d4edd9 100644 (file)
--- a/src/jalview/renderer/ScaleRenderer.java
+++ b/src/jalview/renderer/ScaleRenderer.java
@@ -68,17 +68,23 @@ public class ScaleRenderer
     int scalestartx = (startx / 10) * 10;
 
     SequenceI refSeq = av.getAlignment().getSeqrep();
-    int refSp = 0, refStartI = 0, refEndI = -1;
+    int refSp = 0;
+    int refStartI = 0;
+    int refEndI = -1;
     if (refSeq != null)
     {
-      // find bounds and set origin appopriately
+      // find bounds and set origin appropriately
       // locate first visible position for this sequence
-      int[] refbounds = av.getAlignment().getHiddenColumns()
+      refSp = av.getAlignment().getHiddenColumns()
               .locateVisibleBoundsOfSequence(refSeq);
 
-      refSp = refbounds[0];
-      refStartI = refbounds[1];
-      refEndI = refbounds[2];
+      refStartI = refSeq.findIndex(refSeq.getStart()) - 1;
+
+      int seqlength = refSeq.getLength();
+      // get sequence position past the end of the sequence
+      int pastEndPos = refSeq.findPosition(seqlength + 1);
+      refEndI = refSeq.findIndex(pastEndPos - 1) - 1;
+
       scalestartx = refSp + ((scalestartx - refSp) / 10) * 10;
     }
 

diff --git a/test/jalview/datamodel/HiddenColumnsTest.java b/test/jalview/datamodel/HiddenColumnsTest.java
index 6f1260c..bc1e7b7 100644 (file)
--- a/test/jalview/datamodel/HiddenColumnsTest.java
+++ b/test/jalview/datamodel/HiddenColumnsTest.java
@@ -329,29 +329,17 @@ public class HiddenColumnsTest
     assertEquals(2, seq.findIndex(seq.getStart()));
 
     // no hidden columns
-    assertEquals(
-            Arrays.toString(new int[]
-    { seq.findIndex(seq.getStart()) - 1, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(seq.findIndex(seq.getStart()) - 1, cs.locateVisibleBoundsOfSequence(seq));
 
     // hidden column on gap after end of sequence - should not affect bounds
     colsel.hideSelectedColumns(13, al.getHiddenColumns());
-    assertEquals(
-            Arrays.toString(new int[]
-    { seq.findIndex(seq.getStart()) - 1, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(seq.findIndex(seq.getStart()) - 1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     // hidden column on gap before beginning of sequence - should vis bounds by
     // one
     colsel.hideSelectedColumns(0, al.getHiddenColumns());
-    assertEquals(
-            Arrays.toString(new int[]
-    { seq.findIndex(seq.getStart()) - 2, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(seq.findIndex(seq.getStart()) - 2,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     // hide columns around most of sequence - leave one residue remaining
@@ -361,191 +349,117 @@ public class HiddenColumnsTest
             cs.getVisibleSequenceStrings(0, 5, new SequenceI[]
     { seq })[0]);
 
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 1, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(1, cs.locateVisibleBoundsOfSequence(seq));
     cs.revealAllHiddenColumns(colsel);
 
     // hide whole sequence - should just get location of hidden region
     // containing sequence
     cs.hideColumns(1, 11);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 0, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 15);
-    assertEquals(Arrays
-            .toString(new int[]
-    { 0, seq.findIndex(seq.getStart()) - 1,
-        seq.findIndex(seq.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq)));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     SequenceI seq2 = new Sequence("RefSeq2", "-------A-SD-ASD--E---");
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(7, 17);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 0, seq2.findIndex(seq2.getStart()) - 1,
-        seq2.findIndex(seq2.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq2)));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq2));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(3, 17);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 0, seq2.findIndex(seq2.getStart()) - 1,
-        seq2.findIndex(seq2.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq2)));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq2));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(3, 19);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 0, seq2.findIndex(seq2.getStart()) - 1,
-        seq2.findIndex(seq2.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq2)));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq2));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 0);
-    int[] test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 1);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 1, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 2);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(1, 1);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 2, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(2,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(1, 2);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 1, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(1, 3);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 1, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 2);
     cs.hideColumns(5, 6);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 2);
     cs.hideColumns(5, 6);
     cs.hideColumns(9, 10);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 2);
     cs.hideColumns(7, 11);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(2, 4);
     cs.hideColumns(7, 11);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 1, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(2, 4);
     cs.hideColumns(7, 12);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 1, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(1,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(1, 11);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 12);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 4);
     cs.hideColumns(6, 12);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 1);
     cs.hideColumns(3, 12);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 0, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(0,cs.locateVisibleBoundsOfSequence(seq));
 
-    // These tests cover different behaviour to original
-    // locateVisibleBoundsOfSequence
-    // Previously first values of each were 3,9 and 6 respectively.
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(3, 14);
     cs.hideColumns(17, 19);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 3, seq2.findIndex(seq2.getStart()) - 1,
-        seq2.findIndex(seq2.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq2)));
+    assertEquals(3,cs.locateVisibleBoundsOfSequence(seq2));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(3, 7);
     cs.hideColumns(9, 14);
     cs.hideColumns(17, 19);
-    assertEquals(
-            Arrays.toString(
-                    new int[]
-            { 9, seq2.findIndex(seq2.getStart()) - 1,
-        seq2.findIndex(seq2.getEnd()) - 1 }),
-            Arrays.toString(cs.locateVisibleBoundsOfSequence(seq2)));
+    assertEquals(9,cs.locateVisibleBoundsOfSequence(seq2));
 
     cs.revealAllHiddenColumns(colsel);
     cs.hideColumns(0, 1);
     cs.hideColumns(3, 4);
     cs.hideColumns(6, 8);
     cs.hideColumns(10, 12);
-    test = cs.locateVisibleBoundsOfSequence(seq);
-    assertEquals(Arrays.toString(new int[] { 6, 1, 11 }),
-            Arrays.toString(test));
+    assertEquals(6, cs.locateVisibleBoundsOfSequence(seq));
 
   }
 
@@ -560,7 +474,6 @@ public class HiddenColumnsTest
     cs.hideInsertionsFor(al.getSequenceAt(0));
     assertEquals("G", ""
             + al.getSequenceAt(0).getCharAt(cs.adjustForHiddenColumns(9)));
-
   }
 
   @Test(groups = { "Functional" })