From: Egon W. <ego...@gm...> - 2008-11-26 13:39:58
|
Stefan, please revert this Controller2DHub patch, as there already is addPhenyl(). Egon On Wed, Nov 26, 2008 at 2:27 PM, <sh...@us...> wrote: > Revision: 13301 > http://cdk.svn.sourceforge.net/cdk/?rev=13301&view=rev > Author: shk3 > Date: 2008-11-26 13:27:09 +0000 (Wed, 26 Nov 2008) > > Log Message: > ----------- > adding benzene works > > Modified Paths: > -------------- > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java > > Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java > =================================================================== > --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java 2008-11-26 13:16:12 UTC (rev 13300) > +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java 2008-11-26 13:27:09 UTC (rev 13301) > @@ -37,19 +37,27 @@ > public class AddRingModule extends ControllerModuleAdapter { > > private int ringSize; > + private boolean benzene=false; > > - public AddRingModule(IChemModelRelay chemModelRelay, int ringSize) { > + public AddRingModule(IChemModelRelay chemModelRelay, int ringSize, boolean benzene) { > super(chemModelRelay); > this.ringSize=ringSize; > + this.benzene=benzene; > } > > public void mouseClickedDown(Point2d worldCoord) { > > IAtom closestAtom = chemModelRelay.getClosestAtom(worldCoord); > if (closestAtom == null) { > - chemModelRelay.addRing(ringSize); > + if(benzene) > + chemModelRelay.addRing(ringSize,true); > + else > + chemModelRelay.addRing(ringSize); > } else { > - chemModelRelay.addRing(closestAtom, ringSize); > + if(benzene) > + chemModelRelay.addRing(closestAtom, ringSize,true); > + else > + chemModelRelay.addRing(closestAtom, ringSize); > } > chemModelRelay.updateView(); > } > > Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java > =================================================================== > --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java 2008-11-26 13:16:12 UTC (rev 13300) > +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java 2008-11-26 13:27:09 UTC (rev 13301) > @@ -420,7 +420,18 @@ > } > > public void addRing(int ringSize) { > + addRing(ringSize); > + } > + > + public void addRing(int ringSize, boolean makeBenzene) { > IRing ring = chemModel.getBuilder().newRing(ringSize, "C"); > + if (makeBenzene) { > + // make newRing a benzene ring > + ring.getBond(0).setOrder(IBond.Order.DOUBLE); > + ring.getBond(2).setOrder(IBond.Order.DOUBLE); > + ring.getBond(4).setOrder(IBond.Order.DOUBLE); > + makeRingAromatic(ring); > + } > System.err.println("making ring of size " + ringSize + " actual = " + ring.getAtomCount()); > double bondLength = 2.5; // err... > ringPlacer.placeRing(ring, new Point2d(0, 0), bondLength); > @@ -437,7 +448,7 @@ > addRing(atom, 6, true); > } > > - private void addRing(IAtom atom, int ringSize, boolean makeBenzene) { > + public void addRing(IAtom atom, int ringSize, boolean makeBenzene) { > if (makeBenzene && ringSize != 6) // explode! > return; > IAtomContainer sourceContainer = ChemModelManipulator.getRelevantAtomContainer(chemModel, atom); > > Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java > =================================================================== > --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java 2008-11-26 13:16:12 UTC (rev 13300) > +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java 2008-11-26 13:27:09 UTC (rev 13301) > @@ -48,7 +48,9 @@ > public abstract void updateImplicitHydrogenCounts(); > public void zap(); > public void addRing(int size); > + public void addRing(int size, boolean benzene); > public void addRing(IAtom atom, int size); > + public void addRing(IAtom atom, int size, boolean benzene); > public void addPhenyl(IAtom atom); > // public void addRing(IBond atom, int size); > public void cleanup(); > > > This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site. > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge > Build the coolest Linux based applications with Moblin SDK & win great prizes > Grand prize is a trip for two to an Open Source event anywhere in the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > _______________________________________________ > Cdk-commits mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-commits > -- ---- http://chem-bla-ics.blogspot.com/ |
From: gilleain t. <gil...@gm...> - 2008-11-26 14:57:11
|
Well, there is a reason. The hub now has parallel methods for a) adding a ring to an existing atom b) adding a ring to an empty canvas so, addPhenyl adds to an existing atom, while addRing(size, isBenzene) adds to a blank canvas. gilleain On Wed, Nov 26, 2008 at 1:39 PM, Egon Willighagen <ego...@gm...> wrote: > Stefan, > > please revert this Controller2DHub patch, as there already is addPhenyl(). > > Egon > > > On Wed, Nov 26, 2008 at 2:27 PM, <sh...@us...> wrote: >> Revision: 13301 >> http://cdk.svn.sourceforge.net/cdk/?rev=13301&view=rev >> Author: shk3 >> Date: 2008-11-26 13:27:09 +0000 (Wed, 26 Nov 2008) >> >> Log Message: >> ----------- >> adding benzene works >> >> Modified Paths: >> -------------- >> cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java >> cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java >> cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java >> >> Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java >> =================================================================== >> --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java 2008-11-26 13:16:12 UTC (rev 13300) >> +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/AddRingModule.java 2008-11-26 13:27:09 UTC (rev 13301) >> @@ -37,19 +37,27 @@ >> public class AddRingModule extends ControllerModuleAdapter { >> >> private int ringSize; >> + private boolean benzene=false; >> >> - public AddRingModule(IChemModelRelay chemModelRelay, int ringSize) { >> + public AddRingModule(IChemModelRelay chemModelRelay, int ringSize, boolean benzene) { >> super(chemModelRelay); >> this.ringSize=ringSize; >> + this.benzene=benzene; >> } >> >> public void mouseClickedDown(Point2d worldCoord) { >> >> IAtom closestAtom = chemModelRelay.getClosestAtom(worldCoord); >> if (closestAtom == null) { >> - chemModelRelay.addRing(ringSize); >> + if(benzene) >> + chemModelRelay.addRing(ringSize,true); >> + else >> + chemModelRelay.addRing(ringSize); >> } else { >> - chemModelRelay.addRing(closestAtom, ringSize); >> + if(benzene) >> + chemModelRelay.addRing(closestAtom, ringSize,true); >> + else >> + chemModelRelay.addRing(closestAtom, ringSize); >> } >> chemModelRelay.updateView(); >> } >> >> Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java >> =================================================================== >> --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java 2008-11-26 13:16:12 UTC (rev 13300) >> +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/Controller2DHub.java 2008-11-26 13:27:09 UTC (rev 13301) >> @@ -420,7 +420,18 @@ >> } >> >> public void addRing(int ringSize) { >> + addRing(ringSize); >> + } >> + >> + public void addRing(int ringSize, boolean makeBenzene) { >> IRing ring = chemModel.getBuilder().newRing(ringSize, "C"); >> + if (makeBenzene) { >> + // make newRing a benzene ring >> + ring.getBond(0).setOrder(IBond.Order.DOUBLE); >> + ring.getBond(2).setOrder(IBond.Order.DOUBLE); >> + ring.getBond(4).setOrder(IBond.Order.DOUBLE); >> + makeRingAromatic(ring); >> + } >> System.err.println("making ring of size " + ringSize + " actual = " + ring.getAtomCount()); >> double bondLength = 2.5; // err... >> ringPlacer.placeRing(ring, new Point2d(0, 0), bondLength); >> @@ -437,7 +448,7 @@ >> addRing(atom, 6, true); >> } >> >> - private void addRing(IAtom atom, int ringSize, boolean makeBenzene) { >> + public void addRing(IAtom atom, int ringSize, boolean makeBenzene) { >> if (makeBenzene && ringSize != 6) // explode! >> return; >> IAtomContainer sourceContainer = ChemModelManipulator.getRelevantAtomContainer(chemModel, atom); >> >> Modified: cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java >> =================================================================== >> --- cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java 2008-11-26 13:16:12 UTC (rev 13300) >> +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java 2008-11-26 13:27:09 UTC (rev 13301) >> @@ -48,7 +48,9 @@ >> public abstract void updateImplicitHydrogenCounts(); >> public void zap(); >> public void addRing(int size); >> + public void addRing(int size, boolean benzene); >> public void addRing(IAtom atom, int size); >> + public void addRing(IAtom atom, int size, boolean benzene); >> public void addPhenyl(IAtom atom); >> // public void addRing(IBond atom, int size); >> public void cleanup(); >> >> >> This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site. >> >> ------------------------------------------------------------------------- >> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge >> Build the coolest Linux based applications with Moblin SDK & win great prizes >> Grand prize is a trip for two to an Open Source event anywhere in the world >> http://moblin-contest.org/redirect.php?banner_id=100&url=/ >> _______________________________________________ >> Cdk-commits mailing list >> Cdk...@li... >> https://lists.sourceforge.net/lists/listinfo/cdk-commits >> > > > > -- > ---- > http://chem-bla-ics.blogspot.com/ > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge > Build the coolest Linux based applications with Moblin SDK & win great prizes > Grand prize is a trip for two to an Open Source event anywhere in the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel > |
From: Egon W. <ego...@gm...> - 2008-11-26 16:49:47
|
On Wed, Nov 26, 2008 at 3:57 PM, gilleain torrance <gil...@gm...> wrote: > Well, there is a reason. > > The hub now has parallel methods for > > a) adding a ring to an existing atom > b) adding a ring to an empty canvas So, how does a boolean isBenzene help here?? Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Stefan K. <ste...@eb...> - 2008-11-26 15:10:41
|
Well, adding Phenyl is different from adding a spiro benzene, which is what old jcp did. The addPhenyl isn't used somewhere right now. We could use addPhenyl if a benzene ring is added to an atom, but we still need adding benzene as a fused ring (which isn't there for any ring right now, but we need it). So there is a difference. Stefan On Wednesday 26 November 2008 13:39:56 Egon Willighagen wrote: > Stefan, > > please revert this Controller2DHub patch, as there already is addPhenyl(). > > Egon > > On Wed, Nov 26, 2008 at 2:27 PM, <sh...@us...> wrote: > > Revision: 13301 > > http://cdk.svn.sourceforge.net/cdk/?rev=13301&view=rev > > Author: shk3 > > Date: 2008-11-26 13:27:09 +0000 (Wed, 26 Nov 2008) > > > > Log Message: > > ----------- > > adding benzene works > > > > Modified Paths: > > -------------- > > > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/A > >ddRingModule.java > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/C > >ontroller2DHub.java > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/I > >ChemModelRelay.java > > > > Modified: > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/A > >ddRingModule.java > > =================================================================== --- > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/A > >ddRingModule.java 2008-11-26 13:16:12 UTC (rev 13300) +++ > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/A > >ddRingModule.java 2008-11-26 13:27:09 UTC (rev 13301) @@ -37,19 +37,27 @@ > > public class AddRingModule extends ControllerModuleAdapter { > > > > private int ringSize; > > + private boolean benzene=false; > > > > - public AddRingModule(IChemModelRelay chemModelRelay, int > > ringSize) { + public AddRingModule(IChemModelRelay chemModelRelay, > > int ringSize, boolean benzene) { super(chemModelRelay); > > this.ringSize=ringSize; > > + this.benzene=benzene; > > } > > > > public void mouseClickedDown(Point2d worldCoord) { > > > > IAtom closestAtom = > > chemModelRelay.getClosestAtom(worldCoord); if (closestAtom == null) { > > - chemModelRelay.addRing(ringSize); > > + if(benzene) > > + chemModelRelay.addRing(ringSize,true); > > + else > > + chemModelRelay.addRing(ringSize); > > } else { > > - chemModelRelay.addRing(closestAtom, ringSize); > > + if(benzene) > > + chemModelRelay.addRing(closestAtom, > > ringSize,true); + else > > + chemModelRelay.addRing(closestAtom, > > ringSize); } > > chemModelRelay.updateView(); > > } > > > > Modified: > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/C > >ontroller2DHub.java > > =================================================================== --- > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/C > >ontroller2DHub.java 2008-11-26 13:16:12 UTC (rev 13300) +++ > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/C > >ontroller2DHub.java 2008-11-26 13:27:09 UTC (rev 13301) @@ -420,7 > > +420,18 @@ > > } > > > > public void addRing(int ringSize) { > > + addRing(ringSize); > > + } > > + > > + public void addRing(int ringSize, boolean makeBenzene) { > > IRing ring = chemModel.getBuilder().newRing(ringSize, "C"); > > + if (makeBenzene) { > > + // make newRing a benzene ring > > + ring.getBond(0).setOrder(IBond.Order.DOUBLE); > > + ring.getBond(2).setOrder(IBond.Order.DOUBLE); > > + ring.getBond(4).setOrder(IBond.Order.DOUBLE); > > + makeRingAromatic(ring); > > + } > > System.err.println("making ring of size " + ringSize + " actual = > > " + ring.getAtomCount()); double bondLength = 2.5; // err... > > ringPlacer.placeRing(ring, new Point2d(0, 0), bondLength); > > @@ -437,7 +448,7 @@ > > addRing(atom, 6, true); > > } > > > > - private void addRing(IAtom atom, int ringSize, boolean makeBenzene) > > { + public void addRing(IAtom atom, int ringSize, boolean makeBenzene) > > { if (makeBenzene && ringSize != 6) // explode! > > return; > > IAtomContainer sourceContainer = > > ChemModelManipulator.getRelevantAtomContainer(chemModel, atom); > > > > Modified: > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/I > >ChemModelRelay.java > > =================================================================== --- > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/I > >ChemModelRelay.java 2008-11-26 13:16:12 UTC (rev 13300) +++ > > cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/I > >ChemModelRelay.java 2008-11-26 13:27:09 UTC (rev 13301) @@ -48,7 > > +48,9 @@ > > public abstract void updateImplicitHydrogenCounts(); > > public void zap(); > > public void addRing(int size); > > + public void addRing(int size, boolean benzene); > > public void addRing(IAtom atom, int size); > > + public void addRing(IAtom atom, int size, boolean benzene); > > public void addPhenyl(IAtom atom); > > // public void addRing(IBond atom, int size); > > public void cleanup(); > > > > > > This was sent by the SourceForge.net collaborative development platform, > > the world's largest Open Source development site. > > > > ------------------------------------------------------------------------- > > This SF.Net email is sponsored by the Moblin Your Move Developer's > > challenge Build the coolest Linux based applications with Moblin SDK & > > win great prizes Grand prize is a trip for two to an Open Source event > > anywhere in the world > > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > > _______________________________________________ > > Cdk-commits mailing list > > Cdk...@li... > > https://lists.sourceforge.net/lists/listinfo/cdk-commits -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Egon W. <ego...@gm...> - 2008-11-26 16:51:44
|
On Wed, Nov 26, 2008 at 4:10 PM, Stefan Kuhn <ste...@eb...> wrote: > Well, adding Phenyl is different from adding a spiro benzene, which is what > old jcp did. The addPhenyl isn't used somewhere right now. We could use > addPhenyl if a benzene ring is added to an atom, but we still need adding > benzene as a fused ring (which isn't there for any ring right now, but we > need it). So there is a difference. There already is a outcommented stub for adding rings to a bond. But using a isBenzene boolean is not the way forward... it's only applicable if the other parameter has a very specific value... it's bad design. Moreover, the hack you actually submitted has nothing to do with spiro rings or adding rings to bonds, changes API, and what more... I was already working on this... so why did you not just ask me to do it, instead of changing existing code? Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Stefan K. <ste...@eb...> - 2008-11-26 18:38:58
|
> bad design. Moreover, the hack you actually submitted has nothing to > do with spiro rings or adding rings to bonds Just to make this clear: The code was not supposed to have something to do with that. I was just supposed to make the benzene button work, and I used existing code for this, so it got existing weaknesses, i. e. not adding to bonds. This would have been another task. -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Stefan K. <ste...@eb...> - 2008-11-26 17:42:51
|
Please feel free to do your own solution. That we clashed in our efforts is bad luck, but we didn't talk about each bit we do, did we? The solution with the isBenzene wasn't mine, it was there before (look in revision 13290 [the one before I did the benzene] of Controller(2D)Hub, it has private void addRing(IAtom atom, int ringSize, boolean makeBenzene)). My initial idea was to have a isAromatic, but then I saw this isBenzene ... As I said, feel free to do something better, but please don't blame me. Sorry to say this, but I feel a bit unfairly treated. If you think we should talk about each bit we do, ok, but then it's not just me who should ask. I would have said as long as a lot of things aren't there, we just continue. If all menus are working etc. we file bug reports about everything we find missing and synchronize via this. Stefan On Wednesday 26 November 2008 16:49:04 Egon Willighagen wrote: > On Wed, Nov 26, 2008 at 4:10 PM, Stefan Kuhn <ste...@eb...> wrote: > > Well, adding Phenyl is different from adding a spiro benzene, which is > > what old jcp did. The addPhenyl isn't used somewhere right now. We could > > use addPhenyl if a benzene ring is added to an atom, but we still need > > adding benzene as a fused ring (which isn't there for any ring right now, > > but we need it). So there is a difference. > > There already is a outcommented stub for adding rings to a bond. > > But using a isBenzene boolean is not the way forward... it's only > applicable if the other parameter has a very specific value... it's > bad design. Moreover, the hack you actually submitted has nothing to > do with spiro rings or adding rings to bonds, changes API, and what > more... I was already working on this... so why did you not just ask > me to do it, instead of changing existing code? > > Egon -- Stefan Kuhn B. Sc. M. A. Software Engineer in the Chemoinformatics and Metabolism Team European Bioinformatics Institute (EBI) Wellcome Trust Genome Campus Hinxton, Cambridge CB10 1SD UK Phone +44 1223 49 2657 Fax +44 (0)1223 494 468 |
From: Egon W. <ego...@gm...> - 2008-11-26 20:14:44
|
On Wed, Nov 26, 2008 at 6:42 PM, Stefan Kuhn <ste...@eb...> wrote: > Please feel free to do your own solution. See commit 13321. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Egon W. <ego...@gm...> - 2008-11-26 19:25:33
|
On Wed, Nov 26, 2008 at 6:42 PM, Stefan Kuhn <ste...@eb...> wrote: > Please feel free to do your own solution. > That we clashed in our efforts is bad luck, but we didn't talk about each bit > we do, did we? No, but extending IChemObjectRelay was my assignment on the workshop... see wiki. > The solution with the isBenzene wasn't mine, it was there before (look in > revision 13290 [the one before I did the benzene] of Controller(2D)Hub, it > has private void addRing(IAtom atom, int ringSize, boolean makeBenzene)). +++ cdk/branches/jchempaint-primary/src/main/org/openscience/cdk/controller/IChemModelRelay.java 2008-11-26 13:27:09 UTC (rev 13301) @@ -48,7 +48,9 @@ public abstract void updateImplicitHydrogenCounts(); public void zap(); public void addRing(int size); + public void addRing(int size, boolean benzene); public void addRing(IAtom atom, int size); + public void addRing(IAtom atom, int size, boolean benzene); public void addPhenyl(IAtom atom); // public void addRing(IBond atom, int size); public void cleanup(); that's your commit... I don't see anything removed, so I hope you understand that I ascribe the benzene boolean to be your work... > My > initial idea was to have a isAromatic, but then I saw this isBenzene ... As I > said, feel free to do something better, but please don't blame me. > Sorry to say this, but I feel a bit unfairly treated. If you think we should > talk about each bit we do, ok, but then it's not just me who should ask. We don't have to talk each bit through, but neither should we change things other people are working on with discussing that with the one who is working on it. Or, send a patch to the one working on it, or commit a patch, and ask this person to verify it, or whatever... > I would have said as long as a lot of things aren't there, we just continue. No, that is not what we should be doing. If we liked the old JCP code, we were not rewriting it. One should not just copy/paste code, without cleaning up the design. That's at best a step in place. > If all menus are working etc. we file bug reports about everything we find > missing and synchronize via this. Sorry, but I do not want to work that way. This JCP rewrite should be about code cleanup, not blindly adding old code. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Stefan K. <ste...@eb...> - 2008-11-27 09:34:52
|
> that's your commit... I don't see anything removed, so I hope you > understand that I ascribe the benzene boolean to be your work... Controller2DHub always (at least before my commit) had private void addRing(IAtom atom, int ringSize, boolean makeBenzene) and the only thing I did was to link that code to the button... Not a big thing, is it? |
From: Egon W. <ego...@gm...> - 2008-11-27 09:47:04
|
On Thu, Nov 27, 2008 at 10:34 AM, Stefan Kuhn <ste...@eb...> wrote: >> that's your commit... I don't see anything removed, so I hope you >> understand that I ascribe the benzene boolean to be your work... > Controller2DHub always (at least before my commit) had > private void addRing(IAtom atom, int ringSize, boolean makeBenzene) > and the only thing I did was to link that code to the button... Not a big > thing, is it? No, that's not a big thing... but you also added it to the IChemModelRelay interface... that is a big thing. Your idea of a isAromatic boolean was much better than the isBenzene hack that *I* copied... (which I now removed)... but instead of just going along then, you should bring that up on the list, and say... "hey, that does not make sense". Point is... a lot of CDK code does not (or no longer) make sense. And it only gets worse if we do no peer review it, and just accept what it is. That's why I complain about things that just don't make sense. Actually, my original email did not intend to put any blame anywhere, and I don't think I did; I merely asked to revert the patch, because it was duplicate code. The only reason I directed the email at you, is that you commited something which did not make sense to me. If you see code *I* commited that does not make sense, I expect people to direct email at me too. That's peer review. Egon -- ---- http://chem-bla-ics.blogspot.com/ |