•  

Comment Results

Review Name Created Custom Fields Content
CR-JAL-1 26 Jan 2017

see reply to previous comment

CR-JAL-140 15 Jan 2018

could maybe write concisely as

hiddenColumns.forEach(range -> { /* calculation here */};);
CR-JAL-1 09 Jan 2017 Classification: Ambiguous

threshold doesn't express direction for the condition - need to explain model (+,-) as greater than or less than in rule (or leave as greater than always if there are never any less thans!)

CR-JAL-1 10 Jan 2017

TMI ? - having now looked at CollectionColourScheme I realised you've pushed ColourSchemeI to a lower level - implementers are used by the CollectionColourScheme to compute colours based on the info it provides. The findColour method signature still feels too specific tho'.

CR-JAL-1 09 Jan 2017

suggest provide a default implementation

CR-JAL-1 09 Jan 2017

Did you notice any side effects here ? Need to note them for 2.10.2's release notes.

CR-JAL-1 02 Feb 2017

See JAL-532 and JAL-384.

CR-JAL-1 06 Feb 2017

um - there would be side effects if the CalcId mechanism was broken.... but no matter.

CR-JAL-16 03 Mar 2017

Superclass method shows or hides viewerActionMenu, not viewSelectionMenu. ChimeraViewFrame overrules that to show viewerActionMenu.
Logic to hide viewSelectionMenu could be moved to superclass method instead.
It might be puzzling (why hide an item in a hidden menu?).

CR-JAL-16 03 Mar 2017

I'll rename consensus to gaprow for clarity here

CR-JAL-1 09 Jan 2017

Is there a use case for having a SequenceGroup without a context ? If so, the group could use SequenceI.isNucleotide() to give an accurate response rather than simply returning 'false' regardless. You need to make a note in javadoc if nothing else to warn the casual coder!

CR-JAL-1 20 Jan 2017

What happens if a colourscheme is created programmatically with the same name as an existing scheme ? possibly need to have some logic in the ColourSchemes singleton to handle disambiguation.

CR-JAL-1 20 Jan 2017

Is this something that affects batch operation of Jalview ?

CR-JAL-16 07 Mar 2017

Good question - but raises prior questions such as, how should Colour by Sequence behave with multiple views?

CR-JAL-1 09 Jan 2017

I still feel like TMI is needed by the implementor for a 'simple' colourscheme. If instead of returning asColourSchemeI, one could return as ComplexColourSchemeI or as SimpleColourSchemeI , and have a handler implement isSimple, that would make this cleaner.

CR-JAL-1 08 Feb 2017

? looks the same to me

CR-JAL-1 09 Jan 2017

Merging these does make sense (though we should bear in mind that Jalview features could be used to distinguish disulphide bonded from reduced cysteines), but now the 'Threshold' doesn't - don't you mean <100% here ?

CR-JAL-1 09 Jan 2017

You should just be able to walk up the context tree here, rather than check whether it's an alignment instance - both implement the same method after all.

CR-JAL-1 10 Jan 2017

Need to make sure we test compatibility with legacy jalview colourschemes (created pre 2.10.2)

CR-JAL-1 09 Feb 2017

Removed upper-casing of consensus from PID and Blosum62 findColour methods.

CR-JAL-1 10 Jan 2017

I know YAIGN applies here, but it's worth noting we always planned to have more alphabet types (ie polysaccharides).

CR-JAL-1 10 Jan 2017

this will not be fast. String.toUpperCase computes case according to locale (see eg https://stackoverflow.com/questions/35335737/why-is-string-touppercase-so-slow )

CR-JAL-1 10 Jan 2017 Classification: Improvement desirable Ranking: Minor

see CR-JAL-1#c17 - method is slow regardless of whether it does anything since it touches Locale

CR-JAL-1 10 Jan 2017

this javadoc doesn't really reflect what actually happens - which is that the name should resolve to one of the currently registered colourschemes, or parseable as a single colour or set of user defined symbol colours. Suggest you remove any explicit list of colours at least

CR-JAL-1 10 Jan 2017

CollectionColourScheme(I) is more than a data bean - it also holds the logic for deciding what colour the renderer will use given a particular row, column, sequence, group and viewport settings. Suggest it be renamed and moved from schemes to renderer (perhaps renderer.residueshader ???)

CR-JAL-1 10 Jan 2017

continuing the thread - not sure why Viewport needs both getGlobalColourScheme and getViewportColourScheme

CR-JAL-1 10 Jan 2017

need to mark this in release notes.

Is this actually desirable behaviour ? corner case is that If someone has saved a colourscheme on a network drive that isn't always available, they'll lose it from their preferences forever!

CR-JAL-1 20 Jan 2017

Threshold is held by the collection being coloured (viewport or group), it is a visualisation parameter.

CR-JAL-1 10 Jan 2017

Could some of this logic could be pulled into CollectionColourScheme ?

CR-JAL-1 10 Jan 2017 Ranking: Minor Classification: Risk-prone

shouldn't we raise an exception if nulls are passed to constructor ?

CR-JAL-1 10 Jan 2017

counter example to the SequenceGroup.isNucleotide() logic - we could have an hasRNAStructure() method on AnnotatedCollectionI, which could be computed/maintained for each sequence group also

CR-JAL-1 10 Jan 2017

add to feature request for preferences model & API ?

CR-JAL-1 10 Jan 2017

- preferred the old 'setConservation(..)' signature than having to get the automagically created GroupColourScheme...

CR-JAL-1 20 Jan 2017

Should you also test the other special name: "None" ?

CR-JAL-1 20 Jan 2017

Consider adding a new attribute to allow a Colourscheme to report its mapping to the BioJSON schema ? (JAL-1787)

CR-JAL-16 03 Mar 2017

return SearchResultsI ?

CR-JAL-17 06 Mar 2017

cherrypick to develop

CR-JAL-5 23 Jan 2017

Could you please incorporate commit 80edaa84d6d9beac9f0d2c71b50b7b56fd393427 - (https://source.jalview.org/crucible/changelog/jalview?cs=80edaa84d6d9beac9f0d2c71b50b7b56fd393427)

That provides a thread safe pattern for passing IProgressHandler instances to StructureSelectionManager

CR-JAL-1 20 Jan 2017

could these not be just:
new ClustalxColourScheme().isApplicableTo(..) as for the other tests ?

CR-JAL-21 30 Mar 2017

Updated text and gif.

CR-JAL-1 20 Jan 2017

I think this is very nearly ready to merge to develop. A couple of ambiguities regarding the class/interface architecture that might be worth revising before the merge, but otherwise the patch looks to maintain functionality (and, of course, fix several bugs).

CR-JAL-32 06 Apr 2017

typo: loseFocus

CR-JAL-16 03 Mar 2017

what happens if multiple views with different feature settings are associated with the view ? Also see JAL-2430 - should visible regions be honoured when transferring features as chimera attributes ?

CR-JAL-5 23 Jan 2017

jalview.gui.StructureChooser should be refactored to create a set of classes for each view mode for the dialog.

CR-JAL-16 03 Mar 2017

slight change in behaviour here - we used to report the number of views that would be used to construct the superposition in the tooltip for 'Align'

CR-JAL-5 23 Jan 2017 Classification: Risk-prone Ranking: Major

Not threadsafe - setMappingForPhyre2Model has side effects

CR-JAL-16 06 Mar 2017

yup - you can either pick one, or pick multiple - same for View->Colour by.

CR-JAL-16 07 Mar 2017

Reverted to original text including cardinality, apart from changing Align to Superpose for consistency with other text here

CR-JAL-5 23 Jan 2017

This should be refactored from Phyre2Client - allowing the jalview.io.AnnotationFile's STRUCTMODEL statement handler to call it.

Once you've refactored, please provide a code sample that allows a single structure model provided as a PDB file to be associated with its target (where sequence matches), and template (where match is given by the loaded alignment)

CR-JAL-17 06 Mar 2017

should move this file to testdata, or perhaps instead programmatically generate it (since it's just the example alignment imported repeatedly into one alignment view).