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 Mon, 13 Dec 2010 14:54:44 GMT
I think I have addressed all concerns now. I've switched from using
equals() to ColorUtil.isSameColor(Color, Color). That should take care
of the equals contract problem. If the color branch is now in a state
that everybody is happy with, I'll restart the vote.

On 03.11.2010 08:44:41 Jeremias Maerki wrote:
> 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
> 




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