temp

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Ah it does run when a column selection is made in a split frame (and propagated to the other panel). Which seems to work! Maybe just needs a less clumsy AlignmentViewport.isColSelChanged() - this m...

Ah it does run when a column selection is made in a split frame (and propagated to the other panel). Which seems to work!
Maybe just needs a less clumsy AlignmentViewport.isColSelChanged() - this method seems to be trying to do two different things.

Suggesting renaming to PROPERTY_COLUMNS = "columns" as this is used to notify changes to column selection or visibility, not sequences

Suggesting renaming to PROPERTY_COLUMNS = "columns" as this is used to notify changes to column selection or visibility, not sequences

I think this code path rarely gets run (AlignmentViewport.notifySequence() rarely gets called). So what is it there for?

I think this code path rarely gets run (AlignmentViewport.notifySequence() rarely gets called).
So what is it there for?

Suggest renaming notifyAlignment() to notifyAlignmentChanged(), and notifySequence() to notifyColumnChange() as it it fired for changes to column (not sequence) selection or visibility.

Suggest renaming notifyAlignment() to notifyAlignmentChanged(), and notifySequence() to notifyColumnChange() as it it fired for changes to column (not sequence) selection or visibility.

Unclear what notifySequence() adds. As placed here, it is hardly ever reached.

Unclear what notifySequence() adds. As placed here, it is hardly ever reached.

Not sure how to review this (and the appletgui/js classes) without pointers to documentation and/or examples of how it is used.

Not sure how to review this (and the appletgui/js classes) without pointers to documentation and/or examples of how it is used.

Needs proper Javadoc

Needs proper Javadoc

Files added to doc folder: are this intended as working documents, or publishable documentation? Bearing in mind that that are findable on the web and a user might reasonably expect them to provide...

Files added to doc folder: are this intended as working documents, or publishable documentation? Bearing in mind that that are findable on the web and a user might reasonably expect them to provide documentation.
e.g. http://source.jalview.org/gitweb/?p=jalview.git;a=blob;f=doc/JalviewJS-startupParams.md;h=9ff33529b2dbc2da34d2a573ccf5d042194fbb0d;hb=refs/heads/Jalview-JS/develop

Due to the number of changes this is likely to be a 'best efforts' review.

Due to the number of changes this is likely to be a 'best efforts' review.

Commits to JS-develop from 2nd to 25th June 2020
Commits to JS-develop from 2nd to 25th June 2020
JAL-3445 BSML BBB file format.

- merge of BSML test branch.

- includes BSMLFileTest

- preliminary; does not read complement

- See Jalview-JS/develop/temp/bbb.dtd.pdf

  1. … 3 more files in changeset.
I suggest we close this. It's all ancient history now.

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

Method only called from test code and lacks documentation to state what it returns. Replaced with use of PrivilegedAccessor in the tests.

Method only called from test code and lacks documentation to state what it returns.
Replaced with use of PrivilegedAccessor in the tests.

No unit test for this method. As currently coded, if a single feature of type X is added, then deleted, hasFeatures(X) then returns true. Need to add a second test && !featureStore.get(type).isEmpt...

No unit test for this method. As currently coded, if a single feature of type X is added, then deleted, hasFeatures(X) then returns true.
Need to add a second test
&& !featureStore.get(type).isEmpty()

Could possibly also, in SequenceFeatures.delete(SequenceFeature), remove the FeatureStore from the map if it becomes empty.
However this creates thread-safety dangers so is probably not worth doing.

Method throws NullPointerException if there is no FeatureStore (no features stored) for the given feature type, as easily shown with a unit test. It is not an answer to say "oh but this is checked ...

Method throws NullPointerException if there is no FeatureStore (no features stored) for the given feature type, as easily shown with a unit test.
It is not an answer to say "oh but this is checked by Sequence.hasFeatures(type) before calling this method".
Methods must be robust to any usage, current or future.
Check for null, and unit tests, added.

hidden colour is only used by OverviewRenderer but is passed around between OverviewCanvas and OverviewResColourFinder constructors. Just keep it in OverviewRendere only.

hidden colour is only used by OverviewRenderer but is passed around between OverviewCanvas and OverviewResColourFinder constructors.
Just keep it in OverviewRendere only.

tldr but I've changed the code back to the way it was, since this was a functional change (could result in different colour drawn).

tldr but I've changed the code back to the way it was, since this was a functional change (could result in different colour drawn).

I'm confused. You removed the Graphics2D field in SeqCanvas with the comment // Don't do this! Graphics handles are supposed to be transient so does that not also apply here?

I'm confused. You removed the Graphics2D field in SeqCanvas with the comment
// Don't do this! Graphics handles are supposed to be transient
so does that not also apply here?

In 2.11, scrollY is always zero in this method. Why the change?

In 2.11, scrollY is always zero in this method. Why the change?

I see what you mean, sure. Probably ensureFinalized at least, and maybe others should be indicated as synchronized, as it does major fixing of the database. This ret[] could be created locally; it'...

I see what you mean, sure. Probably ensureFinalized at least, and maybe others should be indicated as synchronized, as it does major fixing of the database. This ret[] could be created locally; it's not a big deal to make a single array each findOverlaps call, probably, if this is a concern. This is now in IntervalStore0.

method needs Javadoc - what does it do?

method needs Javadoc - what does it do?

Not convinced this is even needed. The box is resized when the font size is changed, so no need to repeat on OK (which just closes the dialog). OverviewDimensions.drawBox() is called from OverviewC...

Not convinced this is even needed. The box is resized when the font size is changed, so no need to repeat on OK (which just closes the dialog).
OverviewDimensions.drawBox() is called from OverviewCanvas.paintComponent().
Not sure whether this is an unintended (but useful) repaint or explicitly scheduled.

Added Javadoc to clarify that the BitSet returned is valid at a point in time but doesn't update if columns are revealed or hidden.

Added Javadoc to clarify that the BitSet returned is valid at a point in time but doesn't update if columns are revealed or hidden.

Do we still need this class? Given that bin.Jalview can parse url query parameters.

Do we still need this class? Given that bin.Jalview can parse url query parameters.

reverted to readable

reverted to readable

That works. I have gone for the equivalent but simpler - just pass a flag for 'compareBegin' (true : use begin, false : use end) rather than a function. Comes to the same thing. No loss of generali...

That works. I have gone for the equivalent but simpler - just pass a flag for 'compareBegin' (true : use begin, false : use end) rather than a function. Comes to the same thing. No loss of generality as IntervalI has nothing else that could be extracted as a value.

My original BinarySearcher(List<T>) could be generalised to any type, with a function extracting any value, and another applying any test, but a heck of a lot of object creation involved, so I ditched that idea.

It works fine as long as the methods only read instance fields and don't update them. No conflict arises. If two threads might update a field (such as IntervalStore0.ret) then conflict can arise.

It works fine as long as the methods only read instance fields and don't update them. No conflict arises.
If two threads might update a field (such as IntervalStore0.ret) then conflict can arise.

Hmm. Wasn't that my point? I think we are in agreement. Probably should add a JavaDoc note to the effect that it WILL throw that exception despite the fact that it is optional. Object.equals() does...

Hmm. Wasn't that my point? I think we are in agreement. Probably should add a JavaDoc note to the effect that it WILL throw that exception despite the fact that it is optional. Object.equals() doesn't have any mention of throwing ClassCastException. Every such implementation I have ever seen first checks for not implementing the class for a false return.

AbstractCollection.contains() declares ClassCastException because it may be thrown by the calls to equals() in the loop. It is up to the implementation of equals() whether it checks instanceof, cat...

AbstractCollection.contains() declares ClassCastException because it may be thrown by the calls to equals() in the loop.
It is up to the implementation of equals() whether it checks instanceof, catches and handles the exception, or lets it be thrown.

Just make sure there is no type checking in anything that is done during normal processing. Actually, that's not quite true: Collection.contains: https://docs.oracle.com/javase/8/docs/api/java/ut...

Just make sure there is no type checking in anything that is done during normal processing. Actually, that's not quite true:


Collection.contains: https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#contains-java.lang.Object-

boolean contains(Object o)

Returns true if this collection contains the specified element. More formally, returns true if and only if this collection contains at least one element e such that (o==null ? e==null : o.equals(e)).

Parameters:
o - element whose presence in this collection is to be tested
Returns:
true if this collection contains the specified element
Throws:
ClassCastException - if the type of the specified element is incompatible with this collection (optional)

Also, despite the fact that AbstractCollection.contains() does not throw that exception (I agree there), it's JavaDoc says it does:

/**

  • Unknown macro: {@inheritDoc}



    *

    • <p>This implementation iterates over the elements in the collection,

    • checking each element in turn for equality with the specified element.


    *

    • @throws ClassCastException

  • @throws NullPointerException
    Unknown macro: {@inheritDoc}


*/