[Kde-games-devel] KPat/Freecell broken?

[Parker Coates]

On Mon, Dec 14, 2009 at 16:19, Andreas Pakulat wrote:
> On 14.12.09 21:07:30, Andreas Pakulat wrote:
>> On 14.12.09 12:50:24, Parker Coates wrote:
>>> Fortunately there's an even cleaner solution. CardDeck now has a
>>> cardAnimationsDone() signal, so as soon as I get a chance, I'll modify
>>> this logic to make use of that instead of manually connecting to the
>>> animationStopped() signal of individual cards.
>>
>> That might actually solve the spider problem. I've tried some debugging
>> and for some (unfortunately unknown reason) the bookkeeping of
>> carddeck.cpp m_cardsWaitedFor gets out of sync with reality. Thats why
>> mousePressEvent then ignores all subsequent mouse events, the
>> m_cardsWaitedFor is not 0 once the run is moved to the bottom-left
>> stack.
>
> I've tried to dig into this more, but I feel like I'm stuck as I don't
> really have an idea how the animation of the cards is supposed to be
> working. It seems this is a bit of a timing issue as with all the debug
> output I now had one case where the right order of things happened. My
> last "non-working" debug output is attached along with the diff for
> that.
>
> The output suggests that there's 2 animations started for one of the
> cards, without a stopAnimation. So the "simplest" fix was to check
> m_animation inside Card::animate() and call stopAnimation if its != 0:
>
> Index: card.cpp
> ===================================================================
> --- card.cpp    (revision 1062335)
> +++ card.cpp    (working copy)
> @@ -218,6 +218,9 @@
>
>  void Card::animate( QPointF pos2, qreal z2, qreal scale2, qreal
> rotation2, bool faceup2, bool raised, int duration )
>  {
> +    if( m_animation ) {
> +        stopAnimation();
> +    }
>     if ( duration <= 0 )
>     {
>         setPos( pos2 );
>
> I have no idea wether that change is correct, or wether one should try
> to find why things aren't done in the "proper" order. But I'm not able
> to do the latter anyway.

It definitely looks like the right thing to do. At first I was shocked
that it isn't already there. Then after poking around for a bit, I
remembered why it wasn't. Calling stopAnimation before starting an
animation causes a weird looking jump to happen during the
right-click-to-expose-an-obscured-card animation. In retrospect,
however, I think that's a small price to pay for giving Card a more
sane API.

I'll have to think about whether there's a better way to keep that
animation smooth. I guess I could always add another bool argument to
Card::animate(), called forceAnimationToCompleteWhenStoppingIt, but I
think we can agree that that function has enough arguments already. :)

Not only was this breaking animation tracking (in some as of yet
undiscovered corner case), but it also means that Q*Animations were
leaking. Did you get a chance to test this patch? Please commit if it
doesn't break anything for you. You can actually remove the
m_animation check, since stopAnimation() already does this internally.
If there is no current animation, then stopAnimation() is essentially
a no-op.

I see now how this problem slipped past me: I was missing an assert.
CardDeck::cardStartedAnimation() should have contained "Q_ASSERT(
!m_cardsWaitedFor.contains( card ) );". When I get a chance, I'll add
that, which should flush out any problems in a hurry.

Thanks a lot, Andreas (and Julian). I've learned a valuable lesson
from all this: Don't commit anything in the days leading up to a long
absence.

Parker