changing KGraphicsUtils?

[Matthew Woehlke]

Michael Pyne wrote:
> On Wednesday 30 May 2007, Matthew Woehlke wrote:
>> 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 :( )

I disagreed from the start, I just didn't say anything about the name 
specifically because I did (and still do) not agree with /anything/ in 
Zack's proposal.

>> 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.

Given what I expect to be in there, I think I would prefer creating a 
second namespace if we get to that point :-). But that's me.

I'm rather shooting blind here, but I expect to have at least:
darken
lighten
{dark,light}Contrast (possibly two flavors of each)
shade
fromHsl
luma
contrast

...and possibly:
variation

(...and naming it KColorUtils allows keeping calls shorter. Like I said 
if we do non-color things I don't see a problem adding another namespace 
:-).)

Also I can't think of anything needed by the current usability request 
that isn't either color-related, or wouldn't belong in KGraphicsUtils 
either.

But it's hard to say, really. How many methods do we want in a namespace?

>> -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.

Zack/Thomas will have to take this one, I didn't look closely at what 
the test is doing.

>> +    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. :)

There is isnan() but I want to say it's less portable than simply 
testing that comparisons are always false :-).

> 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.

I would have thought that was obvious :-) ...but I can doc it (note: I 
don't have access to my code handy ATM, so I'll have to do it tomorrow.)

> 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.

Right. You can look at it as self-documenting, or I'm fine removing it.

-- 
Mathew
(sorry, .sig file is on the other computer)