From bd817b546aad8c7456fd5682a890eb79faeb5deb Mon Sep 17 00:00:00 2001 From: gmungoc Date: Wed, 26 Aug 2015 15:40:12 +0100 Subject: [PATCH] JAL-1830 dropped reference counts for mappings, instead remove mapping when no alignment holds a reference to it --- src/jalview/appletgui/SplitFrame.java | 2 +- src/jalview/gui/AlignFrame.java | 2 +- src/jalview/gui/AlignViewport.java | 4 +- src/jalview/io/VamsasAppDatastore.java | 2 +- src/jalview/io/vamsas/Sequencemapping.java | 2 +- .../structure/StructureSelectionManager.java | 111 ++++++------ .../structure/StructureSelectionManagerTest.java | 177 +++++++++++--------- 7 files changed, 158 insertions(+), 142 deletions(-) diff --git a/src/jalview/appletgui/SplitFrame.java b/src/jalview/appletgui/SplitFrame.java index 836d70c..888208a 100644 --- a/src/jalview/appletgui/SplitFrame.java +++ b/src/jalview/appletgui/SplitFrame.java @@ -61,7 +61,7 @@ public class SplitFrame extends EmbmenuFrame { final StructureSelectionManager ssm = StructureSelectionManager .getStructureSelectionManager(topViewport.applet); - ssm.addMappings(protein.getAlignment().getCodonFrames()); + ssm.registerMappings(protein.getAlignment().getCodonFrames()); topViewport.setCodingComplement(bottomViewport); ssm.addCommandListener(cdna); ssm.addCommandListener(protein); diff --git a/src/jalview/gui/AlignFrame.java b/src/jalview/gui/AlignFrame.java index 806ad66..afa6847 100644 --- a/src/jalview/gui/AlignFrame.java +++ b/src/jalview/gui/AlignFrame.java @@ -4887,7 +4887,7 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, al.getCodonFrames().addAll(cf); final StructureSelectionManager ssm = StructureSelectionManager .getStructureSelectionManager(Desktop.instance); - ssm.addMappings(cf); + ssm.registerMappings(cf); } else { diff --git a/src/jalview/gui/AlignViewport.java b/src/jalview/gui/AlignViewport.java index aedfe12..997fd52 100644 --- a/src/jalview/gui/AlignViewport.java +++ b/src/jalview/gui/AlignViewport.java @@ -434,7 +434,7 @@ public class AlignViewport extends AlignmentViewport implements */ if (alignment != null) { - ssm.removeMappings(alignment.getCodonFrames()); + ssm.deregisterMappings(alignment.getCodonFrames()); } /* @@ -442,7 +442,7 @@ public class AlignViewport extends AlignmentViewport implements */ if (align != null) { - ssm.addMappings(align.getCodonFrames()); + ssm.registerMappings(align.getCodonFrames()); } /* diff --git a/src/jalview/io/VamsasAppDatastore.java b/src/jalview/io/VamsasAppDatastore.java index 7df7cb2..d67293a 100644 --- a/src/jalview/io/VamsasAppDatastore.java +++ b/src/jalview/io/VamsasAppDatastore.java @@ -1469,7 +1469,7 @@ public class VamsasAppDatastore { jalview.structure.StructureSelectionManager .getStructureSelectionManager(Desktop.instance) - .addMappings(mappings); + .registerMappings(mappings); } } } diff --git a/src/jalview/io/vamsas/Sequencemapping.java b/src/jalview/io/vamsas/Sequencemapping.java index 4929a06..d23b264 100644 --- a/src/jalview/io/vamsas/Sequencemapping.java +++ b/src/jalview/io/vamsas/Sequencemapping.java @@ -364,7 +364,7 @@ public class Sequencemapping extends Rangetype } bindjvvobj(mapping, sequenceMapping); jalview.structure.StructureSelectionManager - .getStructureSelectionManager(Desktop.instance).addMapping(acf); + .getStructureSelectionManager(Desktop.instance).registerMapping(acf); // Try to link up any conjugate database references in the two sequences // matchConjugateDBRefs(from, to, mapping); // Try to propagate any dbrefs across this mapping. diff --git a/src/jalview/structure/StructureSelectionManager.java b/src/jalview/structure/StructureSelectionManager.java index cf60490..9dd730c 100644 --- a/src/jalview/structure/StructureSelectionManager.java +++ b/src/jalview/structure/StructureSelectionManager.java @@ -21,6 +21,7 @@ package jalview.structure; import jalview.analysis.AlignSeq; +import jalview.api.AlignmentViewPanel; import jalview.api.StructureSelectionManagerProvider; import jalview.commands.CommandI; import jalview.commands.EditCommand; @@ -32,6 +33,8 @@ import jalview.datamodel.Annotation; import jalview.datamodel.PDBEntry; import jalview.datamodel.SearchResults; import jalview.datamodel.SequenceI; +import jalview.gui.AlignFrame; +import jalview.gui.Desktop; import jalview.io.AppletFormatAdapter; import jalview.util.MappingUtils; import jalview.util.MessageManager; @@ -72,12 +75,6 @@ public class StructureSelectionManager */ Set seqmappings = new LinkedHashSet(); - /* - * Reference counters for the above mappings. Remove mappings when ref count - * goes to zero. - */ - private Map seqMappingRefCounts = new HashMap(); - private List commandListeners = new ArrayList(); private List sel_listeners = new ArrayList(); @@ -518,8 +515,12 @@ public class StructureSelectionManager if (resNum != tmp.resNumber && tmp.alignmentMapping != -1) { resNum = tmp.resNumber; - mapping[tmp.alignmentMapping + 1][0] = tmp.resNumber; - mapping[tmp.alignmentMapping + 1][1] = tmp.atomIndex; + if (tmp.alignmentMapping >= -1) + { + // TODO (JAL-1836) address root cause: negative residue no in PDB file + mapping[tmp.alignmentMapping + 1][0] = tmp.resNumber; + mapping[tmp.alignmentMapping + 1][1] = tmp.atomIndex; + } } index++; @@ -911,83 +912,93 @@ public class StructureSelectionManager } /** - * Decrement the reference counter for each of the given mappings, and remove - * it entirely if its reference counter reduces to zero. + * Deregister each mapping in the set, unless there is still an alignment that + * holds a reference to it. Note we do not update the set itself, as it may be + * shared with an alignment view which is still open. * * @param set */ - public void removeMappings(Set set) + public void deregisterMappings(Set set) { if (set != null) { for (AlignedCodonFrame acf : set) { - removeMapping(acf); + deregisterMapping(acf); } } } /** - * Decrement the reference counter for the given mapping, and remove it - * entirely if its reference counter reduces to zero. + * Remove the given mapping provided no alignment holds a reference to it * * @param acf */ - public void removeMapping(AlignedCodonFrame acf) + public void deregisterMapping(AlignedCodonFrame acf) { - if (acf != null && seqmappings.contains(acf)) + if (noReferencesTo(acf)) { - int count = seqMappingRefCounts.get(acf); - count--; - if (count > 0) - { - seqMappingRefCounts.put(acf, count); + boolean removed = seqmappings.remove(acf); + if (removed && seqmappings.isEmpty()) + { // debug + System.out.println("All mappings removed"); } - else + } + } + + /** + * Answers true if no alignment holds a reference to the given mapping + * + * @param acf + * @return + */ + protected boolean noReferencesTo(AlignedCodonFrame acf) + { + AlignFrame[] frames = Desktop.getAlignFrames(); + if (frames == null) + { + return true; + } + for (AlignFrame af : frames) + { + for (AlignmentViewPanel ap : af.getAlignPanels()) { - seqmappings.remove(acf); - seqMappingRefCounts.remove(acf); - if (seqmappings.isEmpty()) - { // debug - System.out.println("All mappings removed"); + AlignmentI al = ap.getAlignment(); + if (al != null && al.getCodonFrames().contains(acf)) + { + return false; } } } + return true; } /** - * Add each of the given codonFrames to the stored set. If not aready present, - * increments its reference count instead. + * Add each of the given codonFrames to the stored set, if not aready present. * * @param set */ - public void addMappings(Set set) + public void registerMappings(Set set) { if (set != null) { for (AlignedCodonFrame acf : set) { - addMapping(acf); + registerMapping(acf); } } } /** - * Add the given mapping to the stored set, or if already stored, increment - * its reference counter. + * Add the given mapping to the stored set, unless already stored. */ - public void addMapping(AlignedCodonFrame acf) + public void registerMapping(AlignedCodonFrame acf) { if (acf != null) { - if (seqmappings.contains(acf)) - { - seqMappingRefCounts.put(acf, seqMappingRefCounts.get(acf) + 1); - } - else + if (!seqmappings.contains(acf)) { seqmappings.add(acf); - seqMappingRefCounts.put(acf, 1); } } } @@ -1006,10 +1017,6 @@ public class StructureSelectionManager { seqmappings.clear(); } - if (seqMappingRefCounts != null) - { - seqMappingRefCounts.clear(); - } if (sel_listeners != null) { sel_listeners.clear(); @@ -1187,20 +1194,4 @@ public class StructureSelectionManager } return null; } - - /** - * Returns the reference count for a mapping - * - * @param acf - * @return - */ - protected int getMappingReferenceCount(AlignedCodonFrame acf) - { - if (seqMappingRefCounts == null) - { - return 0; - } - Integer count = seqMappingRefCounts.get(acf); - return (count == null ? 0 : count.intValue()); - } } diff --git a/test/jalview/structure/StructureSelectionManagerTest.java b/test/jalview/structure/StructureSelectionManagerTest.java index 0162308..3cbdbad 100644 --- a/test/jalview/structure/StructureSelectionManagerTest.java +++ b/test/jalview/structure/StructureSelectionManagerTest.java @@ -1,13 +1,20 @@ package jalview.structure; import static org.testng.AssertJUnit.assertEquals; +import static org.testng.AssertJUnit.assertNotNull; import static org.testng.AssertJUnit.assertTrue; import jalview.datamodel.AlignedCodonFrame; +import jalview.gui.AlignFrame; +import jalview.gui.Desktop; +import jalview.io.FileLoader; +import jalview.io.FormatAdapter; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.Set; +import org.testng.annotations.BeforeClass; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -15,6 +22,13 @@ public class StructureSelectionManagerTest { private StructureSelectionManager ssm; + @BeforeClass(alwaysRun = true) + public static void setUpBeforeClass() throws Exception + { + jalview.bin.Jalview.main(new String[] { "-props", + "test/jalview/testProps.jvprops" }); + } + @BeforeMethod(alwaysRun = true) public void setUp() { @@ -22,42 +36,31 @@ public class StructureSelectionManagerTest } @Test(groups ={ "Functional" }) - public void testAddMapping() + public void testRegisterMapping() { AlignedCodonFrame acf1 = new AlignedCodonFrame(); AlignedCodonFrame acf2 = new AlignedCodonFrame(); - /* - * One mapping only. - */ - ssm.addMapping(acf1); + ssm.registerMapping(acf1); assertEquals(1, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); - assertEquals(1, ssm.getMappingReferenceCount(acf1)); - /* - * A second mapping. - */ - ssm.addMapping(acf2); + ssm.registerMapping(acf2); assertEquals(2, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); assertTrue(ssm.seqmappings.contains(acf2)); - assertEquals(1, ssm.getMappingReferenceCount(acf1)); - assertEquals(1, ssm.getMappingReferenceCount(acf2)); /* - * A second reference to the first mapping. + * Re-adding the first mapping does nothing */ - ssm.addMapping(acf1); + ssm.registerMapping(acf1); assertEquals(2, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); assertTrue(ssm.seqmappings.contains(acf2)); - assertEquals(2, ssm.getMappingReferenceCount(acf1)); - assertEquals(1, ssm.getMappingReferenceCount(acf2)); } @Test(groups ={ "Functional" }) - public void testAddMappings() + public void testRegisterMappings() { AlignedCodonFrame acf1 = new AlignedCodonFrame(); AlignedCodonFrame acf2 = new AlignedCodonFrame(); @@ -71,101 +74,123 @@ public class StructureSelectionManagerTest set2.add(acf3); /* - * Adding both sets adds acf2 twice and acf1 and acf3 once each. + * Add both sets twice; each mapping should be added once only */ - ssm.addMappings(set1); - ssm.addMappings(set2); + ssm.registerMappings(set1); + ssm.registerMappings(set1); + ssm.registerMappings(set2); + ssm.registerMappings(set2); assertEquals(3, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); assertTrue(ssm.seqmappings.contains(acf2)); assertTrue(ssm.seqmappings.contains(acf3)); - assertEquals(1, ssm.getMappingReferenceCount(acf1)); - assertEquals(2, ssm.getMappingReferenceCount(acf2)); - assertEquals(1, ssm.getMappingReferenceCount(acf3)); } + /** + * Test that a mapping is not deregistered if an alignment holds a reference + * to it + */ @Test(groups ={ "Functional" }) - public void testRemoveMapping() + public void testDeregisterMapping_withAlignmentReference() { + Desktop d = Desktop.instance; + assertNotNull(d); + + /* + * alignment with reference to mappings + */ + AlignFrame af1 = new FileLoader().LoadFileWaitTillLoaded( + ">Seq1\nCAGT\n", FormatAdapter.PASTE); + AlignedCodonFrame acf1 = new AlignedCodonFrame(); AlignedCodonFrame acf2 = new AlignedCodonFrame(); + Set mappings = new LinkedHashSet(); + mappings.add(acf1); + mappings.add(acf2); + af1.getViewport().getAlignment().setCodonFrames(mappings); + /* * Add one and remove it. */ - ssm.addMapping(acf1); - ssm.removeMapping(acf1); - ssm.removeMapping(acf2); - assertEquals(0, ssm.seqmappings.size()); - assertEquals(0, ssm.getMappingReferenceCount(acf1)); - assertEquals(0, ssm.getMappingReferenceCount(acf2)); + ssm.registerMapping(acf1); + ssm.deregisterMapping(acf1); + assertEquals(1, ssm.seqmappings.size()); + assertTrue(ssm.seqmappings.contains(acf1)); + } + /** + * Test that a mapping is deregistered if no alignment holds a reference to it + */ + @Test(groups ={ "Functional" }) + public void testDeregisterMapping_withNoReference() + { + Desktop d = Desktop.instance; + assertNotNull(d); + /* - * Add one twice and remove it once. + * alignment with reference to mappings */ - ssm.addMapping(acf1); - ssm.addMapping(acf2); - ssm.addMapping(acf1); - ssm.removeMapping(acf1); - assertEquals(2, ssm.seqmappings.size()); - assertTrue(ssm.seqmappings.contains(acf1)); - assertTrue(ssm.seqmappings.contains(acf2)); - assertEquals(1, ssm.getMappingReferenceCount(acf1)); - assertEquals(1, ssm.getMappingReferenceCount(acf2)); - + AlignFrame af1 = new FileLoader().LoadFileWaitTillLoaded( + ">Seq1\nCAGT\n", FormatAdapter.PASTE); + + AlignedCodonFrame acf1 = new AlignedCodonFrame(); + AlignedCodonFrame acf2 = new AlignedCodonFrame(); + + Set mappings = new LinkedHashSet(); + mappings.add(acf2); + af1.getViewport().getAlignment().setCodonFrames(mappings); + /* - * Remove both once more to clear the set. + * Add one and remove it. */ - ssm.removeMapping(acf1); - ssm.removeMapping(acf2); + ssm.registerMapping(acf1); + assertEquals(1, ssm.seqmappings.size()); + assertTrue(ssm.seqmappings.contains(acf1)); + ssm.deregisterMapping(acf1); assertEquals(0, ssm.seqmappings.size()); - assertEquals(0, ssm.getMappingReferenceCount(acf1)); - assertEquals(0, ssm.getMappingReferenceCount(acf2)); } + /** + * Test that a mapping is not deregistered when a second view is closed but + * the first still holds a reference to the mapping + */ @Test(groups ={ "Functional" }) - public void testRemoveMappings() + public void testDeregisterMapping_onCloseView() { - AlignedCodonFrame acf1 = new AlignedCodonFrame(); - AlignedCodonFrame acf2 = new AlignedCodonFrame(); - AlignedCodonFrame acf3 = new AlignedCodonFrame(); - /* - * Initial ref counts are 3/2/1: + * alignment with reference to mappings */ - ssm.addMapping(acf1); - ssm.addMapping(acf1); - ssm.addMapping(acf1); - ssm.addMapping(acf2); - ssm.addMapping(acf2); - ssm.addMapping(acf3); - - Set set1 = new HashSet(); - set1.add(acf1); - set1.add(acf2); - Set set2 = new HashSet(); - set2.add(acf2); - set2.add(acf3); + AlignFrame af1 = new FileLoader().LoadFileWaitTillLoaded( + ">Seq1\nCAGT\n", FormatAdapter.PASTE); + + AlignedCodonFrame acf1 = new AlignedCodonFrame(); + AlignedCodonFrame acf2 = new AlignedCodonFrame(); + + Set mappings = new LinkedHashSet(); + mappings.add(acf1); + mappings.add(acf2); + af1.getViewport().getAlignment().setCodonFrames(mappings); + af1.newView_actionPerformed(null); /* - * Remove one ref each to acf1, acf2, counts are now 2/1/1: + * Verify that creating the alignment for the new View has registered the + * mappings */ - ssm.removeMappings(set1); - assertEquals(3, ssm.seqmappings.size()); + ssm = StructureSelectionManager + .getStructureSelectionManager(Desktop.instance); + assertEquals(2, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); assertTrue(ssm.seqmappings.contains(acf2)); - assertTrue(ssm.seqmappings.contains(acf3)); - assertEquals(2, ssm.getMappingReferenceCount(acf1)); - assertEquals(1, ssm.getMappingReferenceCount(acf2)); - assertEquals(1, ssm.getMappingReferenceCount(acf3)); /* - * Remove one ref each to acf2, acf3 - they are removed + * Close the second view. Verify that mappings are not removed as the first + * view still holds a reference to them. */ - ssm.removeMappings(set2); - assertEquals(1, ssm.seqmappings.size()); + af1.closeMenuItem_actionPerformed(false); + assertEquals(2, ssm.seqmappings.size()); assertTrue(ssm.seqmappings.contains(acf1)); - assertEquals(2, ssm.getMappingReferenceCount(acf1)); + assertTrue(ssm.seqmappings.contains(acf2)); } } -- 1.7.10.2