JAL-3253: Refactor and prototype JalviewJS.Desktop to run embedded in page

Activity

CR-JAL-181 42

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author 56m 5 I suggest we close this. It's all ancient history now.
    Reviewer - 12% reviewed 5h 42m 37 I have added a large number of commits to this review whi...
    Total   6h 38m 42  
    #permalink

    Objectives

    Major objectives:

    • migrate all static Desktop. methods to 'instance'-safe methods
    • port nascent JalviewLite API to JalviewJS (jalview.javascript.JalviewLiteJsApi)

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Mungo Carstairs

    Also covers some untagged commits including 425ed088 VARNAPanel change field ...

    Also covers some untagged commits including
    425ed088 VARNAPanel change field _border (used by JSComponent) to _myBorder

    • comment "how to avoid this?" - answer might be a CheckStyle rule that flags an error for a field of this name in a class that extends Component?


    b2f46d31 AppletParams
    4b022090 JalviewAppLoader
    08873667 Preferences and other classes

    Mungo Carstairs

    May need some thought as to intended scope of the various singleton classes a...

    May need some thought as to intended scope of the various singleton classes affected.
    For example ScoreModels : provides the built-in models including BLOSUM62, PAM250.
    Additional models can be added by drag and drop (e.g. of resources/blosum80.scm).
    Should doing this in one applet make it available to all? In which case true singleton scope is what is wanted for this class.

    Bob Hanson

    For sure. That is a conversation that I think this clean-up now enables. (I'm...

    For sure. That is a conversation that I think this clean-up now enables. (I'm out of that!)

    Mungo Carstairs

    Could the need for public fields in Instance be avoided by a modified pattern...

    Could the need for public fields in Instance be avoided by a modified pattern:
    1) rename Instance as InstanceProvider
    2) give it only static fields and methods

    public class InstanceProvider {
       private static final String JAVA = "Java";
       private static Map<Object, Map<Class, Object>> instances = new HashMap<>(); // 'singleton' for each class, per context
       private static Object getContext() 
       { 
          // returns JAVA for Java, currentThread.getThreadGroup() for JS
       }
       public static Object getInstance(Class c) 
       {
          return instances.get(getContext()).get(c); // with check for null
       }
       public static void setInstance(Object o) 
       {
            Map<Class, Object> map = instances.get(getContext());
           // create new Map if null
           map.put(o.getClass(), o);
        }
    

    Client code pattern:

    public FeatureSources getInstance() 
        {
            FeatureSources instance = (FeatureSources) InstanceProvider.getInstance(FeatureSources.class);
            if (instance == null) 
            {
                InstanceProvider.setInstance(instance = new FeatureSources());
            }
            return instance;
        }
    

    Advantage: extensible without any code change (no need to add a field to Instance for every new singleton class).

    Mungo Carstairs

    Better? reduce client code to one line FeatureSources.getInstance() : retur...

    Better? reduce client code to one line

    FeatureSources.getInstance() :
      return (FeatureSources) InstanceProvider.getInstance(FeatureSources.class);
    

    by using reflection to instantiate in InstanceProvider:

    public static Object getInstance(Class c, Class<?>... args)
    {
      public static Object getInstance(Class c) 
       {
          Map<Class, Object> map = instances.get(getContext());
          if (map == null) 
          {
              map = new HashMap<>();
              instances.put(getContext(), map);
          }
          Object o = map.get(c);
          if (o == null)
          {
              Constructor con = c.getDeclaredConstructor(args);
              con.setAccessible(true);
              o = con.newInstance();
              map.put(c, o);
          }
          return o;
       }
    }
    

    (with about 6 exceptions declared throwable, or caught internally, by the method).
    No longer requires a setInstance() method. Handles constructor arguments, or a simple no-args constructor.
    Is that transpilable?

    Mungo Carstairs

    Jalview.main() would need to change from new Jalview().doMain(arg); to g...

    Jalview.main() would need to change from

      new Jalview().doMain(arg);
    

    to

      getInstance().doMain(args);
    

    and similarly the Desktop constructor called from Jalview.doMain() to Desktop.getInstance().

    Bob Hanson

    yes, I noticed those two as being unique. And for a few classes we need to al...

    yes, I noticed those two as being unique. And for a few classes we need to allow some sort of

    newInstance()

    call that can reset the singleton.

    Mungo Carstairs

    Hangout discussion points: *private constructors are accessible in Javascri...

    Hangout discussion points:

    • private constructors are accessible in Javascript so that works
    • need to be sure there is no memory leak e.g. if applets are repeatedly stopped and started on page
    • like the convenience of being able to easily identify all classes that follow this singleton pattern
    • StructureSelectionManager is a unique case - serves instance per application and class context - needs revision perhaps?


    Proposed adjustments to the above design:
    1) store the Map<Class, Object> in currentThread.threadGroup if JS - so references disappear when applet shuts down
    2) define a tagging interface e.g.

    public interface SingletonI {}
    

    and restrict InstanceProvider to subclasses:

      public static Object getInstance(Class<? extends SingletonI> c,
              Class<?>... args)
      { ...
    

    Defining a callback method in the interface doesn't work as we are passing the Class, not an object, so we still need to use reflection to invoke the constructor.

    Bob Hanson

    see below.

    see below.

    Bob Hanson

    One thing to think about is what would be the simplest and most natural way o...

    One thing to think about is what would be the simplest and most natural way of calling for the singleton from other classes. Right now it is set up so that if there is a singleton class, all references to that class go through the class's static getInstance() method:

    findQuality(0, maxLength - 1, ScoreModels.getInstance().getBlosum62());

    To me, that has a nice look to it, so I would hope we stick with that.

    If it's OK with you, I would like to table this issue, make sure we have the tests successful, merge the branch, and then think about refining it. I need to make sure I have any applets running right now.

    Mungo Carstairs

    No problem, the call to ScoreModels.getInstance() is unchanged, only how it i...

    No problem, the call to ScoreModels.getInstance() is unchanged, only how it is implemented.

    Mungo Carstairs

    We will need clarity on behaviour of copy and paste. Jalview uses both Toolki...

    We will need clarity on behaviour of copy and paste. Jalview uses both
    Toolkit.getDefaultToolkit().getSystemClipboard() // this has global scope
    and
    Desktop.getInstance().jalviewClipboard // this has per-applet scope
    with the latter 'overriding' the former.
    This may be fine as is, or may create some puzzling behaviour e.g. a "rich paste" (sequence + features) within an applet and a "text paste" (sequence only) across applications.

    Mungo Carstairs

    I am seeing some odd z-plane behaviour, not sure if it is specific to this br...

    I am seeing some odd z-plane behaviour, not sure if it is specific to this branch. If the desktop is brought to front, I can't then bring any other panel to front.

    Mungo Carstairs

    Can't reproduce this now (can't "bring desktop to front"). Will resolve and r...

    Can't reproduce this now (can't "bring desktop to front"). Will resolve and reopen if it recurs.

    Mungo Carstairs

    Too much change around ASequenceFetcher, DbRoot etc to be able to review from...

    Too much change around ASequenceFetcher, DbRoot etc to be able to review from the diffs. They seem to work when testing!
    (The code here may now be overcomplicated for the simplified (post-DAS) world.)

    Mungo Carstairs

    I have added a large number of commits to this review which were tagged "JAL-...

    I have added a large number of commits to this review which were tagged "JAL-3253-applet" or "Jalview-JS/develop-3253" or similar. This should be just the issue id for JIRA to associate the commit with the issue.

    Bob Hanson

    I suggest we close this. It's all ancient history now.

    I suggest we close this. It's all ancient history now.

    /doc/parameters.xlsx Added 1
    Open in IDE #permalink
    /libjs/VARNA-site.zip Changed
    Open in IDE #permalink
    /resources/lang/Messages.properties Changed
    Open in IDE #permalink
    /resources/lang/Messages_es.properties Changed
    Open in IDE #permalink
    /site-resources/javascript/JalviewApplet.js Deleted
    Open in IDE #permalink
    /site-resources/swingjs/JalviewApplet.js Changed
    Open in IDE #permalink
    /site-resources/applets-nocore.html Added
    Open in IDE #permalink
    /site-resources/applets.html Changed
    Open in IDE #permalink
    /src/com/stevesoft/pat/Regex.java Changed