A few issues spotted after a quick review:
• there’s a non-ascii character (‘°’) in CIELabColorSpace.java
• ColorExtTest should be renamed into ColorWithAlternativesTest
• PSGenerator:
• establishColorFromColor always returns true
• therefore, the call to establishDeviceRBG in convertColorToPS will
never be made and is unnecessary
• ICCColorSpaceExt
• the name of the class is not informative. Better use
ICCColorSpaceWithIntent or something like that
• unimplemented methods should really throw an
UnsupportedOperationException or be removed altogether. Their
behaviour is likely to change once they are properly implemented,
introducing regressions. Putting a comment in the Javadoc is not
enough, especially since those methods can silently be called by
intendedToRGB
• there are missing svn properties on GrayScaleColorConverter and
ColorWithAlternatives
More importantly, I keep thinking that there’s a design flaw in
ColorWithAlternatives. Its equals method breaks the contract defined on
Object.equals since it’s not symmetric. This is bound to cause
hard-to-track issues in client code. Also, the way equals is implemented
will make it systematically return false if an instance of
ColorWithAlternatives is being compared with an instance of a sub-class,
which may not be the desirable result.
Either ColorWithAlternatives is not a Color and therefore should not
extend the Color class; or its equals method should be changed to follow
the contract and the comparison of ColorWithAlternatives instances
should be implemented differently.
I don’t have the time nor the energy to submit a counter-proposal.
Therefore I’ll stay out of the way and vote -0.
Vincent
On 28/10/10 09:51, Jeremias Maerki wrote:
> I would like to call for a vote to merge the XML Graphics Commons color
> branch [1] into the Trunk. I've got the code in production since August
> and it's working just fine, especially the named colors functionality.
> I've now done some cleanup work and I believe the branch is now ready to
> be merged into Trunk.
>
> Related documentation and links to further information can be found at [2].
>
>
> [1] https://svn.apache.org/repos/asf/xmlgraphics/commons/branches/Temp_Color
> [2] http://wiki.apache.org/xmlgraphics/ColorHandling
>
>
> +1 from me.
>
> Jeremias Maerki
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
