JAL-2295 additional tests, stronger sanitisation of attribute name

diff --git a/src/jalview/ext/rbvi/chimera/ChimeraCommands.java b/src/jalview/ext/rbvi/chimera/ChimeraCommands.java
index a5ee933..9ef89fd 100644 (file)
--- a/src/jalview/ext/rbvi/chimera/ChimeraCommands.java
+++ b/src/jalview/ext/rbvi/chimera/ChimeraCommands.java
@@ -49,12 +49,18 @@ import MCview.PDBChain;
 public class ChimeraCommands
 {
 
+  private static final String NAMESPACE_PREFIX = "jv_";
+
   /**
-   * utility to construct the commands to colour chains by the given alignment
-   * for passing to Chimera
-   * 
-   * @returns Object[] { Object[] { <model being coloured>,
+   * Constructs Chimera commands to colour residues as per the Jalview alignment
    * 
+   * @param ssm
+   * @param files
+   * @param sequence
+   * @param sr
+   * @param fr
+   * @param alignment
+   * @return
    */
   public static StructureMappingcommandSet getColourBySequenceCommand(
           StructureSelectionManager ssm, String[] files,
@@ -286,8 +292,9 @@ public class ChimeraCommands
   }
 
   /**
-   * Constructs and returns a set of Chimera commands to set attributes on
-   * residues corresponding to features in Jalview
+   * Constructs and returns Chimera commands to set attributes on residues
+   * corresponding to features in Jalview. Attribute names are the Jalview
+   * feature type, with a "jv_" prefix.
    * 
    * @param ssm
    * @param files
@@ -403,7 +410,7 @@ public class ChimeraCommands
       if (!mappedRanges.isEmpty())
       {
         String value = sf.getDescription();
-        if (value == null)
+        if (value == null || value.length() == 0)
         {
           value = type;
         }
@@ -460,8 +467,7 @@ public class ChimeraCommands
     List<String> commands = new ArrayList<String>();
     for (String featureType : featureMap.keySet())
     {
-      String attributeName = "jv_"
-              + featureType.replace(" ", "_").replace("-", "_");
+      String attributeName = makeAttributeName(featureType);
 
       /*
        * clear down existing attributes for this feature
@@ -487,4 +493,38 @@ public class ChimeraCommands
     return commands;
   }
 
+  /**
+   * Makes a prefixed and valid Chimera attribute name. A jv_ prefix is applied
+   * for a 'Jalview' namespace, and any non-alphanumeric character is converted
+   * to an underscore.
+   * 
+   * @param featureType
+   * @return <pre>
+   * @see https://www.cgl.ucsf.edu/chimera/current/docs/UsersGuide/midas/setattr.html
+   * </pre>
+   */
+  protected static String makeAttributeName(String featureType)
+  {
+    StringBuilder sb = new StringBuilder();
+    if (featureType != null)
+    {
+      for (char c : featureType.toCharArray())
+      {
+        sb.append(Character.isLetterOrDigit(c) ? c : '_');
+      }
+    }
+    String attName = NAMESPACE_PREFIX + sb.toString();
+
+    /*
+     * Chimera treats an attribute name ending in 'color' as colour-valued;
+     * Jalview doesn't, so prevent this by appending an underscore
+     */
+    if (attName.toUpperCase().endsWith("COLOR"))
+    {
+      attName += "_";
+    }
+
+    return attName;
+  }
+
 }

diff --git a/test/jalview/ext/rbvi/chimera/ChimeraCommandsTest.java b/test/jalview/ext/rbvi/chimera/ChimeraCommandsTest.java
index b534b4f..f24ff22 100644 (file)
--- a/test/jalview/ext/rbvi/chimera/ChimeraCommandsTest.java
+++ b/test/jalview/ext/rbvi/chimera/ChimeraCommandsTest.java
@@ -20,10 +20,13 @@
  */
 package jalview.ext.rbvi.chimera;
 
-import static org.testng.AssertJUnit.assertEquals;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
 
 import java.awt.Color;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.testng.annotations.Test;
@@ -49,7 +52,89 @@ public class ChimeraCommandsTest
     // they were added; within colour, by model, by chain, ranges in start order
     String command = ChimeraCommands.buildColourCommands(map).get(0);
     assertEquals(
-            "color #0000ff #0:2-5.A,9-23.A,7.B|#1:1.A,4-7.B; color #ffff00 #1:3-5.A,8.A; color #ff0000 #0:3-9.A",
-            command);
+            command,
+            "color #0000ff #0:2-5.A,9-23.A,7.B|#1:1.A,4-7.B; color #ffff00 #1:3-5.A,8.A; color #ff0000 #0:3-9.A");
+  }
+
+  @Test(groups = { "Functional" })
+  public void testBuildSetAttributeCommands()
+  {
+    /*
+     * make a map of { featureType, {featureValue, {residue range specification } } }
+     */
+    Map<String, Map<Object, AtomSpecModel>> featuresMap = new LinkedHashMap<String, Map<Object, AtomSpecModel>>();
+    Map<Object, AtomSpecModel> featureValues = new HashMap<Object, AtomSpecModel>();
+    
+    /*
+     * start with just one feature/value...
+     */
+    featuresMap.put("chain", featureValues);
+    ChimeraCommands.addRange(featureValues, "X", 0, 8, 20, "A");
+  
+    List<String> commands = ChimeraCommands
+            .buildSetAttributeCommands(featuresMap);
+    assertEquals(1, commands.size());
+
+    /*
+     * feature name gets a jv_ namespace prefix
+     * feature value is quoted in case it contains spaces
+     */
+    assertEquals(commands.get(0), "setattr r jv_chain \"X\" #0:8-20.A");
+
+    // add same feature value, overlapping range
+    ChimeraCommands.addRange(featureValues, "X", 0, 3, 9, "A");
+    // same feature value, contiguous range
+    ChimeraCommands.addRange(featureValues, "X", 0, 21, 25, "A");
+    commands = ChimeraCommands.buildSetAttributeCommands(featuresMap);
+    assertEquals(1, commands.size());
+    assertEquals(commands.get(0), "setattr r jv_chain \"X\" #0:3-25.A");
+
+    // same feature value and model, different chain
+    ChimeraCommands.addRange(featureValues, "X", 0, 21, 25, "B");
+    // same feature value and chain, different model
+    ChimeraCommands.addRange(featureValues, "X", 1, 26, 30, "A");
+    commands = ChimeraCommands.buildSetAttributeCommands(featuresMap);
+    assertEquals(1, commands.size());
+    assertEquals(commands.get(0),
+            "setattr r jv_chain \"X\" #0:3-25.A,21-25.B|#1:26-30.A");
+
+    // same feature, different value
+    ChimeraCommands.addRange(featureValues, "Y", 0, 40, 50, "A");
+    commands = ChimeraCommands.buildSetAttributeCommands(featuresMap);
+    assertEquals(2, commands.size());
+    // commands are ordered by feature type but not by value
+    // so use contains to test for the expected command:
+    assertTrue(commands
+            .contains("setattr r jv_chain \"X\" #0:3-25.A,21-25.B|#1:26-30.A"));
+    assertTrue(commands.contains("setattr r jv_chain \"Y\" #0:40-50.A"));
+
+    featuresMap.clear();
+    featureValues.clear();
+    featuresMap.put("side-chain binding!", featureValues);
+    ChimeraCommands.addRange(featureValues, "metal ion!", 0, 7, 15, "A");
+    // feature names are sanitised to change space or hyphen to underscore
+    commands = ChimeraCommands.buildSetAttributeCommands(featuresMap);
+    assertTrue(commands
+            .contains("setattr r jv_side_chain_binding_ \"metal ion!\" #0:7-15.A"));
+  }
+
+  /**
+   * Tests for the method that prefixes and sanitises a feature name so it can
+   * be used as a valid, namespaced attribute name in Chimera
+   */
+  @Test(groups = { "Functional" })
+  public void testMakeAttributeName()
+  {
+    assertEquals(ChimeraCommands.makeAttributeName(null), "jv_");
+    assertEquals(ChimeraCommands.makeAttributeName(""), "jv_");
+    assertEquals(ChimeraCommands.makeAttributeName("helix"), "jv_helix");
+    assertEquals(ChimeraCommands.makeAttributeName("Hello World 24"),
+            "jv_Hello_World_24");
+    assertEquals(
+            ChimeraCommands.makeAttributeName("!this is-a_very*{odd(name"),
+            "jv__this_is_a_very__odd_name");
+    // name ending in color gets underscore appended
+    assertEquals(ChimeraCommands.makeAttributeName("helixColor"),
+            "jv_helixColor_");
   }
 }