JAL-3518 pull up closeViewer() and external viewer process monitor
[jalview.git] / src / jalview / ext / rbvi / chimera / JalviewChimeraBinding.java
index d7bcc66..bc4eef4 100644 (file)
  */
 package jalview.ext.rbvi.chimera;
 
-import jalview.api.AlignmentViewPanel;
-import jalview.api.structures.JalviewStructureDisplayI;
-import jalview.bin.Cache;
-import jalview.datamodel.AlignmentI;
-import jalview.datamodel.HiddenColumns;
-import jalview.datamodel.PDBEntry;
-import jalview.datamodel.SearchResultMatchI;
-import jalview.datamodel.SearchResultsI;
-import jalview.datamodel.SequenceFeature;
-import jalview.datamodel.SequenceI;
-import jalview.gui.Preferences;
-import jalview.gui.StructureViewer.ViewerType;
-import jalview.httpserver.AbstractRequestHandler;
-import jalview.io.DataSourceType;
-import jalview.structure.AtomSpec;
-import jalview.structure.StructureMappingcommandSet;
-import jalview.structure.StructureSelectionManager;
-import jalview.structures.models.AAStructureBindingModel;
-import jalview.util.MessageManager;
-
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.net.BindException;
 import java.util.ArrayList;
-import java.util.BitSet;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -57,19 +36,29 @@ import ext.edu.ucsf.rbvi.strucviz2.ChimeraManager;
 import ext.edu.ucsf.rbvi.strucviz2.ChimeraModel;
 import ext.edu.ucsf.rbvi.strucviz2.StructureManager;
 import ext.edu.ucsf.rbvi.strucviz2.StructureManager.ModelType;
+import jalview.api.AlignmentViewPanel;
+import jalview.bin.Cache;
+import jalview.datamodel.AlignmentI;
+import jalview.datamodel.PDBEntry;
+import jalview.datamodel.SearchResultMatchI;
+import jalview.datamodel.SearchResultsI;
+import jalview.datamodel.SequenceFeature;
+import jalview.datamodel.SequenceI;
+import jalview.gui.StructureViewer.ViewerType;
+import jalview.httpserver.AbstractRequestHandler;
+import jalview.io.DataSourceType;
+import jalview.structure.AtomSpec;
+import jalview.structure.AtomSpecModel;
+import jalview.structure.StructureCommand;
+import jalview.structure.StructureCommandI;
+import jalview.structure.StructureSelectionManager;
+import jalview.structures.models.AAStructureBindingModel;
 
 public abstract class JalviewChimeraBinding extends AAStructureBindingModel
 {
-  public static final String CHIMERA_FEATURE_GROUP = "Chimera";
-
-  // Chimera clause to exclude alternate locations in atom selection
-  private static final String NO_ALTLOCS = "&~@.B-Z&~@.2-9";
-
-  private static final boolean debug = false;
+  public static final String CHIMERA_SESSION_EXTENSION = ".py";
 
-  private static final String PHOSPHORUS = "P";
-
-  private static final String ALPHACARBON = "CA";
+  public static final String CHIMERA_FEATURE_GROUP = "Chimera";
 
   /*
    * Object through which we talk to Chimera
@@ -82,34 +71,12 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   private AbstractRequestHandler chimeraListener;
 
   /*
-   * set if chimera state is being restored from some source - instructs binding
-   * not to apply default display style when structure set is updated for first
-   * time.
-   */
-  private boolean loadingFromArchive = false;
-
-  /*
-   * flag to indicate if the Chimera viewer should ignore sequence colouring
-   * events from the structure manager because the GUI is still setting up
-   */
-  private boolean loadingFinished = true;
-
-  /*
    * Map of ChimeraModel objects keyed by PDB full local file name
    */
-  private Map<String, List<ChimeraModel>> chimeraMaps = new LinkedHashMap<>();
+  protected Map<String, List<ChimeraModel>> chimeraMaps = new LinkedHashMap<>();
 
   String lastHighlightCommand;
 
-  /*
-   * incremented every time a load notification is successfully handled -
-   * lightweight mechanism for other threads to detect when they can start
-   * referring to new structures.
-   */
-  private long loadNotifiesHandled = 0;
-
-  private Thread chimeraMonitor;
-
   /**
    * Open a PDB structure file in Chimera and set up mappings from Jalview.
    * 
@@ -147,34 +114,7 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
       if (!alreadyOpen)
       {
         chimeraManager.openModel(file, pe.getId(), ModelType.PDB_MODEL);
-        if (chimeraManager.isChimeraX())
-        {
-          /*
-           * ChimeraX hack: force chimera model name to pdbId
-           */
-          int modelNumber = chimeraMaps.size() + 1;
-          String command = "setattr #" + modelNumber + " models name "
-                  + pe.getId();
-          executeCommand(command, false);
-          modelsToMap.add(new ChimeraModel(pe.getId(), ModelType.PDB_MODEL,
-                  modelNumber, 0));
-        }
-        else
-        {
-          /*
-           * Chimera: query for actual models and find the one with
-           * matching model name - set in viewer.openModel()
-           */
-          List<ChimeraModel> newList = chimeraManager.getModelList();
-          // JAL-1728 newList.removeAll(oldList) does not work
-          for (ChimeraModel cm : newList)
-          {
-            if (cm.getModelName().equals(pe.getId()))
-            {
-              modelsToMap.add(cm);
-            }
-          }
-        }
+        addChimeraModel(pe, modelsToMap);
       }
 
       chimeraMaps.put(file, modelsToMap);
@@ -194,6 +134,31 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   }
 
   /**
+   * Adds the ChimeraModel corresponding to the given PDBEntry, based on model
+   * name matching PDB id
+   * 
+   * @param pe
+   * @param modelsToMap
+   */
+  protected void addChimeraModel(PDBEntry pe,
+          List<ChimeraModel> modelsToMap)
+  {
+    /*
+     * Chimera: query for actual models and find the one with
+     * matching model name - already set in viewer.openModel()
+     */
+    List<ChimeraModel> newList = chimeraManager.getModelList();
+    // JAL-1728 newList.removeAll(oldList) does not work
+    for (ChimeraModel cm : newList)
+    {
+      if (cm.getModelName().equals(pe.getId()))
+      {
+        modelsToMap.add(cm);
+      }
+    }
+  }
+
+  /**
    * Constructor
    * 
    * @param ssm
@@ -207,40 +172,14 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   {
     super(ssm, pdbentry, sequenceIs, protocol);
     chimeraManager = new ChimeraManager(new StructureManager(true));
-    String viewerType = Cache.getProperty(Preferences.STRUCTURE_DISPLAY);
-    chimeraManager.setChimeraX(ViewerType.CHIMERAX.name().equals(viewerType));
+    chimeraManager.setChimeraX(ViewerType.CHIMERAX.equals(getViewerType()));
     setStructureCommands(new ChimeraCommands());
   }
 
-  /**
-   * Starts a thread that waits for the Chimera process to finish, so that we can
-   * then close the associated resources. This avoids leaving orphaned Chimera
-   * viewer panels in Jalview if the user closes Chimera.
-   */
-  protected void startChimeraProcessMonitor()
+  @Override
+  protected ViewerType getViewerType()
   {
-    final Process p = chimeraManager.getChimeraProcess();
-    chimeraMonitor = new Thread(new Runnable()
-    {
-
-      @Override
-      public void run()
-      {
-        try
-        {
-          p.waitFor();
-          JalviewStructureDisplayI display = getViewer();
-          if (display != null)
-          {
-            display.closeViewer(false);
-          }
-        } catch (InterruptedException e)
-        {
-          // exit thread if Chimera Viewer is closed in Jalview
-        }
-      }
-    });
-    chimeraMonitor.start();
+    return ViewerType.CHIMERA;
   }
 
   /**
@@ -264,300 +203,17 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
    * Close down the Jalview viewer and listener, and (optionally) the associated
    * Chimera window.
    */
+  @Override
   public void closeViewer(boolean closeChimera)
   {
-    getSsm().removeStructureViewerListener(this, this.getStructureFiles());
-    if (closeChimera)
-    {
-      chimeraManager.exitChimera();
-    }
+    super.closeViewer(closeChimera);
     if (this.chimeraListener != null)
     {
       chimeraListener.shutdown();
       chimeraListener = null;
     }
+    chimeraManager.clearOnChimeraExit();
     chimeraManager = null;
-
-    if (chimeraMonitor != null)
-    {
-      chimeraMonitor.interrupt();
-    }
-    releaseUIResources();
-  }
-
-  /**
-   * {@inheritDoc}
-   */
-  @Override
-  public String superposeStructures(AlignmentI[] _alignment,
-          int[] _refStructure, HiddenColumns[] _hiddenCols)
-  {
-    StringBuilder allComs = new StringBuilder(128);
-    String[] files = getStructureFiles();
-
-    if (!waitForFileLoad(files))
-    {
-      return null;
-    }
-
-    refreshPdbEntries();
-    StringBuilder selectioncom = new StringBuilder(256);
-    boolean chimeraX = chimeraManager.isChimeraX();
-    for (int a = 0; a < _alignment.length; a++)
-    {
-      int refStructure = _refStructure[a];
-      AlignmentI alignment = _alignment[a];
-      HiddenColumns hiddenCols = _hiddenCols[a];
-
-      if (refStructure >= files.length)
-      {
-        System.err.println("Ignoring invalid reference structure value "
-                + refStructure);
-        refStructure = -1;
-      }
-
-      /*
-       * 'matched' bit i will be set for visible alignment columns i where
-       * all sequences have a residue with a mapping to the PDB structure
-       */
-      BitSet matched = new BitSet();
-      for (int m = 0; m < alignment.getWidth(); m++)
-      {
-        if (hiddenCols == null || hiddenCols.isVisible(m))
-        {
-          matched.set(m);
-        }
-      }
-
-      SuperposeData[] structures = new SuperposeData[files.length];
-      for (int f = 0; f < files.length; f++)
-      {
-        structures[f] = new SuperposeData(alignment.getWidth());
-      }
-
-      /*
-       * Calculate the superposable alignment columns ('matched'), and the
-       * corresponding structure residue positions (structures.pdbResNo)
-       */
-      int candidateRefStructure = findSuperposableResidues(alignment,
-              matched, structures);
-      if (refStructure < 0)
-      {
-        /*
-         * If no reference structure was specified, pick the first one that has
-         * a mapping in the alignment
-         */
-        refStructure = candidateRefStructure;
-      }
-
-      int nmatched = matched.cardinality();
-      if (nmatched < 4)
-      {
-        return MessageManager.formatMessage("label.insufficient_residues",
-                nmatched);
-      }
-
-      /*
-       * Generate select statements to select regions to superimpose structures
-       */
-      String[] selcom = new String[files.length];
-      for (int pdbfnum = 0; pdbfnum < files.length; pdbfnum++)
-      {
-        final int modelNo = pdbfnum + (chimeraX ? 1 : 0);
-        // todo correct resolution to model number
-        String chainCd = "." + structures[pdbfnum].chain;
-        int lpos = -1;
-        boolean run = false;
-        StringBuilder molsel = new StringBuilder();
-        if (chimeraX)
-        {
-          molsel.append("/" + structures[pdbfnum].chain + ":");
-        }
-
-        int nextColumnMatch = matched.nextSetBit(0);
-        while (nextColumnMatch != -1)
-        {
-          int pdbResNum = structures[pdbfnum].pdbResNo[nextColumnMatch];
-          if (lpos != pdbResNum - 1)
-          {
-            /*
-             * discontiguous - append last residue now
-             */
-            if (lpos != -1)
-            {
-              molsel.append(String.valueOf(lpos));
-              if (!chimeraX)
-              {
-                molsel.append(chainCd);
-              }
-              molsel.append(",");
-            }
-            run = false;
-          }
-          else
-          {
-            /*
-             * extending a contiguous run
-             */
-            if (!run)
-            {
-              /*
-               * start the range selection
-               */
-              molsel.append(String.valueOf(lpos));
-              molsel.append("-");
-            }
-            run = true;
-          }
-          lpos = pdbResNum;
-          nextColumnMatch = matched.nextSetBit(nextColumnMatch + 1);
-        }
-
-        /*
-         * and terminate final selection
-         */
-        if (lpos != -1)
-        {
-          molsel.append(String.valueOf(lpos));
-          if (!chimeraX)
-          {
-            molsel.append(chainCd);
-          }
-        }
-        if (molsel.length() > 1)
-        {
-          selcom[pdbfnum] = molsel.toString();
-          selectioncom.append("#").append(String.valueOf(modelNo));
-          if (!chimeraX)
-          {
-            selectioncom.append(":");
-          }
-          selectioncom.append(selcom[pdbfnum]);
-          // selectioncom.append(" ");
-          if (pdbfnum < files.length - 1)
-          {
-            selectioncom.append("|");
-          }
-        }
-        else
-        {
-          selcom[pdbfnum] = null;
-        }
-      }
-
-      StringBuilder command = new StringBuilder(256);
-      for (int pdbfnum = 0; pdbfnum < files.length; pdbfnum++)
-      {
-        final int modelNo = pdbfnum + (chimeraX ? 1 : 0);
-        if (pdbfnum == refStructure || selcom[pdbfnum] == null
-                || selcom[refStructure] == null)
-        {
-          continue;
-        }
-        if (command.length() > 0)
-        {
-          command.append(";");
-        }
-
-        /*
-         * Form Chimera match command, from the 'new' structure to the
-         * 'reference' structure e.g. (50 residues, chain B/A, alphacarbons):
-         * 
-         * match #1:1-30.B,81-100.B@CA #0:21-40.A,61-90.A@CA
-         * 
-         * @see
-         * https://www.cgl.ucsf.edu/chimera/docs/UsersGuide/midas/match.html
-         */
-        command.append(chimeraX ? "align " : "match ");
-        command.append(getModelSpec(modelNo));
-        if (!chimeraX)
-        {
-          command.append(":");
-        }
-        command.append(selcom[pdbfnum]);
-        command.append("@").append(
-                structures[pdbfnum].isRna ? PHOSPHORUS : ALPHACARBON);
-        // JAL-1757 exclude alternate CA locations - ChimeraX syntax tbd
-        if (!chimeraX)
-        {
-          command.append(NO_ALTLOCS);
-        }
-        command.append(chimeraX ? " toAtoms " : " ")
-                .append(getModelSpec(refStructure + (chimeraX ? 1 : 0)));
-        if (!chimeraX)
-        {
-          command.append(":");
-        }
-        command.append(selcom[refStructure]);
-        command.append("@").append(
-                structures[refStructure].isRna ? PHOSPHORUS : ALPHACARBON);
-        if (!chimeraX)
-        {
-          command.append(NO_ALTLOCS);
-        }
-      }
-      if (selectioncom.length() > 0)
-      {
-        if (debug)
-        {
-          System.out.println("Select regions:\n" + selectioncom.toString());
-          System.out.println(
-                  "Superimpose command(s):\n" + command.toString());
-        }
-        // allComs.append("~display all; ");
-        // if (chimeraX)
-        // {
-        // allComs.append("show ").append(selectioncom.toString())
-        // .append(" pbonds");
-        // }
-        // else
-        // {
-        // allComs.append("chain @CA|P; ribbon ");
-        // allComs.append(selectioncom.toString());
-        // }
-        if (allComs.length() > 0) {
-          allComs.append(";");
-        }
-        allComs.append(command.toString());
-      }
-    }
-
-    String error = null;
-    if (selectioncom.length() > 0)
-    {
-      // TODO: visually distinguish regions that were superposed
-      if (selectioncom.substring(selectioncom.length() - 1).equals("|"))
-      {
-        selectioncom.setLength(selectioncom.length() - 1);
-      }
-      if (debug)
-      {
-        System.out.println("Select regions:\n" + selectioncom.toString());
-      }
-      allComs.append(";~display all; ");
-      if (chimeraX)
-      {
-        allComs.append("show @CA|P pbonds; show ")
-                .append(selectioncom.toString()).append(" ribbons; view");
-      }
-      else
-      {
-        allComs.append("chain @CA|P; ribbon ; focus");
-        allComs.append(selectioncom.toString());
-      }
-      // allComs.append("; ~display all; chain @CA|P; ribbon ")
-      // .append(selectioncom.toString()).append("; focus");
-      List<String> chimeraReplies = executeCommand(allComs.toString(),
-              true);
-      for (String reply : chimeraReplies)
-      {
-        if (reply.toLowerCase().contains("unequal numbers of atoms"))
-        {
-          error = reply;
-        }
-      }
-    }
-    return error;
   }
 
   /**
@@ -605,11 +261,10 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
       return true;
     }
 
-    boolean launched = chimeraManager.launchChimera(
-            StructureManager.getChimeraPaths(chimeraManager.isChimeraX()));
+    boolean launched = chimeraManager.launchChimera(getChimeraPaths());
     if (launched)
     {
-      startChimeraProcessMonitor();
+      startExternalViewerMonitor(chimeraManager.getChimeraProcess());
     }
     else
     {
@@ -619,27 +274,35 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   }
 
   /**
+   * Returns a list of candidate paths to the Chimera program executable
+   * 
+   * @return
+   */
+  protected List<String> getChimeraPaths()
+  {
+    return StructureManager.getChimeraPaths(false);
+  }
+
+  /**
    * Answers true if the Chimera process is still running, false if ended or not
    * started.
    * 
    * @return
    */
-  public boolean isChimeraRunning()
+  @Override
+  public boolean isViewerRunning()
   {
     return chimeraManager.isChimeraLaunched();
   }
 
   /**
    * Send a command to Chimera, and optionally log and return any responses.
-   * <p>
-   * Does nothing, and returns null, if the command is the same as the last one
-   * sent [why?].
    * 
    * @param command
    * @param getResponse
    */
   @Override
-  public List<String> executeCommand(final String command,
+  public List<String> executeCommand(final StructureCommandI command,
           boolean getResponse)
   {
     if (chimeraManager == null || command == null)
@@ -648,79 +311,20 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
       return null;
     }
     List<String> reply = null;
-    viewerCommandHistory(false);
-    if (true /*lastCommand == null || !lastCommand.equals(command)*/)
+    // trim command or it may never find a match in the replyLog!!
+    String cmd = command.getCommand().trim();
+    List<String> lastReply = chimeraManager
+            .sendChimeraCommand(cmd, getResponse);
+    if (getResponse)
     {
-      // trim command or it may never find a match in the replyLog!!
-      List<String> lastReply = chimeraManager.sendChimeraCommand(command.trim(),
-              getResponse);
-      if (getResponse)
-      {
-        reply = lastReply;
-        if (debug)
-        {
-          log("Response from command ('" + command + "') was:\n"
-                  + lastReply);
-        }
-      }
+      reply = lastReply;
+      Cache.log.debug(
+              "Response from command ('" + cmd + "') was:\n" + lastReply);
     }
-    viewerCommandHistory(true);
 
     return reply;
   }
 
-  /**
-   * Send a Chimera command asynchronously in a new thread. If the progress
-   * message is not null, display this message while the command is executing.
-   * 
-   * @param command
-   * @param progressMsg
-   */
-  protected abstract void sendAsynchronousCommand(String command,
-          String progressMsg);
-
-  /**
-   * @param command
-   */
-  protected void executeWhenReady(String command)
-  {
-    waitForChimera();
-    executeCommand(command, false);
-    waitForChimera();
-  }
-
-  private void waitForChimera()
-  {
-    while (chimeraManager != null && chimeraManager.isBusy())
-    {
-      try
-      {
-        Thread.sleep(15);
-      } catch (InterruptedException q)
-      {
-      }
-    }
-  }
-
-  // End StructureListener
-  // //////////////////////////
-
-  /**
-   * instruct the Jalview binding to update the pdbentries vector if necessary
-   * prior to matching the viewer's contents to the list of structure files
-   * Jalview knows about.
-   */
-  public abstract void refreshPdbEntries();
-
-  /**
-   * map between index of model filename returned from getPdbFile and the first
-   * index of models from this file in the viewer. Note - this is not trimmed -
-   * use getPdbFile to get number of unique models.
-   */
-  private int _modelFileNameMap[];
-
-  // ////////////////////////////////
-  // /StructureListener
   @Override
   public synchronized String[] getStructureFiles()
   {
@@ -854,7 +458,7 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
         atomSpecs.add(spec);
       } catch (IllegalArgumentException e)
       {
-        System.err.println("Failed to parse atomspec: " + atomSpec);
+        Cache.log.error("Failed to parse atomspec: " + atomSpec);
       }
     }
     return atomSpecs;
@@ -889,103 +493,6 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
     System.err.println("## Chimera log: " + message);
   }
 
-  private void viewerCommandHistory(boolean enable)
-  {
-    // log("(Not yet implemented) History "
-    // + ((debug || enable) ? "on" : "off"));
-  }
-
-  public long getLoadNotifiesHandled()
-  {
-    return loadNotifiesHandled;
-  }
-
-  /**
-   * called when the binding thinks the UI needs to be refreshed after a Chimera
-   * state change. this could be because structures were loaded, or because an
-   * error has occurred.
-   */
-  public abstract void refreshGUI();
-
-  @Override
-  public void setLoadingFromArchive(boolean loadingFromArchive)
-  {
-    this.loadingFromArchive = loadingFromArchive;
-  }
-
-  /**
-   * 
-   * @return true if Chimeral is still restoring state or loading is still going
-   *         on (see setFinsihedLoadingFromArchive)
-   */
-  @Override
-  public boolean isLoadingFromArchive()
-  {
-    return loadingFromArchive && !loadingFinished;
-  }
-
-  /**
-   * modify flag which controls if sequence colouring events are honoured by the
-   * binding. Should be true for normal operation
-   * 
-   * @param finishedLoading
-   */
-  @Override
-  public void setFinishedLoadingFromArchive(boolean finishedLoading)
-  {
-    loadingFinished = finishedLoading;
-  }
-
-  /**
-   * Ask Chimera to save its session to the given file. Returns true if
-   * successful, else false.
-   * 
-   * @param filepath
-   * @return
-   */
-  public boolean saveSession(String filepath)
-  {
-    if (isChimeraRunning())
-    {
-      /*
-       * Chimera:  https://www.cgl.ucsf.edu/chimera/current/docs/UsersGuide/midas/save.html
-       * ChimeraX: https://www.cgl.ucsf.edu/chimerax/docs/user/commands/save.html
-       */
-      String command = isChimeraX() ? "save session " : "save ";
-      List<String> reply = chimeraManager.sendChimeraCommand(command + filepath,
-              true);
-      if (reply.contains("Session written"))
-      {
-        return true;
-      }
-      else
-      {
-        Cache.log
-                .error("Error saving Chimera session: " + reply.toString());
-      }
-    }
-    return false;
-  }
-
-  /**
-   * Ask Chimera to open a session file. Returns true if successful, else false.
-   * The filename must have a .py (Chimera) or .cxs (ChimeraX) extension for
-   * this command to work.
-   * 
-   * @param filepath
-   * @return
-   */
-  public boolean openSession(String filepath)
-  {
-    /*
-     * Chimera:  https://www.cgl.ucsf.edu/chimera/current/docs/UsersGuide/midas/open.html
-     * ChimeraX: https://www.cgl.ucsf.edu/chimerax/docs/user/commands/open.html
-     */
-    executeCommand("open " + filepath, true);
-    // todo: test for failure - how?
-    return true;
-  }
-
   /**
    * Send a 'show' command for all atoms in the currently selected columns
    * 
@@ -1016,7 +523,11 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
 
   /**
    * Constructs and send commands to Chimera to set attributes on residues for
-   * features visible in Jalview
+   * features visible in Jalview.
+   * <p>
+   * The syntax is: setattr r &lt;attName&gt; &lt;attValue&gt; &lt;atomSpec&gt;
+   * <p>
+   * For example: setattr r jv_chain "Ferredoxin-1, Chloroplastic" #0:94.A
    * 
    * @param avp
    * @return
@@ -1024,30 +535,23 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   public int sendFeaturesToViewer(AlignmentViewPanel avp)
   {
     // TODO refactor as required to pull up to an interface
-    AlignmentI alignment = avp.getAlignment();
 
-    String[] files = getStructureFiles();
-    if (files == null)
-    {
-      return 0;
-    }
-
-    StructureMappingcommandSet commandSet = ChimeraCommands
-            .getSetAttributeCommandsForFeatures(getSsm(), files,
-                    getSequence(), avp, chimeraManager.isChimeraX());
-    String[] commands = commandSet.commands;
-    if (commands.length > 10)
+    Map<String, Map<Object, AtomSpecModel>> featureValues = buildFeaturesMap(
+            avp);
+    List<StructureCommandI> commands = getCommandGenerator()
+            .setAttributes(featureValues);
+    if (commands.size() > 10)
     {
       sendCommandsByFile(commands);
     }
     else
     {
-      for (String command : commands)
+      for (StructureCommandI command : commands)
       {
         sendAsynchronousCommand(command, null);
       }
     }
-    return commands.length;
+    return commands.size();
   }
 
   /**
@@ -1057,22 +561,22 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
    * 
    * @param commands
    */
-  protected void sendCommandsByFile(String[] commands)
+  protected void sendCommandsByFile(List<StructureCommandI> commands)
   {
-    boolean toChimeraX = chimeraManager.isChimeraX();
     try
     {
-      File tmp = File.createTempFile("chim", toChimeraX ? ".cxc" : ".com");
+      File tmp = File.createTempFile("chim", getCommandFileExtension());
       tmp.deleteOnExit();
       PrintWriter out = new PrintWriter(new FileOutputStream(tmp));
-      for (String command : commands)
+      for (StructureCommandI command : commands)
       {
-        out.println(command);
+        out.println(command.getCommand());
       }
       out.flush();
       out.close();
       String path = tmp.getAbsolutePath();
-      String command = "open " + (toChimeraX ? "" : "cmd:") + path;
+      StructureCommandI command = getCommandGenerator()
+              .openCommandFile(path);
       sendAsynchronousCommand(command, null);
     } catch (IOException e)
     {
@@ -1082,6 +586,16 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   }
 
   /**
+   * Returns the file extension required for a file of commands to be read by
+   * the structure viewer
+   * @return
+   */
+  protected String getCommandFileExtension()
+  {
+    return ".com";
+  }
+
+  /**
    * Get Chimera residues which have the named attribute, find the mapped
    * positions in the Jalview sequence(s), and set as sequence features
    * 
@@ -1102,7 +616,7 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
     // fails for 'average.bfactor' (which is bad):
 
     String cmd = "list residues attr '" + attName + "'";
-    List<String> residues = executeCommand(cmd, true);
+    List<String> residues = executeCommand(new StructureCommand(cmd), true);
 
     boolean featureAdded = createFeaturesForAttributes(attName, residues);
     if (featureAdded)
@@ -1217,14 +731,14 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
   }
 
   @Override
-  public int getModelNoForFile(String pdbFile)
+  public String getModelIdForFile(String pdbFile)
   {
     List<ChimeraModel> foundModels = chimeraMaps.get(pdbFile);
     if (foundModels != null && !foundModels.isEmpty())
     {
-      return foundModels.get(0).getModelNumber();
+      return String.valueOf(foundModels.get(0).getModelNumber());
     }
-    return -1;
+    return "";
   }
 
   /**
@@ -1250,8 +764,20 @@ public abstract class JalviewChimeraBinding extends AAStructureBindingModel
     return atts;
   }
 
-  public boolean isChimeraX()
+  /**
+   * Returns the file extension to use for a saved viewer session file (.py)
+   * 
+   * @return
+   */
+  @Override
+  public String getSessionFileExtension()
+  {
+    return CHIMERA_SESSION_EXTENSION;
+  }
+
+  @Override
+  public String getHelpURL()
   {
-    return chimeraManager.isChimeraX();
+    return "https://www.cgl.ucsf.edu/chimera/docs/UsersGuide";
   }
 }