JAL-3779 apply properties to all groups with given name, not just first

diff --git a/src/jalview/io/AnnotationFile.java b/src/jalview/io/AnnotationFile.java
index 2182b24..934be41 100755 (executable)
--- a/src/jalview/io/AnnotationFile.java
+++ b/src/jalview/io/AnnotationFile.java
@@ -25,6 +25,7 @@ import java.io.BufferedReader;
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
@@ -1086,8 +1087,8 @@ public class AnnotationFile
               if (rowset != null && rowset.size() > 0)
               {
                 AlignmentAnnotation alan = null;
-                for (int elm = 0, elmSize = rowset
-                        .size(); elm < elmSize; elm++)
+                for (int elm = 0,
+                        elmSize = rowset.size(); elm < elmSize; elm++)
                 {
                   alan = (AlignmentAnnotation) rowset.elementAt(elm);
                   alan.groupRef = theGroup;
@@ -1549,110 +1550,120 @@ public class AnnotationFile
     }
 
     String name = st.nextToken();
-    SequenceGroup sg = null;
-    for (SequenceGroup _sg : al.getGroups())
+
+    Map<String, String> properties = new HashMap<>();
+    while (st.hasMoreTokens())
     {
-      if ((sg = _sg).getName().equals(name))
-      {
-        break;
-      }
-      else
+      String keyValue = st.nextToken();
+      String key = keyValue.substring(0, keyValue.indexOf("="));
+      String value = keyValue.substring(keyValue.indexOf("=") + 1);
+      properties.put(key, value);
+    }
+
+    for (SequenceGroup sg : al.getGroups())
+    {
+      if (sg.getName().equals(name))
       {
-        sg = null;
+        addProperties(sg, properties, al);
       }
     }
+  }
 
-    if (sg != null)
+  /**
+   * Helper method that applies any specified properties to a SequenceGroup
+   * 
+   * @param sg
+   * @param properties
+   * @param al
+   */
+  private void addProperties(SequenceGroup sg,
+          Map<String, String> properties, AlignmentI al)
+  {
+    ColourSchemeI def = sg.getColourScheme();
+    for (String key : properties.keySet())
     {
-      String keyValue, key, value;
-      ColourSchemeI def = sg.getColourScheme();
-      while (st.hasMoreTokens())
+      String value = properties.get(key);
+      if (key.equalsIgnoreCase("description"))
       {
-        keyValue = st.nextToken();
-        key = keyValue.substring(0, keyValue.indexOf("="));
-        value = keyValue.substring(keyValue.indexOf("=") + 1);
-
-        if (key.equalsIgnoreCase("description"))
-        {
-          sg.setDescription(value);
-        }
-        else if (key.equalsIgnoreCase("colour"))
-        {
-          // TODO need to notify colourscheme of view reference once it is
-          // available
-          sg.cs.setColourScheme(
-                  ColourSchemeProperty.getColourScheme(null, al, value));
-        }
-        else if (key.equalsIgnoreCase("pidThreshold"))
-        {
-          sg.cs.setThreshold(Integer.parseInt(value), true);
+        sg.setDescription(value);
+      }
+      else if (key.equalsIgnoreCase("colour"))
+      {
+        // TODO need to notify colourscheme of view reference once it is
+        // available
+        sg.cs.setColourScheme(
+                ColourSchemeProperty.getColourScheme(null, al, value));
+      }
+      else if (key.equalsIgnoreCase("pidThreshold"))
+      {
+        sg.cs.setThreshold(Integer.parseInt(value), true);
 
-        }
-        else if (key.equalsIgnoreCase("consThreshold"))
-        {
-          sg.cs.setConservationInc(Integer.parseInt(value));
-          Conservation c = new Conservation("Group", sg.getSequences(null),
-                  sg.getStartRes(), sg.getEndRes() + 1);
+      }
+      else if (key.equalsIgnoreCase("consThreshold"))
+      {
+        sg.cs.setConservationInc(Integer.parseInt(value));
+        Conservation c = new Conservation("Group", sg.getSequences(null),
+                sg.getStartRes(), sg.getEndRes() + 1);
 
-          c.calculate();
-          c.verdict(false, 25); // TODO: refer to conservation percent threshold
+        c.calculate();
+        c.verdict(false, 25); // TODO: refer to conservation percent threshold
 
-          sg.cs.setConservation(c);
+        sg.cs.setConservation(c);
 
-        }
-        else if (key.equalsIgnoreCase("outlineColour"))
-        {
-          sg.setOutlineColour(ColorUtils.parseColourString(value));
-        }
-        else if (key.equalsIgnoreCase("displayBoxes"))
-        {
-          sg.setDisplayBoxes(Boolean.valueOf(value).booleanValue());
-        }
-        else if (key.equalsIgnoreCase("showUnconserved"))
-        {
-          sg.setShowNonconserved(Boolean.valueOf(value).booleanValue());
-        }
-        else if (key.equalsIgnoreCase("displayText"))
-        {
-          sg.setDisplayText(Boolean.valueOf(value).booleanValue());
-        }
-        else if (key.equalsIgnoreCase("colourText"))
-        {
-          sg.setColourText(Boolean.valueOf(value).booleanValue());
-        }
-        else if (key.equalsIgnoreCase("textCol1"))
-        {
-          sg.textColour = ColorUtils.parseColourString(value);
-        }
-        else if (key.equalsIgnoreCase("textCol2"))
-        {
-          sg.textColour2 = ColorUtils.parseColourString(value);
-        }
-        else if (key.equalsIgnoreCase("textColThreshold"))
-        {
-          sg.thresholdTextColour = Integer.parseInt(value);
-        }
-        else if (key.equalsIgnoreCase("idColour"))
-        {
-          Color idColour = ColorUtils.parseColourString(value);
-          sg.setIdColour(idColour == null ? Color.black : idColour);
-        }
-        else if (key.equalsIgnoreCase("hide"))
-        {
-          // see bug https://mantis.lifesci.dundee.ac.uk/view.php?id=25847
-          sg.setHidereps(true);
-        }
-        else if (key.equalsIgnoreCase("hidecols"))
-        {
-          // see bug https://mantis.lifesci.dundee.ac.uk/view.php?id=25847
-          sg.setHideCols(true);
-        }
-        sg.recalcConservation();
       }
-      if (sg.getColourScheme() == null)
+      else if (key.equalsIgnoreCase("outlineColour"))
       {
-        sg.setColourScheme(def);
+        sg.setOutlineColour(ColorUtils.parseColourString(value));
       }
+      else if (key.equalsIgnoreCase("displayBoxes"))
+      {
+        sg.setDisplayBoxes(Boolean.valueOf(value).booleanValue());
+      }
+      else if (key.equalsIgnoreCase("showUnconserved"))
+      {
+        sg.setShowNonconserved(Boolean.valueOf(value).booleanValue());
+      }
+      else if (key.equalsIgnoreCase("displayText"))
+      {
+        sg.setDisplayText(Boolean.valueOf(value).booleanValue());
+      }
+      else if (key.equalsIgnoreCase("colourText"))
+      {
+        sg.setColourText(Boolean.valueOf(value).booleanValue());
+      }
+      else if (key.equalsIgnoreCase("textCol1"))
+      {
+        sg.textColour = ColorUtils.parseColourString(value);
+      }
+      else if (key.equalsIgnoreCase("textCol2"))
+      {
+        sg.textColour2 = ColorUtils.parseColourString(value);
+      }
+      else if (key.equalsIgnoreCase("textColThreshold"))
+      {
+        sg.thresholdTextColour = Integer.parseInt(value);
+      }
+      else if (key.equalsIgnoreCase("idColour"))
+      {
+        Color idColour = ColorUtils.parseColourString(value);
+        sg.setIdColour(idColour == null ? Color.black : idColour);
+      }
+      else if (key.equalsIgnoreCase("hide"))
+      {
+        // see bug https://mantis.lifesci.dundee.ac.uk/view.php?id=25847
+        sg.setHidereps(true);
+      }
+      else if (key.equalsIgnoreCase("hidecols"))
+      {
+        // see bug https://mantis.lifesci.dundee.ac.uk/view.php?id=25847
+        sg.setHideCols(true);
+      }
+      sg.recalcConservation();
+    }
+
+    if (sg.getColourScheme() == null)
+    {
+      sg.setColourScheme(def);
     }
   }
 

diff --git a/test/jalview/io/AnnotationFileIOTest.java b/test/jalview/io/AnnotationFileIOTest.java
index 05ce22d..9c4be2d 100644 (file)
--- a/test/jalview/io/AnnotationFileIOTest.java
+++ b/test/jalview/io/AnnotationFileIOTest.java
@@ -20,21 +20,26 @@
  */
 package jalview.io;
 
+import static org.testng.AssertJUnit.assertEquals;
 import static org.testng.AssertJUnit.assertNotNull;
 import static org.testng.AssertJUnit.assertTrue;
 
-import jalview.datamodel.AlignmentI;
-import jalview.datamodel.HiddenColumns;
-import jalview.gui.JvOptionPane;
-import jalview.io.AnnotationFile.ViewDef;
-
+import java.awt.Color;
 import java.io.File;
 import java.util.Hashtable;
+import java.util.List;
 
 import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import jalview.datamodel.AlignmentI;
+import jalview.datamodel.HiddenColumns;
+import jalview.datamodel.SequenceGroup;
+import jalview.gui.AlignFrame;
+import jalview.gui.JvOptionPane;
+import jalview.io.AnnotationFile.ViewDef;
+
 public class AnnotationFileIOTest
 {
 
@@ -165,4 +170,45 @@ public class AnnotationFileIOTest
             + "\nCouldn't complete Annotation file roundtrip input/output/input test for '"
             + annotFile + "'.");
   }
+  
+  @Test(groups="Functional")
+  public void testAnnotateAlignmentView()
+  {
+    long t1 = System.currentTimeMillis();
+    /*
+     * JAL-3779 test multiple groups of the same name get annotated
+     */
+    AlignFrame af = new FileLoader().LoadFileWaitTillLoaded(
+            ">Seq1\nQRSIL\n>Seq2\nFTHND\n>Seq3\nRPVSL\n",
+            DataSourceType.PASTE);
+    long t2 = System.currentTimeMillis();
+    System.err.println("t0: " + (t2 - t1));
+    // seq1 and seq3 are in distinct groups both named Group1
+    String annotationFile = "JALVIEW_ANNOTATION\nSEQUENCE_GROUP\tGroup1\t*\t*\t1\n"
+            + "SEQUENCE_GROUP\tGroup2\t*\t*\t2\n"
+            + "SEQUENCE_GROUP\tGroup1\t*\t*\t3\n"
+            + "PROPERTIES\tGroup1\toutlineColour=blue\tidColour=red\n";
+    new AnnotationFile().annotateAlignmentView(af.getViewport(), annotationFile, DataSourceType.PASTE);
+    
+    AlignmentI al = af.getViewport().getAlignment();
+    List<SequenceGroup> groups = al.getGroups();
+    assertEquals(3, groups.size());
+    SequenceGroup sg = groups.get(0);
+    assertEquals("Group1", sg.getName());
+    assertTrue(sg.contains(al.getSequenceAt(0)));
+    assertEquals(Color.BLUE, sg.getOutlineColour());
+    assertEquals(Color.RED, sg.getIdColour());
+    sg = groups.get(1);
+    assertEquals("Group2", sg.getName());
+    assertTrue(sg.contains(al.getSequenceAt(1)));
+    
+    /*
+     * the bug fix: a second group of the same name is also given properties
+     */
+    sg = groups.get(2);
+    assertEquals("Group1", sg.getName());
+    assertTrue(sg.contains(al.getSequenceAt(2)));
+    assertEquals(Color.BLUE, sg.getOutlineColour());
+    assertEquals(Color.RED, sg.getIdColour());
+  }
 }