You are viewing a plain text version of this content. The canonical link for it is here.
Posted to general@xmlgraphics.apache.org by Jeremias Maerki <de...@jeremias-maerki.ch> on 2010/12/13 15:54:44 UTC
Re: [VOTE] Merge XGC color branch into Trunk
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
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Web Maestro Clay <th...@gmail.com>.
Is this VOTE still running? +0 from me if so.
"My religion is simple. My religion is kindness."
- His Holiness the Dalai Lama of Tibet
On Dec 15, 2010, at 3:58 AM, Adrian Cumiskey <ad...@gmail.com> wrote:
> I've not had time to follow so +0 from me.
>
> Adrian.
>
> On 2010-12-14 5:38 PM, "Jeremias Maerki" <de...@jeremias-maerki.ch> wrote:
>
> On 14.12.2010 10:17:04 Simon Pepping wrote:
>> On Tue, Dec 14, 2010 at 09:22:20AM +0100, Simon Peppin...
> Thanks
>
>
>> Note that NamedColorTest fails, but it did that already without my
>> patch.
> Fixed: http://svn.apache.org/viewvc?rev=1049003&view=rev
>
>
> Jeremias Maerki
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: gene...
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Adrian Cumiskey <ad...@gmail.com>.
I've not had time to follow so +0 from me.
Adrian.
On 2010-12-14 5:38 PM, "Jeremias Maerki" <de...@jeremias-maerki.ch> wrote:
On 14.12.2010 10:17:04 Simon Pepping wrote:
> On Tue, Dec 14, 2010 at 09:22:20AM +0100, Simon Peppin...
Thanks
> Note that NamedColorTest fails, but it did that already without my
> patch.
Fixed: http://svn.apache.org/viewvc?rev=1049003&view=rev
Jeremias Maerki
---------------------------------------------------------------------
To unsubscribe, e-mail: gene...
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 14.12.2010 10:17:04 Simon Pepping wrote:
> On Tue, Dec 14, 2010 at 09:22:20AM +0100, Simon Pepping wrote:
> > These are my suggestions to you. I will not again react to this issue.
> > Please merge ASAP.
>
> On second thought, done the suggested changes.
Thanks
> Note that NamedColorTest fails, but it did that already without my
> patch.
Fixed: http://svn.apache.org/viewvc?rev=1049003&view=rev
Jeremias Maerki
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Tue, Dec 14, 2010 at 09:22:20AM +0100, Simon Pepping wrote:
> These are my suggestions to you. I will not again react to this issue.
> Please merge ASAP.
On second thought, done the suggested changes.
Note that NamedColorTest fails, but it did that already without my
patch.
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Mon, Dec 13, 2010 at 09:33:17PM +0100, Jeremias Maerki wrote:
> On 13.12.2010 20:43:43 Simon Pepping wrote:
> > On Mon, Dec 13, 2010 at 03:54:44PM +0100, Jeremias Maerki wrote:
> > > 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.
> >
> > Don't you mean the following with the class test:
> >
> > //Consider same-ness only between colors of the same class (not subclasses)
> > //but consider a ColorWithAlternatives without alternatives to be the same as a Color.
> > Class<?> cl1 = col1.getClass();
> > if (cl1 == ColorWithAlternatives.class
> > && !((ColorWithAlternatives)col1).hasAlternativeColors()) {
> > cl1 = Color.class;
> > }
> > Class<?> cl2 = col2.getClass();
> > if (cl2 == ColorWithAlternatives.class
> > && !((ColorWithAlternatives)col2).hasAlternativeColors()) {
> > cl2 = Color.class;
> > }
> > if (cl1 != cl2) {
> > return false;
> > }
>
> That's about the same, isn't it?
Not quite. Case: col1 is ColorWithAlternatives and has no
alternatives, col2 is some other subclass of Color. In your
formulation skipClassTest is true, and further tests are applied; in
mine false is returned.
I feel that my formulation does straightforwardly what the comment
says it is doing: a ColorWithAlternatives without alternatives is
considered equivalent to a Color. In your formulation, there is a
complex condition, of which it is hard to see what exactly is tested:
!(col1.getClass() == ColorWithAlternatives.class
&& !((ColorWithAlternatives)col1).hasAlternativeColors())
&&
!(col2.getClass() == ColorWithAlternatives.class
&& !((ColorWithAlternatives)col2).hasAlternativeColors())
&&
(col1.getClass() != col2.getClass())
This kind of hard to understand tests make modifying of the code, when
required, difficult. I am just trying to be helpful to you and to
later developers. Feel free to ignore my suggestion.
> > Even so, subclasses of ColorWithAlternatives may come out
> > unexpectedly: An object of ColorWithAlternatives with no alternative
> > colors, and a Color object will be equal. In contrast, an object of a
> > Subclass of ColorWithAlternatives with no alternative colors, and a
> > Color object will not be equal. Why is that so?
>
> Because I know that a ColorWithAlternatives without actual alternatives
> will behave exactly the same way as the corresponding Color object. But
> I can't say the same about some subclass of ColorWithAlternatives. Maybe
> such a subclass will add functionality that was not foreseen. In such a
> case, isSameColor() will have to be updated. I guess I should just have
> skipped this test to avoid more discussions, just considering the
> objects of two different classes different. After all, this is just
> about state checking and who cares about the rarest of rare cases where
> two such color objects follow each other?
ColorWithAlternatives is bound by the behaviour of Color.equals.
Similarly, future subclasses of ColorWithAlternatives are bound by the
behaviour of their superclass.
I feel strongly about this one. Unexpected behaviour for future
subclassing is what triggered this discussion.
> > The part where alternative colors are compared, could well be a method
> > of ColorWithAlternatives, putting the logic in the right class.
>
> Do you feel strongly about that? I don't see the advantage, especially
> since that would add a public method only used by ColorUtil.
No.
These are my suggestions to you. I will not again react to this issue.
Please merge ASAP.
> Jeremias Maerki
>
> PS: anyone want to write a master thesis on this?
Sorry for being more theoretically minded. I appreciate your numerous
contributions to FOP.
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 13.12.2010 20:43:43 Simon Pepping wrote:
> On Mon, Dec 13, 2010 at 03:54:44PM +0100, Jeremias Maerki wrote:
> > 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.
>
> Don't you mean the following with the class test:
>
> //Consider same-ness only between colors of the same class (not subclasses)
> //but consider a ColorWithAlternatives without alternatives to be the same as a Color.
> Class<?> cl1 = col1.getClass();
> if (cl1 == ColorWithAlternatives.class
> && !((ColorWithAlternatives)col1).hasAlternativeColors()) {
> cl1 = Color.class;
> }
> Class<?> cl2 = col2.getClass();
> if (cl2 == ColorWithAlternatives.class
> && !((ColorWithAlternatives)col2).hasAlternativeColors()) {
> cl2 = Color.class;
> }
> if (cl1 != cl2) {
> return false;
> }
That's about the same, isn't it?
> Even so, subclasses of ColorWithAlternatives may come out
> unexpectedly: An object of ColorWithAlternatives with no alternative
> colors, and a Color object will be equal. In contrast, an object of a
> Subclass of ColorWithAlternatives with no alternative colors, and a
> Color object will not be equal. Why is that so?
Because I know that a ColorWithAlternatives without actual alternatives
will behave exactly the same way as the corresponding Color object. But
I can't say the same about some subclass of ColorWithAlternatives. Maybe
such a subclass will add functionality that was not foreseen. In such a
case, isSameColor() will have to be updated. I guess I should just have
skipped this test to avoid more discussions, just considering the
objects of two different classes different. After all, this is just
about state checking and who cares about the rarest of rare cases where
two such color objects follow each other?
> The part where alternative colors are compared, could well be a method
> of ColorWithAlternatives, putting the logic in the right class.
Do you feel strongly about that? I don't see the advantage, especially
since that would add a public method only used by ColorUtil.
Jeremias Maerki
PS: anyone want to write a master thesis on this?
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Tue, Dec 21, 2010 at 10:01:53AM +0100, Jeremias Maerki wrote:
> The color branch of Apache XML Graphics Commons is now merged into the
> trunk.
Great work. Thanks. Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
The color branch of Apache XML Graphics Commons is now merged into the
trunk.
On 20.12.2010 09:39:00 Jeremias Maerki wrote:
> Hi Helder,
>
> I haven't restarted it. Probably doesn't make sense and would only be a
> formality since I haven't got any indication that not all objections
> were sufficiently addressed. And there were no -1s in the first place.
> Anyway, I plan to do the merge tomorrow or on Wednesday and then move on
> to prepare Batik and FOP for a merge.
>
> On 19.12.2010 18:54:46 Helder Magalhães wrote:
> > Hi everyone,
> >
> > > If the color branch is now in a state
> > > that everybody is happy with, I'll restart the vote.
> >
> > Guess I've missed that one. Wasn't clear (and still isn't) that a new
> > vote round was triggered but, although I haven't followed the last
> > changes, the concept and new features seem appealing to me as
> > previously.
> >
> > Again, +1 from me and congratulations for the effort, Jeremias! ;-)
> >
> > Cheers,
> > Helder
Jeremias Maerki
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Hi Helder,
I haven't restarted it. Probably doesn't make sense and would only be a
formality since I haven't got any indication that not all objections
were sufficiently addressed. And there were no -1s in the first place.
Anyway, I plan to do the merge tomorrow or on Wednesday and then move on
to prepare Batik and FOP for a merge.
On 19.12.2010 18:54:46 Helder Magalhães wrote:
> Hi everyone,
>
> > If the color branch is now in a state
> > that everybody is happy with, I'll restart the vote.
>
> Guess I've missed that one. Wasn't clear (and still isn't) that a new
> vote round was triggered but, although I haven't followed the last
> changes, the concept and new features seem appealing to me as
> previously.
>
> Again, +1 from me and congratulations for the effort, Jeremias! ;-)
>
> Cheers,
> Helder
Jeremias Maerki
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Mon, Dec 13, 2010 at 03:54:44PM +0100, Jeremias Maerki wrote:
> 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.
Don't you mean the following with the class test:
//Consider same-ness only between colors of the same class (not subclasses)
//but consider a ColorWithAlternatives without alternatives to be the same as a Color.
Class<?> cl1 = col1.getClass();
if (cl1 == ColorWithAlternatives.class
&& !((ColorWithAlternatives)col1).hasAlternativeColors()) {
cl1 = Color.class;
}
Class<?> cl2 = col2.getClass();
if (cl2 == ColorWithAlternatives.class
&& !((ColorWithAlternatives)col2).hasAlternativeColors()) {
cl2 = Color.class;
}
if (cl1 != cl2) {
return false;
}
Even so, subclasses of ColorWithAlternatives may come out
unexpectedly: An object of ColorWithAlternatives with no alternative
colors, and a Color object will be equal. In contrast, an object of a
Subclass of ColorWithAlternatives with no alternative colors, and a
Color object will not be equal. Why is that so?
The part where alternative colors are compared, could well be a method
of ColorWithAlternatives, putting the logic in the right class.
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: [VOTE] Merge XGC color branch into Trunk
Posted by Helder Magalhães <he...@gmail.com>.
Hi everyone,
> If the color branch is now in a state
> that everybody is happy with, I'll restart the vote.
Guess I've missed that one. Wasn't clear (and still isn't) that a new
vote round was triggered but, although I haven't followed the last
changes, the concept and new features seem appealing to me as
previously.
Again, +1 from me and congratulations for the effort, Jeremias! ;-)
Cheers,
Helder
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org