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: [email protected] For additional commands, e-mail: [email protected]
