changing KGraphicsUtils?

[Michael Pyne]

On Wednesday 30 May 2007, Matthew Woehlke wrote:
> A number of people have expressed confusion/dissatisfaction over the
> implementation of blendColors that Thomas committed. In an attempt to
> address these comments, I would like to suggest supporting two methods
> for the moment; Zack's, and mine. To make it very clear which does what,
> I have adopted Hans Meine's suggestion of making the names mix() and
> overlayColors() (feel free to argue for mixColors() and/or overlay(),
> and/or paint[Colors]() as an alternative to overlay[Colors]()).

The names seem fine to me, I suspect the API documentation will be more 
important for understanding. :)

> mix() does a basic linear blend, treating the alpha channel as 'just
> another channel', which is the correct behavior if one is calculating
> bands in a gradient. At least Ingo Klöcker also suggested this behavior.
> This signature is an exact match for what is in KDevelop 3.4 (which
> originated in Ion's predecessor) and can immediately replace
> alphaBlendColors in Plastik with only very minor effort (although it has
> been questioned whether Plastik will ultimately need this method or not).

I think it's important to note that in the case of styles we definitely want 
alpha-blending.  I believe Plastik used methods like that to simulate real 
alpha in situations where it could control both the background and foreground 
painting.  For example, rounded buttons can look weird if the transparent 
corners do not get drawn with the parent widget's background.

But even though we can do alpha that doesn't mean we won't need color 
blending.

> Also, about the name... I don't like it. :-)

> (And IIRC there was exactly 
> zero discussion about it.)

Usually no discussion means that there is no disagreement (unless you mean 
there was no discussion before it was committed, I've been too busy to read 
every thread message :( )

> Since I expect this namespace to ultimately 
> hold a good chunk of the usability stuff, what about KColorUtils
> instead, since we are working with colors and not* images?

We are not working with images yet, but since the end result for most of this 
work appears to be usability-related, I don't think it's too far-fetched that 
the namespace may expand to include other types of things related to graphics 
that don't limit themselves precisely to colors.  I think it's best to leave 
it as KGraphicsUtils (or similar) because otherwise we artifically limit the 
namespace to dealing with colors only, and who knows what we may have in 
there by KDE 4.4.

> Patch attached. Please comment on any clean-up (naming, formatting, etc)
> needed as well. This also fixes some existing consistency problems (e.g.
> TestBlendColors not matching the naming scheme of other kdeui tests, the
> error in the blendColors example code). I would like to commit Monday if
> there are no (unresolved) issues at that time.

> -void TestBlendColors::test() {
> +void tst_KColorUtils::testOverlay()
> +{
>      QColor color1(10, 10, 100);
>      QColor color2(10, 10, 160);
> -    QColor blended = KGraphicsUtils::blendColor(color1, color2);
> +    QColor blended = KColorUtils::overlayColors(color1, color2);
>      QCOMPARE(blended, color2); // no transparacny.

transparancy should be transparency.  Best to fix it now while the code is 
still being actively worked.

> +    if (bias <= 0.0) return c1;
> +    if (bias >= 1.0) return c2;
> +    if (!(bias < 1.0)) return c1; // bias == NaN

I realize you've got it documented but is there no better (more obvious) way 
to test if a floating number is NaN?  I believe your method is correct (and 
clever) but it took me at least 30 seconds to prove that to myself. :)

Speaking of NaN, in the API documentation for the function you could probably 
mention that NaN results in returning c1 (or better yet, an undefined color) 
since you're testing for it anyways.

Also you use the register keyword in mix().  I'm no compiler guru but I'm 
pretty sure that the keyword isn't useful as gcc should automatically leave 
the result in a register for QColor::fromRgbF() if there's a register 
available.  The keyword doesn't hurt things of course, it just seems 
extraneous.

Other than that I think the patch is fine (and what I've pointed out is just 
small stuff).  I agree with adding the function as it's what I think I'd be 
using more often of the two.

Regards,
 - Michael Pyne
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070530/d44edaa9/attachment.sig>