xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vincent Hennebert <vhenneb...@gmail.com>
Subject Re: [VOTE] Merge XGC color branch into Trunk
Date Mon, 01 Nov 2010 20:29:14 GMT
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: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org


Mime
View raw message