Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#348 cdk-jchempaint renderbasic

Needs_Review
closed
nobody
cdk-1.4.x (181)
5
2012-10-28
2011-05-29
Egon Willighagen
No

This report is for the CDK-JChemPaint renderbasic module patch. The patch is not fully clean with respect to CDK guidelines. But, as explained on the cdk-jchempaint mailing list, I will not want to wait with CDK 1.4.0 much longer. So, this patch is here, the patch is now. If accepted, it will go in. If not, we'll further clean up and it will come later. I am not asking for special treatment here, and hope the reviewers will do a good job. There is some time for clean up.

The full set of patches is available at:

https://github.com/egonw/cdk/tree/283-14x-renderbasic

This is the result of hard work from Niels Out, Stefan Kuhn, Arvid Berg, Mark Rijnbeek, Gilleain Torrance and me (and as such, a joint project between the groups in Uppsala and at the EBI). There are also occasional constributions by others. It brings back 2D depiction of molecules, making use of the renderer API introduced earlier in the 1.4.x series.

I suggest to not review all those patches as in that above patch, but in the form of a collapsed patch. When approved, we'd still merge in that long set of patches, but a single patch files is easier to review.

Moreover, I would like the reviewers to leave the comments on GitHub, where I uploaded this single patch:

https://github.com/egonw/cdk/commit/f4af0e9527f5c0a7a4606a0957c7a20a64127a9c

(This patch must not be commited, and only exists for review!)

I note that there are some changes to stuff from the render module:

modified: src/main/org/openscience/cdk/renderer/IRenderer.java

modified: src/main/org/openscience/cdk/renderer/RendererModel.java

modified: src/main/org/openscience/cdk/renderer/elements/TextElement.java

modified: src/main/org/openscience/cdk/renderer/generators/IGenerator.java

modified: src/main/org/openscience/cdk/renderer/selection/IChemObjectSelection.java

These would need special attention, and extra double checking. Most changes are new files.

The nightly reports are available here:

http://pele.farmbio.uu.se/nightly-jcp/

There are no open JavaDoc issues:

http://pele.farmbio.uu.se/nightly-jcp/ojdcheck/renderbasic.html

There are many missing unit tests, and a few failing tests:

http://pele.farmbio.uu.se/nightly-jcp/test/result-renderbasic.html

And there are still about 50 PMD warnings left, mostly minor:

http://pele.farmbio.uu.se/nightly-jcp/pmd/renderbasic.html

Obviously, the reviewers will have to come from outside the EBI and Uppsala labs. Everyone, however, is qualified to give his/her opinion on the patch.

The final decision will have to be made by Rajarshi, though.

Discussion

  • Rajarshi, I have uploaded a few more commits today, some addressing issues reported by you, but also some bugs (fixing 4 failing unit tests):

    https://github.com/egonw/cdk/commits/283-14x-renderbasic

    There is one patch that needs application to CDK itself, being in the render module, which is already part of the cdk-1.4.x series, which I filed elsewhere.

     
  • Rajarshi Guha
    Rajarshi Guha
    2011-06-06

    Took a look at the commits - seems pretty much OK.

    I tried applying the 1.4.x patch, but that fails on the latest 1.4.x code

     
  • Pushed into cdk-1.4.x.