QSequentialAnimationGroup and destruction of component Animations

vikramg

I struggled for several hours to trace the source of a
*** Error in `MyApp': free(): invalid pointer: 0x6e802e6c ***
segfault I was running into at the exit of my app. I had taken care to use modern RAII idioms, so there should not have been any naked new/deletes. I finally traced the problem to the following:

I have in my code:

class MyClass
{
    . . . 
private:
    QVariantAnimation A, B;
    QSequentialAnimationGroup AB;
};

In the constructor I set up the animations A and B, and add them to the SeqAnimGroup:

AB.addAnimation(&A);
AB.addAnimation(&B);

I had assumed that the default destructor of MyClass could destroy all component animations. It turned out that the segfault was on the A object. Evidently it was being deleted twice. It was then that I read in the documentation, to my astonishment, that the QSeqAnimGroup destructor also destroys its component animations!

Why is this!?? QSeqAnimGroup ought to destroy its components only if it has assumed ownership of them! We are not std::moveing the component animations into the SeqAnimGroup (nor am I sure if C++ class member objects can be moved at all). The SeqAnimGroup only takes pointers to its components. In every case where one might imagine this would be used, the component animations will have been created somewhere else, and will be naturally destroyed when they go out of scope. It seems wholly unnecessary for the SeqAnimGroup to destroy them in its own destructor.

The solution is to call removeAnimation() on the SeqAnimGroup before it gets destroyed, but this should at least have been stated in bold red in the documentation. Am I missing something here? Are there other similar cases elsewhere in the libraries to be aware of?

Christian Ehrlicher

I would say - RTM

Adds animation to this group. This will call insertAnimation with index equals to animationCount().
Note: The group takes ownership of the animation.

vikramg

@Christian-Ehrlicher Ah, thanks for pointing it out, I hadn't noticed that either. I agree, RTM is the final word and I'll mark this solved, but I'm still grumbling about the design philosophy behind such a choice. :)