javascript

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
I suggest we close this. It's all ancient history now.

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

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 ...

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.

This reset also needs to happen in copyAlignPanel() (which also calls loadViewport() where the flag is set on).

This reset also needs to happen in copyAlignPanel() (which also calls loadViewport() where the flag is set on).

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

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

No proprietary format files please! Is this meant to be a temporary working record or a permanent one. If the latter, can it be in csv or tsv format?

No proprietary format files please! Is this meant to be a temporary working record or a permanent one. If the latter, can it be in csv or tsv format?

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

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

Deferred to JAL-3265

Deferred to JAL-3265

Failing test getValidSourceDBRefExceptionXTest() (Network not Functional test). Now throws NullPointerException but coded only to expect SiftsException. Traced to uninitialized field DBRefEntry.ucv...

Failing test getValidSourceDBRefExceptionXTest() (Network not Functional test).
Now throws NullPointerException but coded only to expect SiftsException.
Traced to uninitialized field DBRefEntry.ucversion. Initialising this to "" (like version) resolves this.

I am nervous of doing much processing in a static block in case of errors giving ExceptionInInitializerError. Also possible dependency on order of classloading if one class's static codes reference...

I am nervous of doing much processing in a static block in case of errors giving ExceptionInInitializerError.
Also possible dependency on order of classloading if one class's static codes references another's.
Maybe move back to inside constructor?
In fact, was there any need to change this class from the 'standard' singleton pattern, if we don't want it "per applet"?

? there are no tests that call this getInstance() for this class. And if there were they would normally be in the same package so the method could be either package private, or called by reflection.

? there are no tests that call this getInstance() for this class. And if there were they would normally be in the same package so the method could be either package private, or called by reflection.

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.

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.

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.)

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.)

So what if anything used to access this? Why I hate public fields!

So what if anything used to access this? Why I hate public fields!

Could write file.contains("/") rather than file.indexOf('/') >= 0, for simplicity and readability. This is buggy - also in the original. Fetching from URL "https://www.ebi.ac.uk/Tools/dbfetch/dbfet...

Could write file.contains("/") rather than file.indexOf('/') >= 0, for simplicity and readability.
This is buggy - also in the original. Fetching from URL "https://www.ebi.ac.uk/Tools/dbfetch/dbfetch/pdb/1a70/pdb" results in file.substring(52, 18).
Issue JAL-3272 raised for that.

I have restored member variables to the top of the class declaration to keep standard source code ordering.

I have restored member variables to the top of the class declaration to keep standard source code ordering.

We have Platform.openURL() calling BrowserLauncher.openURL() and and vice versa, confusing. Suggest we make BrowserLauncher a package private class and route all calls via Platform.

We have Platform.openURL() calling BrowserLauncher.openURL() and and vice versa, confusing.
Suggest we make BrowserLauncher a package private class and route all calls via Platform.

Possibly the most strangely designed class in Jalview, but this version of it seems to work as well as any other!

Possibly the most strangely designed class in Jalview, but this version of it seems to work as well as any other!

This goes back to 2005...but it would be tidier to overload Cache.setProperty() to take a boolean value, and apply Boolean.toString() there...oh well.

This goes back to 2005...but it would be tidier to overload Cache.setProperty() to take a boolean value, and apply Boolean.toString() there...oh well.

Is this still 'TODO'? I just tried it and it worked fine (uniref50.fa + exampleFeatures.txt, popup menu on FER_CAPAA, Link sub-menu, Pfam family target).

Is this still 'TODO'? I just tried it and it worked fine (uniref50.fa + exampleFeatures.txt, popup menu on FER_CAPAA, Link sub-menu, Pfam family target).

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 h...

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.

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 refere...

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.

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.

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.

see below.

see below.

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

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

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 *li...

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.

May be an opportunity to simplify a bit here. Use Object rather than StructureSelectionManagerProvider for context. Use HashMap rather than IdentityHashMap.

May be an opportunity to simplify a bit here.
Use Object rather than StructureSelectionManagerProvider for context.
Use HashMap rather than IdentityHashMap.

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().

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().

Jalview.quit() calls System.exit() when running headless. Is this a problem?

Jalview.quit() calls System.exit() when running headless. Is this a problem?

Better? reduce client code to one line FeatureSources.getInstance() : return (FeatureSources) InstanceProvider.getInstance(FeatureSources.class); by using reflection to instantiate in InstancePr...

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?

If these are all put into a lookup map keyed by lower case value, no need for the case statement in getAppletParams().

If these are all put into a lookup map keyed by lower case value, no need for the case statement in getAppletParams().