Mungo Carstairs

About
  • This user has not yet committed.

Reviews

71 active reviews

Counts may include data that you cannot access.
Thursday 25 Feb 2021
3:57 pm

Mungo Carstairs

3:56 pm

Mungo Carstairs

This is likely an improvement on the "roll your own" pattern of setting Runnable handlers for dialog responses as hacked together for JalviewJS (JAL-3048). If so, it would be preferable (but no sma...

This is likely an improvement on the "roll your own" pattern of setting Runnable handlers for dialog responses as hacked together for JalviewJS (JAL-3048). If so, it would be preferable (but no small job) to change all dialogs to this pattern.

There are still numerous dialogs that have not been changed to either pattern (so won't work in SwingJS): see comment at https://issues.jalview.org/browse/?focusedCommentId=20525&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#JAL-3048comment-20525.

2:21 pm

Mungo Carstairs

Class has some compiler warnings which could easily be resolved (typed Hashtable, unused import).

Class has some compiler warnings which could easily be resolved (typed Hashtable, unused import).

2:17 pm

Mungo Carstairs

It would be good to add Javadoc to this method

It would be good to add Javadoc to this method

2:14 pm

Mungo Carstairs

As an aside, I would suggest inlining (removing) the two overloaded constructors that call this one (to reduce code bloat).

As an aside, I would suggest inlining (removing) the two overloaded constructors that call this one (to reduce code bloat).

Thursday 11 Feb 2021
5:06 pm

Mungo Carstairs

Or can it be compiled to Java 8 bytecode?

Or can it be compiled to Java 8 bytecode?

3:00 pm

Mungo Carstairs

'var' is since Java 10. We are still supporting Java 8 runtime which will not run this code (sadly). Compile with JDK 8 to find other occurrences (also Preferences, line 596 is not Java 8 compatibl...

'var' is since Java 10. We are still supporting Java 8 runtime which will not run this code (sadly).
Compile with JDK 8 to find other occurrences (also Preferences, line 596 is not Java 8 compatible syntax).

2:27 pm

Mungo Carstairs

Above statement is a duplicate - should only be inside the check for service != null. It fails with NPE if service is null.

Above statement is a duplicate - should only be inside the check for service != null. It fails with NPE if service is null.

2:26 pm

Mungo Carstairs

startjob and canceljob are only referenced in jbinit() so should be local variables in that method.

startjob and canceljob are only referenced in jbinit() so should be local variables in that method.

2:17 pm

Mungo Carstairs

Field startJob used to be the return value of showRunDialog(). As it no longer is, it can be removed.

Field startJob used to be the return value of showRunDialog(). As it no longer is, it can be removed.

12:29 pm

Mungo Carstairs

JAL-3809: Edit web service parameters dialog does not work in jalviewjs
JAL-3809: Edit web service parameters dialog does not work in jalviewjs
Thursday 13 Aug 2020
4:35 pm

Mungo Carstairs

100f has been changed to 1000 (integer) as a constant SCALE_TICKS. setExtent() has been removed.

100f has been changed to 1000 (integer) as a constant SCALE_TICKS.
setExtent() has been removed.

Wednesday 05 Aug 2020
2:27 pm

Mungo Carstairs

changed

changed

Tuesday 07 Jul 2020
4:12 pm

Mungo Carstairs

4:08 pm

Mungo Carstairs

Will we need isNoJSLinux I wonder?

Will we need isNoJSLinux I wonder?

3:22 pm

Mungo Carstairs

or use equalsIgnoreCase()

or use equalsIgnoreCase()

3:20 pm

Mungo Carstairs

Use Cache.log.error() for logging (throughout)

Use Cache.log.error() for logging (throughout)

3:10 pm

Mungo Carstairs

Test for if (!lafSet) is missing here - also metal, nimbus, quaqua, vaqua

Test for if (!lafSet) is missing here - also metal, nimbus, quaqua, vaqua

2:51 pm

Mungo Carstairs

Using StringBuilder is an improvement. But for full marks, use it consistently (no string concatenation). Also in appendIfNotNull().

Using StringBuilder is an improvement. But for full marks, use it consistently (no string concatenation). Also in appendIfNotNull().

2:34 pm

Mungo Carstairs

It would be preferable to extract lines 370-454 as a method setLookAndFeel() rather than adding to the already overlong doMain() method length.

It would be preferable to extract lines 370-454 as a method setLookAndFeel() rather than adding to the already overlong doMain() method length.

2:28 pm

Mungo Carstairs

these lines not formatted?

these lines not formatted?

2:06 pm

Mungo Carstairs

Could parameterise this method by class name to reduce code duplication with setSystemLookAndFeel()

Could parameterise this method by class name to reduce code duplication with setSystemLookAndFeel()

1:54 pm

Mungo Carstairs

JAL-3608: Set choice of Look and Feel via system property
JAL-3608: Set choice of Look and Feel via system property
Tuesday 30 Jun 2020
4:50 pm

Mungo Carstairs

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.

2:24 pm

Mungo Carstairs

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

2:12 pm

Mungo Carstairs

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?

1:54 pm

Mungo Carstairs

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.

1:52 pm

Mungo Carstairs

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

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

12:36 pm

Mungo Carstairs

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.

12:18 pm

Mungo Carstairs

Needs proper Javadoc

Needs proper Javadoc