[Kde-games-devel] KPat Progress and Regressions

[Parker Coates]

On Fri, Nov 13, 2009 at 15:40, Aaron J. Seigo wrote:
> On November 13, 2009, you wrote:
>> > This would be nice to have. Patches are very welcome. :)
>>
>> i'm quited tempted; maybe for kde 4.5 (and we can probably steal code from
>> libplasma for this as well since we do this kind of transition a lot in
>> plasma)
>
> meh; i got annoyed with something else i was working on and spent the 5
> minutes it took to implement this. draft patch attached.
>
> similar could be done for the card deck "pads" on card-drag hover/leave.

The patch looks good. I have a few comments though.

- The code style doesn't match the rest of the file. Curly braces
should be on their own lines. Parentheses should be padded with
spaces. Nothing major.

- The 50 ms and the 0.2 increment are repeated in several place. Could
we instead define these as constants calculated at compile time from
the desired animation duration and the number of frames?

- You wrap some floating point literals in "qreal()", but not others.
Is there a reason for this, or is it just an over site? Although I
replaced all instances of "float" and "double" with "qreal" in the
code base, none of the literals are currently cast. Is there a
document outlining the best practices with respect to qreal usage? I'm
not sure when it is necessary and when it isn't.

Take this line for example:

    if ( m_highlightFadeAmount > qreal(1.0) || m_highlightFadeAmount < 0) {

Why use a double-literal-cast-to-qreal for one comparision and an
integer for the other.

Or this line:

    m_highlightFadeAmount = qBound( qreal(0.0), m_highlightFadeAmount,
qreal(1.0) );

Does this produce different/better/worse code than "qBound<qreal>(
0.0, m_highlightFadeAmount, 1.0 );" or "qBound<qreal>( 0,
m_highlightFadeAmount, 1 );"

Anyway, that's kind of off topic. Barring the above, the patch looks
great. Feel free to commit when you're happy with it.

Parker