#927 Bug in IChemModel change event propagation

cdk-1.2.x
closed
nobody
None
5
2012-11-03
2009-06-09
Egon Willighagen
No

when a IMoleculeSet gets removed from a IChemModel, then a change in
that IMoleculeSet should not trigger a changeevent in the
IChemModel...

Right?

However, that is not what currently is implemented:

  • @Test public void testStateChanged_ButNotAfterRemoval_MoleculeSet() {
  • ChemObjectListenerImpl listener = new ChemObjectListenerImpl();
  • IChemModel chemObject = (IChemModel)newChemObject();
  • chemObject.addListener(listener);
    +
  • IMoleculeSet molSet = chemObject.getBuilder().newMoleculeSet();
  • chemObject.setMoleculeSet(molSet);
  • Assert.assertTrue(listener.changed);
  • // reset the listener
  • listener.reset(); Assert.assertFalse(listener.changed);
  • // remove the set from the IChemModel
  • chemObject.setMoleculeSet(null);
  • // changing the set must not trigger a change event in the IChemModel
  • molSet.addAtomContainer(chemObject.getBuilder().newMolecule());
  • Assert.assertFalse(listener.changed);
  • }

Attached patches are against cdk-1.2.x...

Is my assumption correct? Are these actual bugs?

Discussion

  • Well, the cause of this is line 100 in ChemModel in the setMoleculeSet method:

    this.setOfMolecules.addListener(this);

    which means that the reference you have to the molecule set (molSet) still shares a reference to the listener.

    I don't think that this behaviour makes sense, so your test is valid, yes.

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-08-03

    I think your partches are correct and fix the bug

     
  • Argh... I am sleeping... these are the patches for the unit tests, and have both already been applied... I thought I had uploaded the fix too...

    Sorry!!!

    Will dig those up now...

     
  • the patch

    0001-Fixed-unit-tests-the-IChemModel.setFoo-null-should.patch

    fixes the unit test, and can be applied on top of the other two which were applied earlier to cdk-1.2.x already.

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-11-01

    Patch still fails

    Applying: Fixed unit tests: the IChemModel.setFoo(null) should actually give a change event on the listener of the IChemModel
    error: patch failed: src/test/org/openscience/cdk/interfaces/AbstractChemModelTest.java:228
    error: src/test/org/openscience/cdk/interfaces/AbstractChemModelTest.java: patch does not apply
    Patch failed at 0001.

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-11-01

    Latest patches applied and pushed