From: Thorsten M. <Tho...@un...> - 2008-09-05 09:17:50
Attachments:
Java2DRenderer.patch
|
Hi all, I just started using the 1.2 codebase for our KNIME nodes. Some small comments: - Quite a lot of methods, constants, classes are one now and have been replaced by others. You should prepare a list what is gone and what to use instead. An example being CDKConstants.BONDORDER_AROMATIC. This one has just silenty gone away. By searching the mailing lists if found, that one has to use get/setFlag now. - You may add an svn-Property svn:eol-style=native to all source files. This makes sure the line endings are corrected upon checking out the files. So *nix users get LR, Windows get CRLF transparently. - There are quite a lot of warnings in my Eclipse that collection are used without generics. This looks like an even easier "mass-patch" than the iterable one, Egon applied, as Eclipse can infer the generic types automatically. If it cannot, then there it is something suspicious with it anyway. - Maybe you could define a standard set of Eclipse settings for warnings/errors, so that everyone has the same and tries to make his/her code comply warning-free. This at least looks much better and sometime also avoids error (I find the null-access check warnings quite nice). Of course this only works for Eclipse users, but still this is better than nothing. - The same holds for formatting settings, like tab sizes, places of parentheses, etc. You can export these settings from Eclipse and import them in another installation. That was quite useful for KNIME, too. Then I tried out the new Java2DRenderer it found it quite nice, looking much better than the old one :-) However, it still has some problems/bugs. I tried to fix at least some of them, specifically - Atom symbol fonts were way to big (at least on my computer), so I made them smaller - Hydrogens were displayed even if they were disabled in the renderer model (bonds were already omitted) - The atom symbols background is now a circle instead of a rectangle and is filled with the graphics object background color if the renderer's model background color is null. Maybe this can even made the default, currently it is white which looks bad on the standard-gray background of AWT/Swing components. - Fixed some generics-related warnings. - Removed all System.out and made them logger.debug, as the renderer is extremely chatty and floods stdout. I attached a patch for this. Maybe someone can have a look at it. If it's OK I will check this one in. BTW, how are you going to handle patches on the branch? Merging them back to trunk immediately or doing a "bulk-merge" if 1.2 gets stable? In KNIME we are doing the former. Regards, Thorsten -- Thorsten Meinl room: Z815 Nycomed Chair for Bioinformatics fax: +49 7531 88-5132 and Information Mining phone: +49 7531 88-5016 Box 712, 78457 Konstanz, Germany |
From: Egon W. <ego...@gm...> - 2008-09-05 16:02:23
|
On Fri, Sep 5, 2008 at 11:17 AM, Thorsten Meinl <Tho...@un...> wrote: > I just started using the 1.2 codebase for our KNIME nodes. Some small > comments: > - Quite a lot of methods, constants, classes are one now and have been > replaced by others. You should prepare a list what is gone and what to use > instead. Yes, indeed (not sure about having the resources yet, but we might be able to aggregate user questions). Some help on where things have changed can be found at: http://cheminfo.informatics.indiana.edu/~rguha/code/java/nightly/apicomp.html > An example being CDKConstants.BONDORDER_AROMATIC. This one has just > silenty gone away. By searching the mailing lists if found, that one has to > use get/setFlag now. API has slightly diverged. Please use IBond.set/getFlag(CDKConstants.BONDORDER_AROMATIC). > - You may add an svn-Property svn:eol-style=native to all source files. This > makes sure the line endings are corrected upon checking out the files. So > *nix users get LR, Windows get CRLF transparently. Please file a project maintainance task. Sounds good. > - There are quite a lot of warnings in my Eclipse that collection are used > without generics. This looks like an even easier "mass-patch" than the > iterable one, Egon applied, as Eclipse can infer the generic types > automatically. If it cannot, then there it is something suspicious with it > anyway. Mmm... yeah, would have guessed so too. But the thing is, I have converted some classes manually, and did encounter inconsistencies... e.g. bug #2028685. So, yes, I'd love to see CDK 1.2.0 using generics throughout... Thorsten, do you think you can post an overview of all eclipse warnings about generics online? So that we can identify the hotspots? > - Maybe you could define a standard set of Eclipse settings for > warnings/errors, so that everyone has the same and tries to make his/her > code comply warning-free. This at least looks much better and sometime also > avoids error (I find the null-access check warnings quite nice). Of course > this only works for Eclipse users, but still this is better than nothing. There would be room for that in the CDK policy... one concern is, however, that not every is using Eclipse. BTW, we already use PMD to do some checking... > - The same holds for formatting settings, like tab sizes, places of > parentheses, etc. You can export these settings from Eclipse and import them > in another installation. That was quite useful for KNIME, too. Yes, Bioclipse is using that too... > Then I tried out the new Java2DRenderer it found it quite nice, looking much > better than the old one :-) However, it still has some problems/bugs. I > tried to fix at least some of them, specifically Yes, confirmed. The code is yet far from being a replacement for the old code. See my reply on your later email. > - Atom symbol fonts were way to big (at least on my computer), so I made > them smaller Yeah, I was having trouble they were too small... need to work out how and why... > - Hydrogens were displayed even if they were disabled in the renderer model > (bonds were already omitted) > - The atom symbols background is now a circle instead of a rectangle and is > filled with the graphics object background color if the renderer's model > background color is null. Maybe this can even made the default, currently it > is white which looks bad on the standard-gray background of AWT/Swing > components. > - Fixed some generics-related warnings. > - Removed all System.out and made them logger.debug, as the renderer is > extremely chatty and floods stdout. > > I attached a patch for this. Maybe someone can have a look at it. If it's OK > I will check this one in. Feel free to open a branch. I figured out how to use Git for branch merging, this that works terrific. > BTW, how are you going to handle patches on the branch? Merging them back to > trunk immediately or doing a "bulk-merge" if 1.2 gets stable? In KNIME we > are doing the former. I'll merge every now and then; Git is really cool about that. So, somewhere between immediately and at the end. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-09-06 02:49:44
|
On Sep 5, 2008, at 12:02 PM, Egon Willighagen wrote: > Yes, indeed (not sure about having the resources yet, but we might be > able to aggregate user questions). Maybe make a note on the Wiki? >> - Maybe you could define a standard set of Eclipse settings for >> warnings/errors, so that everyone has the same and tries to make >> his/her >> code comply warning-free. This at least looks much better and >> sometime also >> avoids error (I find the null-access check warnings quite nice). >> Of course >> this only works for Eclipse users, but still this is better than >> nothing. > > There would be room for that in the CDK policy... one concern is, > however, that not every is using Eclipse. If anybody's using IDEA, I highly recommend listening to their warnings - I usually go ahead and just accept their fixes when editing CDK code that hasn't been generic'ized yet and it does a nice job. ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- So the Zen master asked the hot-dog vendor, "Can you make me one with everything?" - TauZero on Slashdot |
From: Egon W. <ego...@gm...> - 2008-09-06 19:59:03
|
On Fri, Sep 5, 2008 at 6:01 PM, Egon Willighagen <ego...@gm...> wrote: >> - There are quite a lot of warnings in my Eclipse that collection are used >> without generics. This looks like an even easier "mass-patch" than the >> iterable one, Egon applied, as Eclipse can infer the generic types >> automatically. If it cannot, then there it is something suspicious with it >> anyway. > > Mmm... yeah, would have guessed so too. But the thing is, I have > converted some classes manually, and did encounter inconsistencies... > e.g. bug #2028685. Another problem here, is that there are cases where not just one type of object is put into a List, so one would have to say something like List<Double|String> ... :( Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: gilleain t. <gil...@gm...> - 2008-09-06 20:12:32
|
On the subject of generics, there's an odd one in the ConformerContainer (in the org.openscience.cdk package): @TestMethod("testToArray_arrayObject") public <IAtomContainer> IAtomContainer[] toArray(IAtomContainer[] ts) { throw new UnsupportedOperationException(); } which gives an error in Eclipse of "The type parameter IAtomContainer is hiding the type IAtomContainer". I think it's just a mistake, given the name of the method... I could file it as a bug, but it seems trivial. gilleain On Sat, Sep 6, 2008 at 8:59 PM, Egon Willighagen <ego...@gm...> wrote: > On Fri, Sep 5, 2008 at 6:01 PM, Egon Willighagen > <ego...@gm...> wrote: >>> - There are quite a lot of warnings in my Eclipse that collection are used >>> without generics. This looks like an even easier "mass-patch" than the >>> iterable one, Egon applied, as Eclipse can infer the generic types >>> automatically. If it cannot, then there it is something suspicious with it >>> anyway. >> >> Mmm... yeah, would have guessed so too. But the thing is, I have >> converted some classes manually, and did encounter inconsistencies... >> e.g. bug #2028685. > > Another problem here, is that there are cases where not just one type > of object is put into a List, so one would have to say something like > List<Double|String> ... :( > > Egon > > -- > ---- > 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-09-06 20:16:33
|
On Sat, Sep 6, 2008 at 10:12 PM, gilleain torrance <gil...@gm...> wrote: > On the subject of generics, there's an odd one in the > ConformerContainer (in the org.openscience.cdk package): > > @TestMethod("testToArray_arrayObject") > public <IAtomContainer> IAtomContainer[] toArray(IAtomContainer[] ts) { > throw new UnsupportedOperationException(); > } > > which gives an error in Eclipse of "The type parameter IAtomContainer > is hiding the type IAtomContainer". I think it's just a mistake, given > the name of the method... > > I could file it as a bug, but it seems trivial. Best to file it anyway, unless you know what the correct return value should be, and fix it directly. Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-09-07 01:09:44
|
On Sep 6, 2008, at 4:12 PM, gilleain torrance wrote: > On the subject of generics, there's an odd one in the > ConformerContainer (in the org.openscience.cdk package): > > @TestMethod("testToArray_arrayObject") > public <IAtomContainer> IAtomContainer[] toArray(IAtomContainer > [] ts) { > throw new UnsupportedOperationException(); > } > > which gives an error in Eclipse of "The type parameter IAtomContainer > is hiding the type IAtomContainer". I think it's just a mistake, given > the name of the method... Actually, it s weird - there seem to be two return types? And the input type is same as one of the return types A bug report would be good - please assign to me ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- So the Zen master asked the hot-dog vendor, "Can you make me one with everything?" - TauZero on Slashdot |
From: Thorsten M. <Tho...@un...> - 2008-09-08 08:40:57
|
> Please file a project maintainance task. Sounds good. Just done. > Mmm... yeah, would have guessed so too. But the thing is, I have > converted some classes manually, and did encounter inconsistencies... > e.g. bug #2028685. Jep, these are cases where one should have a deep look anyway. If its really intended, that two different types of objects are stored within a container you can write List<?> to the rescue. But it is strange. > Thorsten, do you think you can post an overview of all eclipse > warnings about generics online? So that we can identify the hotspots? What exactly do you mean? With my settings I currently have about 15,300 warnings in CDK. Quite lot of them being generics-related but also Javadoc and others. > There would be room for that in the CDK policy... one concern is, > however, that not every is using Eclipse. Yes, true. But for those using it, it can't hurt. I forgot, that you can even set project specific settings inside the project so that everyone checking it out gets them automatically. This would be quite a good approach I believe. > BTW, we already use PMD to do some checking... Need to have a look at this one. > Feel free to open a branch. I figured out how to use Git for branch > merging, this that works terrific. I will open one and fix things I come across while integrating CDK 1.2 into KNIME. -- Thorsten Meinl room: Z815 Nycomed Chair for Bioinformatics fax: +49 (0)7531 88-5132 and Information Mining phone: +49 (0)7531 88-5016 Box 712, 78457 Konstanz, Germany |
From: Egon W. <ego...@gm...> - 2008-09-08 10:08:00
|
On Mon, Sep 8, 2008 at 10:40 AM, Thorsten Meinl <Tho...@un...> wrote: >> Please file a project maintainance task. Sounds good. > Just done. Thanx. >> Mmm... yeah, would have guessed so too. But the thing is, I have >> converted some classes manually, and did encounter inconsistencies... >> e.g. bug #2028685. > Jep, these are cases where one should have a deep look anyway. If its really > intended, that two different types of objects are stored within a container > you can write List<?> to the rescue. But it is strange. Difficult to read the code this way, for the least... >> Thorsten, do you think you can post an overview of all eclipse >> warnings about generics online? So that we can identify the hotspots? > What exactly do you mean? With my settings I currently have about 15,300 > warnings in CDK. Quite lot of them being generics-related but also Javadoc > and others. As HTML page or so... >> There would be room for that in the CDK policy... one concern is, >> however, that not every is using Eclipse. > Yes, true. But for those using it, it can't hurt. I forgot, that you can even > set project specific settings inside the project so that everyone checking it > out gets them automatically. This would be quite a good approach I believe. If you like to write this up for CDK News... >> BTW, we already use PMD to do some checking... > Need to have a look at this one. http://pmd.sf.net/ >> Feel free to open a branch. I figured out how to use Git for branch >> merging, this that works terrific. > I will open one and fix things I come across while integrating CDK 1.2 into > KNIME. OK, great! Egon -- ---- http://chem-bla-ics.blogspot.com/ |
From: Rajarshi G. <rg...@in...> - 2008-09-25 16:15:29
|
On Sep 5, 2008, at 5:17 AM, Thorsten Meinl wrote: > Then I tried out the new Java2DRenderer it found it quite nice, > looking much better than the old one :-) However, it still has some > problems/bugs. I tried to fix at least some of them, specifically [snip] > I attached a patch for this. Maybe someone can have a look at it. > If it's OK I will check this one in. have these patches been merged into trunk or 1.2.x/ ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: D070 5427 CC5B 7938 929C DD13 66A1 922C 51E7 9E84 ------------------------------------------------------------------- The most important statistic for car manufacturers is autocorrelation. |
From: Thorsten M. <Tho...@un...> - 2008-09-25 17:06:29
|
Am Donnerstag, 25. September 2008 schrieb Rajarshi Guha: > have these patches been merged into trunk or 1.2.x/ Nope, not yet. I had planned to open up a branch separate and put things I discover during updating the KNIME nodes for 2.0 to CDK 1.2. However, for time reasons we decided to ship CDK 1.0 with KNIME 2.0 and release an updated version with CDK 1.2 lateron. So I haven't done anything further. Thorsten -- Thorsten Meinl room: Z815 Nycomed Chair for Bioinformatics fax: +49 (0)7531 88-5132 and Information Mining phone: +49 (0)7531 88-5016 Box 712, 78457 Konstanz, Germany |
From: Egon W. <ego...@gm...> - 2008-09-26 10:58:41
|
On Thu, Sep 25, 2008 at 6:15 PM, Rajarshi Guha <rg...@in...> wrote: > have these patches been merged into trunk or 1.2.x/ [?] I'm about to commit to cdk1.2.x. Egon -- ---- http://chem-bla-ics.blogspot.com/ |