xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremias Maerki <...@jeremias-maerki.ch>
Subject Re: [VOTE] Merge XGC color branch into Trunk
Date Wed, 03 Nov 2010 07:44:41 GMT
On 01.11.2010 21:29:14 Vincent Hennebert wrote:
> A few issues spotted after a quick review:
> • there’s a non-ascii character (‘°’) in CIELabColorSpace.java

fixed

> • ColorExtTest should be renamed into ColorWithAlternativesTest

fixed

> • PSGenerator:
>   • establishColorFromColor always returns true
>   • therefore, the call to establishDeviceRBG in convertColorToPS will
>     never be made and is unnecessary

Yep, something's off there. Will look into it.

> • ICCColorSpaceExt
>   • the name of the class is not informative. Better use
>     ICCColorSpaceWithIntent or something like that

This class comes from Batik. Will have to check what I can do here. But
it is a reminder that I've actually been breaking
backwards-compatibility of Batik's SVGColorProfileElementBridge.

>   • 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

I guess it makes sense to make the methods private as long as they are
not used. Anyway, throwing UnsupportedOperationException doesn't work in
this case. Batik or FOP would crash whenever a color profile is
encountered that has a rendering intent that is not implemented. The
implemented way ensures that the user gets at least an approximate
result.

> • there are missing svn properties on GrayScaleColorConverter and
>   ColorWithAlternatives

...which you could have easily fixed yourself in the time it took you to
write this down. Fixed.

> 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.

Haven't I already brought this up and documented on the Wiki?
http://wiki.apache.org/xmlgraphics/ColorHandling

I guess I'll have to revisit the decisions from back then if two people
are not happy with the current approach.

> 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



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