On Mon, Nov 25, 2013 at 1:50 PM, Emmanuel Bourg <ebo...@apache.org> wrote: > Hi Damjan,
Hi Emmanuel > I reviewed the API, here are my observations: Thank you. Firstly, we discussed several options before for the 1.0 release, and agreed that the contents of trunk would be quickly pushed out as 1.0 with minimal changes (many/most users are using 1.0-SNAPSHOT), and then the big API redesign would be 2.0. I've also been thinking of doing a complete rewrite for 2.0 and only pulling in some of the good bits we have now. So it's extremely discouraging to be pushed for more and more changes, many of which will have no post-1.x value, and don't even fit in with what was originally agreed on. > - Would that make sense to move the LZM implementation to commons-compress? I think you mean LZW, and let's discuss that on COMPRESS-243. > - What is the purpose of CachingInputStream and CachingOutputStream, > there is no Javadoc on these classes. Could they be merged into commons-io? It looks like CachingInputStream is used by IccProfileParser, and appears to be used to store data that has been read from the underlying stream so it can be re-read later. You can copy it to commons-io, but I'd prefer not having a runtime dependency on it. And it's ByteSourceInputStream you really want in commons-io and/or commons-compress, a gem that allows seeking over an InputStream. > - Why is ZLibUtils in org.apache.commons.imaging.common instead of > org.apache.commons.imaging.util ? The whole org.apache.commons.imaging.util package is a bit dated. > - FastByteArrayOutputStream is only used by PackBits in the same > package, it could be made package private. Yes. > - BitArrayOutputStream in org.apache.commons.imaging.common is only used > by classes in org.apache.commons.imaging.common.itu_t4. It could be > moved into this package and made package private. Yes. > - There are several unused public methods in BinaryInputStream Yes. > - org.apache.commons.imaging.common.Compression is never used I imagine the idea was to use it instead of using the different compression classes directly, but whoever started writing it never finished. > - RgbBufferedImageFactory is only used in RoundtripTest, should this be > moved with the test sources? Was probably meant to be the way to create all images in the API, but again never got finished, and is probably wrong now since some image formats produce paletted images. > - SimpleBufferedImageFactory is only used by ImageParser and could be > made package private. And bears suspiciously high resemblance to RgbBufferedImageFactory. > - What about replacing org.apache.commons.imaging.common.ByteOrder with > java.nio.ByteOrder? Enum vs public static final, hmm. > - Several methods in BinaryFunctions have an unused 'name' parameter. Sometimes it's used to form exception text. > - UnicodeUtils is only used by PngWriter and could be made package > private. Considering that any byte sequence is a valid ISO-8859-1 string > this class could also be removed. Yes. > - Why does RationalNumberUtilities extend Number? And shouldn't this > class be named RationalNumberUtils for consistency with the other *Utils > classes? I think the class could simply be merged into RationalNumber. Merging seems best. > - There are several unused methods in ColorTools, I don't know if they > are actually useful and are just missing a unit test. Not sure. > - ZigZag is only used in JpegDecoder and could be made package private Yes. > - The public permissive field in JpegImageParser is never used Yes. > - The public Attribute_Tag field in TiffField is never used Yes. > - Some public static final constants don't have an uppercased name, I > fixed them on the trunk Thank you, but that probably broke compatibility for 1.0-SNAPSHOT users, so now we have to release RC7 as 1.0 :-). > - The PixelParser classes in > org.apache.commons.imaging.formats.bmp.pixelparsers seems to be used > only internally by BmpImageParser. They can't be made package private > but could at least be excluded from the Javadoc. Hidden Javadocs don't hide packages from IDE code completion. There is only 2 choices w.r.t. packages: keeping everything in one package to hide internal classes by giving them package private access, and keeping classes in different packages to better structure code but then having to make them public as a result (and choice 3, a pipe dream, use OSGi and don't export the packages with internal classes). Maybe a public factory method in a public base class returning package-private subclasses would work? > - Same comment for org.apache.commons.imaging.formats.bmp.writers > > - The Dct, JpegInputStream and YCbCrConverter classes in > org.apache.commons.imaging.formats.jpeg.decoder are only used internally > by JpegDecoder and could be made package private. Yes. YCbCrConverter could also be moved into ColorConversions, but I also think there's better ways to write it than the massive lookup tables I used. > - What about moving the methods in ColorConversions into the respective > Color classes? For example, ColorConversions.convertCIELuvtoXYZ() could > become ColorCieLuv.toXYZ(). Another idea would be to use a fluent > interface to perform the conversions. Something like > ColorConverter.fromRGB(r, g, b).toHSL(). Maybe the former. Some of these conversions are lossy, and there is no standard canonical color format that fromRGB() could return, so I'm not sure you want to chain them. Also color conversions are also done per pixel, so they're performance critical, and 2 method calls, 2 format conversions, and potentially an extra memory allocation in fromRGB(), are going to be very slow. Already I think those methods should be made to operate on reusable rewritable pre-allocated objects they get passed in, not allocate and return a new object per pixel. > > I haven't reviewed everything yet, but I have the feeling the public API > could be better polished to remove unused or internal stuff that are > probably not useful for the end users. Yes, there's plenty more. But users are begging for a 1.0 release, not higher API standards. Thank you again for the review - it will all get fixed or replaced at some stage. Finally, please remember: 1.0 releases are usually the ones that never work :-). > Emmanuel Bourg Damjan > Le 24/11/2013 17:54, Damjan Jovanovic a écrit : >> Please vote on releasing commons-imaging 1.0 from RC7. >> >> Last failed vote was for RC5, RC6 had a bust POM. Many improvements >> were made since: >> * PMD configured better and all PMD warnings fixed, the ones remaining >> now are bugs in PMD itself. >> * Test coverage greatly improved in many areas, only a few packages >> now have < 50% coverage, and that's due to obscure TIFF photometric >> interpreters and PSD data parsers that require special images to test >> which Imaging can't itself generate, a massive barely used >> semi-obsolete Debug class that drags down coverage for >> org.apache.commons.imaging.util, and areas of code I don't understand >> and so can't easily test (ICC, PSD). While improving test coverage, I >> also found and fixed 2 bugs. >> * Added package-level Javadoc for image formats. >> * RAT exclusions for text-based images, and made a single info.txt >> with image formation for all images and added a license header to it. >> * Fixed Jacoco configuration in the POM and started using it instead >> of that 40+ minute Cobertura nonsense. >> * Fixed website broken links. >> * Also started assembling a list of why making releases is such a >> pain, email coming later :). >> >> Imaging 1.0 RC7 is available here: >> https://dist.apache.org/repos/dist/dev/commons/imaging/ (SVN revision 3671) >> >> Maven artifacts: >> https://repository.apache.org/content/repositories/staging/org/apache/commons/commons-imaging/ >> >> Change log(s): >> https://dist.apache.org/repos/dist/dev/commons/imaging/RELEASE-NOTES.txt >> http://people.apache.org/~damjan/imaging-1.0-RC7/changes-report.html >> http://people.apache.org/~damjan/imaging-1.0-RC7/jira-report.html >> >> Tag: >> https://svn.apache.org/repos/asf/commons/proper/imaging/tags/IMAGING_1_0_RC7 >> (SVN revision 1544953) >> >> Site: >> http://people.apache.org/~damjan/imaging-1.0-RC7/ >> >> KEYS: >> http://www.apache.org/dist/commons/KEYS >> >> I have tested it with OpenJDK 6 on FreeBSD 9.1. >> >> Please review and vote. This vote will close no sooner than 72 hours >> from now, on Wednesday 27 November 2013 at 19:00 GMT. >> >> [ ] +1 Release these artifacts >> [ ] +0 OK, but... >> [ ] -0 OK, but really should fix... >> [ ] -1 I oppose this release because... >> >> Thank you! >> >> Damjan Jovanovic >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
