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

[Parker Coates]

On Tue, Dec 15, 2009 at 08:23, Andreas Pakulat wrote:
> On 14.12.09 20:09:26, Parker Coates wrote:
>> 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.
>
> Well, its your code, but I think it might be better to actually find out
> why the order of things is messed up there. As reverting the
> waitForCards-change at least fixes the games to be playable, maybe this
> can wait until you have time again to look into the code yourself.
>
>> 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.
>
> See above, I've played a round of freecell and spider with the change, but
> am a bit hesitant to put it in as it seems to just hide other bugs in the
> animation code.
>
> If you still think this should go in I'll commit it of course :)

Nah. If everything is working currently, you're right it can wait.

Parker