blend function urgently needed in kdelibs

[Thiago Macieira]

Matthew Woehlke wrote:
>> Comments:
>> 1) You're mixing the enums. ColorSpec is declared in a flag-like
>> manner, but it has no Q_DECLARE_FLAGS; BlendMode or BlendFlags (choose
>> one) is declared in a non-flag mode but has Q_DECLARE_FLAGS. You want
>> to review that.
>
>Thanks, but that's actually intentional, it should not be used as flags
>except by KColorPrivate (although Matt Nowell is doing his darndest to
>talk me out of that :-)). I guess making it 'not flags-like' would mean
>'making the values 1, 2, 3, ...'? (I want to confirm because I may end
>up going that way...)

That's what I meant. If you need to use the type in a flags-like manner 
internally, you can use (1<<type) wherever needed. But this is minor.

As for BlendMode, you used Q_DECLARE_FLAGS, but the enum values do not 
support operation in flags mode. Besides, it doesn't make semantic sense: 
what would be a Replace, Multiply *and* Add blend?

>> 2) Please remove the protected function and the static inlines. They
>> have no business being in a public header file.
>
>Then where /do/ protected functions belong? (What if someone wanted to
>subclass KColor?)

Why would anyone want to subclass KColor? What information/manipulators 
would you want to provide sub-classers that shouldn't be available to 
normal users?

If you have a valid case for that above, then it makes sense to have a 
protected function. Otherwise, simply remove them.

In this case, I see no need for a protected "blend" function that doesn't 
even seem to touch the private members.

Rule of thumb: if you have no immediate need or use-case for a function, 
don't publish it. If the need you thought of actually shows up later, you 
can add it at that time. Exception: virtual functions.

>I suspected someone would complain about the static inlines, the thing
>is I would like them to be available... are they OK non-inline? (It's
>not like they're going to change, though...)

No, they shouldn't be there at all. Not these, at least. Why do you want 
to provide them? Who would use them?

If you want to make those functions present for your own code to be 
readable, make them static non-members in your .cpp file.

>> 3) Constructors must be explicit or explicitly implicit. (i.e., add
>> the krazy mark if it's to be implicit)
>
>I must admit I'm not entirely familiar with what the difference is, how
>do I know which should be which? Is there a doc somewhere?

The two constructors:
    KColor( const QColor& );
    KColor( const KColor& );
are different in meaning. The latter one is a copy-constructor and it 
generally makes no sense to make it explicit. So you can leave it as it 
is.

The QColor overload, however, is the question. The Library Code Policy 
(http://techbase.kde.org/Policies/Library_Code_Policy) says:

"For each constructor (other than the copy constructor), check if you 
should make the constructor explicit in order to minimize wrong use of 
the constructor. 
Basically, each constructor that may take only one argument should be 
marked explicit unless the whole point of the constructor is to allow 
implicit casting."

In this case, I would say that the whole point is to allow implicit 
casting, so you want to tell krazy that you know it is implicit. Just add 
an " // implicit" after it to tell krazy to go away.

>> 4) If you want implicit sharing, use QSharedDataPointer.
>
>See conversation with Matt Nowell, this is still in the air.

I have received no emails from Matt Nowell.

-- 
  Thiago Macieira  -  thiago (AT) macieira.info - thiago (AT) kde.org
    PGP/GPG: 0x6EF45358; fingerprint:
    E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070522/3c99b29b/attachment.sig>