commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [VOTE] Release Imaging 1.0 from RC7
Date Tue, 26 Nov 2013 14:03:52 GMT
On Tue, Nov 26, 2013 at 12:31 AM, Damjan Jovanovic <damjan@apache.org>wrote:

> On Mon, Nov 25, 2013 at 1:50 PM, Emmanuel Bourg <ebourg@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.
>


The 1.0 release has not happened yet, quickly or otherwise ;) and that's
OK. I am grateful that Emmanuel was willing and able put the time in and
effort for a his review.

Damjan, you done a great job of patiently going through RCs, it's a slow
grind but we are getting there.

I know that we agreed on a road map for 1.0 and tentatively for 2.0 (where
is that thread...? ;), but we all have different schedules and priorities,
and no hard schedule for [imaging], so I see Emmanuel input and commits as
welcomed improvements.

The [VOTE] is still open of course, but Emmanuel's recent work seem worthy
of inclusion in 1.0. If we have volunteers willing to put in time to polish
this release, that much the better IMO. Even if 1.0 might be supplanted by
a 2.0, and who knows when that will be (look how long it tool [pool2] to
come out, years it seems), we might as well present the best 1.0 we can.

A short-term plan for 1.0 could be to keep the momentum Emmanuel and Damjan
have _right now_, resolve the issues in this thread, cut another RC and
then I'd bet we'd get a 1.0 ASAP.

Gary


>
> > - 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-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message