From 882adcab8cf6f9c72a7e946223f817878c5ee444 Mon Sep 17 00:00:00 2001 From: gmungoc Date: Tue, 28 Apr 2015 16:55:17 +0100 Subject: [PATCH] JAL-1684 removed reflection from ViewStyle (to Junit), and removed duplications with AlignViewportI --- src/jalview/api/AlignViewportI.java | 54 ------ src/jalview/api/ViewStyleI.java | 31 ++- src/jalview/appletgui/AlignViewport.java | 2 +- src/jalview/bin/JalviewLite.java | 4 +- src/jalview/gui/AlignViewport.java | 2 +- src/jalview/viewmodel/AlignmentViewport.java | 21 +- src/jalview/viewmodel/styles/ViewStyle.java | 223 ++++++++++++---------- test/jalview/viewmodel/styles/TestviewStyle.java | 27 --- test/jalview/viewmodel/styles/ViewStyleTest.java | 174 +++++++++++++++++ 9 files changed, 328 insertions(+), 210 deletions(-) delete mode 100644 test/jalview/viewmodel/styles/TestviewStyle.java create mode 100644 test/jalview/viewmodel/styles/ViewStyleTest.java diff --git a/src/jalview/api/AlignViewportI.java b/src/jalview/api/AlignViewportI.java index 1a1cba2..8482f6b 100644 --- a/src/jalview/api/AlignViewportI.java +++ b/src/jalview/api/AlignViewportI.java @@ -43,12 +43,8 @@ import jalview.schemes.ColourSchemeI; public interface AlignViewportI extends ViewStyleI { - int getCharWidth(); - int getEndRes(); - int getCharHeight(); - /** * calculate the height for visible annotation, revalidating bounds where * necessary ABSTRACT GUI METHOD @@ -183,29 +179,6 @@ public interface AlignViewportI extends ViewStyleI void updateGroupAnnotationSettings(boolean applyGlobalSettings, boolean preserveNewGroupSettings); - /** - * @return true if a reference sequence is set and should be displayed - */ - public boolean isDisplayReferenceSeq(); - - /** - * @return set the flag for displaying reference sequences when they are - * available - */ - public void setDisplayReferenceSeq(boolean displayReferenceSeq); - - /** - * @return true if colourschemes should render according to reference sequence - * rather than consensus if available - */ - public boolean isColourByReferenceSeq(); - - /** - * @return true set flag for deciding if colourschemes should render according - * to reference sequence rather than consensus if available - */ - public void setColourByReferenceSeq(boolean colourByReferenceSeq); - void setSequenceColour(SequenceI seq, Color col); Color getSequenceColour(SequenceI seq); @@ -288,35 +261,8 @@ public interface AlignViewportI extends ViewStyleI String getSequenceSetId(); - boolean isShowSequenceFeatures(); - - void setShowSequenceFeatures(boolean b); - - /** - * - * @param flag - * indicating if annotation panel shown below alignment - * - */ - void setShowAnnotation(boolean b); - - /** - * flag indicating if annotation panel shown below alignment - * - * @return - */ - boolean isShowAnnotation(); - - boolean isRightAlignIds(); - - void setRightAlignIds(boolean rightAlignIds); - boolean areFeaturesDisplayed(); - void setShowSequenceFeaturesHeight(boolean selected); - - boolean isShowSequenceFeaturesHeight(); - void setFeaturesDisplayed(FeaturesDisplayedI featuresDisplayedI); void alignmentChanged(AlignmentViewPanel ap); diff --git a/src/jalview/api/ViewStyleI.java b/src/jalview/api/ViewStyleI.java index 0313c5c..933c270 100644 --- a/src/jalview/api/ViewStyleI.java +++ b/src/jalview/api/ViewStyleI.java @@ -79,10 +79,21 @@ public interface ViewStyleI void setShowUnconserved(boolean showunconserved); + /** + * @return true if a reference sequence is set and should be displayed + */ boolean isDisplayReferenceSeq(); + /** + * @return set the flag for displaying reference sequences when they are + * available + */ void setDisplayReferenceSeq(boolean displayReferenceSeq); + /** + * @return true if colourschemes should render according to reference sequence + * rather than consensus if available + */ boolean isColourByReferenceSeq(); void setSeqNameItalics(boolean default1); @@ -95,14 +106,26 @@ public interface ViewStyleI void setRightAlignIds(boolean rightAlignIds); + /** + * Returns true if annotation panel should be shown below alignment + * + * @return + */ boolean isShowAnnotation(); + /** + * Set flag for whether annotation panel should be shown below alignment + * + * @param b + */ void setShowAnnotation(boolean b); - void setShowSeqFeaturesHeight(boolean selected); - - boolean isShowSequenceFeaturesHeight(); + void setShowSequenceFeaturesHeight(boolean selected); + /** + * @return true set flag for deciding if colourschemes should render according + * to reference sequence rather than consensus if available + */ void setColourByReferenceSeq(boolean colourByReferenceSeq); Color getTextColour(); @@ -117,7 +140,7 @@ public interface ViewStyleI boolean isShowColourText(); - boolean isShowSeqFeaturesHeight(); + boolean isShowSequenceFeaturesHeight(); void setConservationColourSelected(boolean conservationColourSelected); diff --git a/src/jalview/appletgui/AlignViewport.java b/src/jalview/appletgui/AlignViewport.java index fdc6a0c..1ed4583 100644 --- a/src/jalview/appletgui/AlignViewport.java +++ b/src/jalview/appletgui/AlignViewport.java @@ -41,7 +41,7 @@ import jalview.structure.VamsasSource; import jalview.viewmodel.AlignmentViewport; public class AlignViewport extends AlignmentViewport implements - AlignViewportI, SelectionSource, VamsasSource, CommandListener + SelectionSource, VamsasSource, CommandListener { boolean cursorMode = false; diff --git a/src/jalview/bin/JalviewLite.java b/src/jalview/bin/JalviewLite.java index cb9d893..ebadb84 100644 --- a/src/jalview/bin/JalviewLite.java +++ b/src/jalview/bin/JalviewLite.java @@ -1242,7 +1242,7 @@ public class JalviewLite extends Applet implements String file = "No file"; - String file2 = "No file"; + String file2 = null; Button launcher = new Button( MessageManager.getString("label.start_jalview")); @@ -1459,7 +1459,7 @@ public class JalviewLite extends Applet implements file = data.toString(); } } - if ("true".equalsIgnoreCase(getParameter("enableSplitFrame"))) + if (TRUE.equalsIgnoreCase(getParameter("enableSplitFrame"))) { file2 = getParameter("file2"); } diff --git a/src/jalview/gui/AlignViewport.java b/src/jalview/gui/AlignViewport.java index cf12145..a54fa99 100644 --- a/src/jalview/gui/AlignViewport.java +++ b/src/jalview/gui/AlignViewport.java @@ -83,7 +83,7 @@ import jalview.ws.params.AutoCalcSetting; * @version $Revision: 1.141 $ */ public class AlignViewport extends AlignmentViewport implements - SelectionSource, AlignViewportI, CommandListener + SelectionSource, CommandListener { Font font; diff --git a/src/jalview/viewmodel/AlignmentViewport.java b/src/jalview/viewmodel/AlignmentViewport.java index d8ade26..302b0ef 100644 --- a/src/jalview/viewmodel/AlignmentViewport.java +++ b/src/jalview/viewmodel/AlignmentViewport.java @@ -75,7 +75,7 @@ import jalview.workers.StrucConsensusThread; * */ public abstract class AlignmentViewport implements AlignViewportI, - ViewStyleI, CommandListener, VamsasSource + CommandListener, VamsasSource { protected ViewStyleI viewStyle = new ViewStyle(); @@ -506,15 +506,6 @@ public abstract class AlignmentViewport implements AlignViewportI, } /** - * @param selected - * @see jalview.api.ViewStyleI#setShowSeqFeaturesHeight(boolean) - */ - public void setShowSeqFeaturesHeight(boolean selected) - { - viewStyle.setShowSeqFeaturesHeight(selected); - } - - /** * alignment displayed in the viewport. Please use get/setter */ protected AlignmentI alignment; @@ -2093,7 +2084,7 @@ public abstract class AlignmentViewport implements AlignViewportI, @Override public void setShowSequenceFeaturesHeight(boolean selected) { - viewStyle.setShowSeqFeaturesHeight(selected); + viewStyle.setShowSequenceFeaturesHeight(selected); } @Override @@ -2193,14 +2184,6 @@ public abstract class AlignmentViewport implements AlignViewportI, { return viewStyle.isShowColourText(); } - /** - * @return - * @see jalview.api.ViewStyleI#isShowSeqFeaturesHeight() - */ - public boolean isShowSeqFeaturesHeight() - { - return viewStyle.isShowSeqFeaturesHeight(); - } /** * @param conservationColourSelected diff --git a/src/jalview/viewmodel/styles/ViewStyle.java b/src/jalview/viewmodel/styles/ViewStyle.java index adc2baf..d6e529c 100644 --- a/src/jalview/viewmodel/styles/ViewStyle.java +++ b/src/jalview/viewmodel/styles/ViewStyle.java @@ -1,8 +1,6 @@ package jalview.viewmodel.styles; import java.awt.Color; -import java.lang.reflect.Method; -import java.util.HashMap; import jalview.api.ViewStyleI; @@ -17,7 +15,6 @@ import jalview.api.ViewStyleI; */ public class ViewStyle implements ViewStyleI { - private boolean abovePIDThreshold = false; int charHeight; @@ -151,104 +148,133 @@ public class ViewStyle implements ViewStyleI */ private boolean scaleProteinAsCdna = true; - public ViewStyle(ViewStyleI viewStyle) - { - ViewStyle.configureFrom(this, viewStyle); + /** + * Copy constructor + * + * @param vs + */ + public ViewStyle(ViewStyleI vs) + { + setAbovePIDThreshold(vs.getAbovePIDThreshold()); + setCentreColumnLabels(vs.isCentreColumnLabels()); + setCharHeight(vs.getCharHeight()); + setCharWidth(vs.getCharWidth()); + setColourAppliesToAllGroups(vs.getColourAppliesToAllGroups()); + setColourByReferenceSeq(vs.isColourByReferenceSeq()); + setColourText(vs.getColourText()); + setConservationColourSelected(vs.isConservationColourSelected()); + setConservationSelected(vs.getConservationSelected()); + setDisplayReferenceSeq(vs.isDisplayReferenceSeq()); + setFontName(vs.getFontName()); + setFontSize(vs.getFontSize()); + setFontStyle(vs.getFontStyle()); + setIdWidth(vs.getIdWidth()); + setIncrement(vs.getIncrement()); + setRenderGaps(vs.isRenderGaps()); + setRightAlignIds(vs.isRightAlignIds()); + setScaleAboveWrapped(vs.getScaleAboveWrapped()); + setScaleLeftWrapped(vs.getScaleLeftWrapped()); + setScaleProteinAsCdna(vs.isScaleProteinAsCdna()); + setScaleRightWrapped(vs.getScaleRightWrapped()); + setSeqNameItalics(vs.isSeqNameItalics()); + setShowAnnotation(vs.isShowAnnotation()); + setShowBoxes(vs.getShowBoxes()); + setShowColourText(vs.isShowColourText()); + setShowDBRefs(vs.isShowDBRefs()); + setShowHiddenMarkers(vs.getShowHiddenMarkers()); + setShowJVSuffix(vs.getShowJVSuffix()); + setShowNPFeats(vs.isShowNPFeats()); + setShowSequenceFeaturesHeight(vs.isShowSequenceFeaturesHeight()); + setShowSequenceFeatures(vs.isShowSequenceFeatures()); + setShowText(vs.getShowText()); + setShowUnconserved(vs.getShowUnconserved()); + setTextColour(vs.getTextColour()); + setTextColour2(vs.getTextColour2()); + setThreshold(vs.getThreshold()); + setThresholdTextColour(vs.getThresholdTextColour()); + setUpperCasebold(vs.isUpperCasebold()); + setWrapAlignment(vs.getWrapAlignment()); + setWrappedWidth(vs.getWrappedWidth()); + // ViewStyle.configureFrom(this, viewStyle); } public ViewStyle() { } - private static HashMap getters, isErs, setters; - static + /** + * Returns true if all attributes of the ViewStyles have the same value + */ + @Override + public boolean equals(Object other) { - getters = new HashMap(); - isErs = new HashMap(); - setters = new HashMap(); - // Match Getters and Setters - for (Method m : ViewStyleI.class.getMethods()) - { - if (m.getDeclaringClass() == ViewStyleI.class) - { - if (m.getName().startsWith("get")) - { - getters.put(m.getName().substring(3), m); - } - if (m.getName().startsWith("is")) - { - isErs.put(m.getName().substring(2), m); - } - if (m.getName().startsWith("set")) - { - setters.put(m.getName().substring(3), m); - } - } - } - } - - private static void configureFrom(ViewStyle us, ViewStyleI viewStyle) - { - // try and do the set thing - for (String prop : setters.keySet()) - { - Method getter = getters.get(prop); - Method setter = setters.get(prop); - if (getter == null) - { - getter = isErs.get(prop); - } - if (getter != null && setter != null) - { - try - { - setter.invoke(us, getter.invoke(viewStyle)); - } catch (Exception q) - { - System.err.println("Unexpected exception setting view property " - + prop + " by reflection"); - q.printStackTrace(); - } - - } - } - } - - private static boolean equivalent(ViewStyle us, ViewStyleI them) - { - // look for properties we can set - for (String prop : setters.keySet()) + if (other == null || !(other instanceof ViewStyle)) { - Method getter = getters.get(prop); - if (getter == null) - { - getter = isErs.get(prop); - } - if (getter != null) - { - try - { - if (!getter.invoke(them).equals(getter.invoke(us))) - { - return false; - } - } catch (Exception q) - { - System.err.println("Unexpected exception testing equivalence of property " - + prop + " by reflection"); - q.printStackTrace(); - } - } + return false; } - - return true; + ViewStyle vs = (ViewStyle) other; + + boolean match = (getAbovePIDThreshold() == vs.getAbovePIDThreshold() + && isCentreColumnLabels() == vs.isCentreColumnLabels() + && getCharHeight() == vs.getCharHeight() + && getCharWidth() == vs.getCharWidth() + && getColourAppliesToAllGroups() == vs + .getColourAppliesToAllGroups() + && isColourByReferenceSeq() == vs.isColourByReferenceSeq() + && getColourText() == vs.getColourText() + && isConservationColourSelected() == vs + .isConservationColourSelected() + && getConservationSelected() == vs.getConservationSelected() + && isDisplayReferenceSeq() == vs.isDisplayReferenceSeq() + && getFontSize() == vs.getFontSize() + && getFontStyle() == vs.getFontStyle() + && getIdWidth() == vs.getIdWidth() + && getIncrement() == vs.getIncrement() + && isRenderGaps() == vs.isRenderGaps() + && isRightAlignIds() == vs.isRightAlignIds() + && getScaleAboveWrapped() == vs.getScaleAboveWrapped() + && getScaleLeftWrapped() == vs.getScaleLeftWrapped() + && isScaleProteinAsCdna() == vs.isScaleProteinAsCdna() + && getScaleRightWrapped() == vs.getScaleRightWrapped() + && isSeqNameItalics() == vs.isSeqNameItalics() + && isShowAnnotation() == vs.isShowAnnotation() + && getShowBoxes() == vs.getShowBoxes() + && isShowColourText() == vs.isShowColourText() + && isShowDBRefs() == vs.isShowDBRefs() + && getShowHiddenMarkers() == vs.getShowHiddenMarkers() + && getShowJVSuffix() == vs.getShowJVSuffix() + && isShowNPFeats() == vs.isShowNPFeats() + && isShowSequenceFeaturesHeight() == vs + .isShowSequenceFeaturesHeight() + && isShowSequenceFeatures() == vs.isShowSequenceFeatures() + && getShowText() == vs.getShowText() + && getShowUnconserved() == vs.getShowUnconserved() + && getThreshold() == vs.getThreshold() + && getThresholdTextColour() == vs.getThresholdTextColour() + && isUpperCasebold() == vs.isUpperCasebold() + && getWrapAlignment() == vs.getWrapAlignment() && getWrappedWidth() == vs + .getWrappedWidth()); + /* + * and compare non-primitive types; syntax below will match null with null values + */ + match = match + && String.valueOf(getFontName()).equals( + String.valueOf(vs.getFontName())); + match = match + && String.valueOf(getTextColour()).equals( + String.valueOf(vs.getTextColour())); + match = match + && String.valueOf(getTextColour2()).equals( + String.valueOf(vs.getTextColour2())); + return match; + // return equivalent(this, (ViewStyle) other); + } + + @Override + public int hashCode() + { + return 0; // TODO } - - public boolean equals(ViewStyleI other) - { - return other == null ? false : equivalent(this, other); - } - /** * @return the upperCasebold */ @@ -559,7 +585,7 @@ public class ViewStyle implements ViewStyleI * @return the showSeqFeaturesHeight */ @Override - public boolean isShowSeqFeaturesHeight() + public boolean isShowSequenceFeaturesHeight() { return showSeqFeaturesHeight; } @@ -570,13 +596,6 @@ public class ViewStyle implements ViewStyleI return showSequenceFeatures; } - @Override - public boolean isShowSequenceFeaturesHeight() - { - - return showSeqFeaturesHeight; - } - /** * GUI state * @@ -794,7 +813,7 @@ public class ViewStyle implements ViewStyleI } @Override - public void setShowSeqFeaturesHeight(boolean selected) + public void setShowSequenceFeaturesHeight(boolean selected) { showSeqFeaturesHeight = selected; @@ -897,9 +916,9 @@ public class ViewStyle implements ViewStyleI } @Override - public boolean sameStyle(ViewStyleI them) + public boolean sameStyle(ViewStyleI that) { - return equivalent(this, them); + return this.equals(that); } @Override diff --git a/test/jalview/viewmodel/styles/TestviewStyle.java b/test/jalview/viewmodel/styles/TestviewStyle.java deleted file mode 100644 index 47735b5..0000000 --- a/test/jalview/viewmodel/styles/TestviewStyle.java +++ /dev/null @@ -1,27 +0,0 @@ -package jalview.viewmodel.styles; - -import org.junit.Assert; -import org.junit.Test; - -public class TestviewStyle -{ - - @Test - public void testSetterGetter() - { - ViewStyle orig = new ViewStyle(); - orig.setCharHeight(23); - orig.setWrappedWidth(253); - orig.setWrapAlignment(true); - orig.setScaleProteinAsCdna(true); - - ViewStyle copy = new ViewStyle(orig); - orig.setWrapAlignment(false); - Assert.assertEquals(copy.getCharHeight(), orig.getCharHeight()); - Assert.assertEquals(copy.getWrappedWidth(), orig.getWrappedWidth()); - Assert.assertEquals(copy.getWrapAlignment(), !orig.getWrapAlignment()); - Assert.assertEquals(copy.isScaleProteinAsCdna(), - orig.isScaleProteinAsCdna()); - } - -} diff --git a/test/jalview/viewmodel/styles/ViewStyleTest.java b/test/jalview/viewmodel/styles/ViewStyleTest.java new file mode 100644 index 0000000..9a0820f --- /dev/null +++ b/test/jalview/viewmodel/styles/ViewStyleTest.java @@ -0,0 +1,174 @@ +package jalview.viewmodel.styles; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.awt.Color; +import java.lang.reflect.Field; +import java.util.Random; + +import org.junit.Assert; +import org.junit.Test; + + +public class ViewStyleTest +{ + + Random r = new Random(); + + /** + * This test uses reflection to set all fields on a ViewStyle, make a copy of + * it, and verify all fields match. This test should fail if a getter/setter + * pair are added to the class but missing in the copy constructor. Using + * reflection in the copy constructor itself is broken by obfuscation when the + * applet is built. + * + * To prove this test works, simply comment out a line in the ViewStyle copy + * constructor, or add a new member field to ViewStyle. + * + * @throws IllegalAccessException + * @throws IllegalArgumentException + */ + @Test + public void testCopyConstructor() throws IllegalArgumentException, + IllegalAccessException + { + ViewStyle vs1 = new ViewStyle(); + Field[] fields = ViewStyle.class.getDeclaredFields(); + for (Field field : fields) + { + field.setAccessible(true); + changeValue(vs1, field); + } + + ViewStyle vs2 = new ViewStyle(vs1); + + for (Field field1 : fields) { + final Object value1 = field1.get(vs1); + final Object value2 = field1.get(vs2); + String msg = "Mismatch in " + field1.getName() + "(" + value1 + "/" + + value2 + ") - not set in copy constructor?"; + assertEquals(msg, value1, value2); + } + } + + /** + * Change the value of one field in a ViewStyle object + * + * @param vs + * @param field + * @throws IllegalAccessException + */ + protected void changeValue(ViewStyle vs, Field field) + throws IllegalAccessException + { + Class type = field.getType(); + final int numValue = 1 + r.nextInt(100); + + if (type.equals(boolean.class) || type.equals(Boolean.class)) + { + boolean value = (Boolean) field.get(vs); + // System.out.println("Setting " + field.getName() + " to " + !value); + field.set(vs, !value); + } + else if (type.equals(short.class) || type.equals(int.class) + || type.equals(long.class) || type.equals(float.class) + || type.equals(double.class)) + { + final int value = (int) (1 + field.getDouble(vs)); + // System.out.println("Setting " + field.getName() + " to " + value); + field.set(vs, value); + } + else if (type.equals(Integer.class)) + { + field.set(vs, (int) (1 + getNumberValue(field, vs))); + } + else if (type.equals(Float.class)) + { + field.set(vs, (float) (1f + getNumberValue(field, vs))); + } + else if (type.equals(Long.class)) + { + field.set(vs, (long) (1L + getNumberValue(field, vs))); + } + else if (type.equals(Double.class)) + { + field.set(vs, 1d + getNumberValue(field, vs)); + } + else if (type.equals(Short.class)) + { + field.set(vs, (short) (1 + getNumberValue(field, vs))); + } + else if (type.equals(Byte.class)) + { + field.set(vs, (byte) (1 + getNumberValue(field, vs))); + } + else if (type.equals(Character.class)) + { + field.set(vs, (char) (1 + getNumberValue(field, vs))); + } + else if (type.equals(String.class)) + { + field.set(vs, "Joe" + field.get(vs)); + } + else if (type.equals(Color.class)) + { + field.set(vs, Color.RED.equals(field.get(vs)) ? Color.BLACK + : Color.RED); + } + else + { + Assert.fail("Unhandled field type (add to test): " + field.getName() + + ":" + type); + } + } + + private double getNumberValue(Field field, ViewStyle vs) + throws IllegalArgumentException, IllegalAccessException + { + if (field.get(vs) == null) + { + return 0d; + } + return ((Number) field.get(vs)).doubleValue(); + } + + /** + * Test that the equals method compares every field by changing them one by + * one in a cloned ViewStyle. + * + * This test will fail if a new field is added to ViewStyle but not to the + * comparisons in ViewStyle.equals(). + * + * To confirm that this test works, temporarily comment out one of the field + * comparisons in ViewStyle.equals() + * + * @throws IllegalAccessException + * @throws IllegalArgumentException + */ + @Test + public void testEquals() throws IllegalArgumentException, + IllegalAccessException + { + ViewStyle vs1 = new ViewStyle(); + ViewStyle vs2 = new ViewStyle(vs1); + + assertFalse(vs1.equals(null)); + assertFalse(vs1.equals(this)); + assertTrue(vs1.equals(vs2)); + assertTrue(vs2.equals(vs1)); + + Field[] fields = ViewStyle.class.getDeclaredFields(); + for (Field field : fields) + { + field.setAccessible(true); + Object oldValue = field.get(vs2); + changeValue(vs2, field); + assertFalse("equals method ignores " + field.getName(), + vs1.equals(vs2)); + // restore original value before testing the next field + field.set(vs2, oldValue); + } + } +} -- 1.7.10.2