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