On Tue, Nov 26, 2013 at 12:31 AM, Damjan Jovanovic <dam...@apache.org>wrote:
> 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. > I think the point is that ByteOrder is a JRE provided class, we should not reinvent the wheel, unless we need to support middle-endian. Does Java run on PDP-11? ;) Gary > > - 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 > > -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> Spring Batch in Action <http://www.manning.com/templier/> Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
