Explicitly add clips and tracks to their respective containers

[sakertooth]

Currently, we add clips and tracks to tracks and track containers in their base classes. As a consequence for adding it to the container before the object has completely finished constructing, the application can crash at selective moments due to the use of an incomplete object (most commonly by usage from the audio thread).

To fix this, callers must now add clips and tracks to their respective containers explicitly after construction has completed.

This is a continuation of #7594, just without being too ambitious to avoid bugs. There is most likely a lot more refactoring that can be done here to make resource management a bit more safe.

[messmerd]

I haven't tested it but it looks good to me

[sakertooth]

I have made some changes, in particular, making addClip, removeClip, addTrack, and removeTrack all virtual. Now, any behavior that depended on the clip or track being added to the parent in the constructor can happen within these functions as extendable behavior on top of the normal functions.

My next goal, which I might not achieve in this PR, is to remove the Track* and TrackContainer* parameters from the constructors to prevent misuse (e.g. passing in a track to the constructor of a Clip, but then adding it to another track by accident). Edit: Requires sweeping changes, will leave for another PR.

[messmerd]

Looks pretty good still - just a few questions

[sakertooth]

I just did the simple solution for now by calling addClip in the constructor where appropriate. Everything else I tried just meant a large refactor which I didnt want to do here.

[allejok96]

Cool I didn't know you could override the return type of createClip like that.

So now Track::createClip is responsible for creating Clip instances and adding them to itself. That makes sense. This comment should change:

lmms/include/Track.h

Line 121 in 2806a31

// -- for usage by Clip only ---------------

Follow up questions (we can deal with these in another PR):

• Should we prevent making a Clip any other way?
• Should we make Track::addClip private?
• Should Track::removeClip use a signal instead of being called from Clip?
• Why should TrackContainer::addTrack be called from Track::create when Track::addClip is called from Track::createClip?

I like the direction you are going with this and code looks good. Haven't tested it though.

[sakertooth]

Sorry about the wait @allejok96. I might need to rework the design. I wanted this to be a simple fix but to do this properly might require a bit more effort.

Should we prevent making a Clip any other way?

I think we should make clips like we are making a simple QObject, using new. Keep that simple structure, and only parent clips to tracks when they were added explicitly to the Track. This probably goes against what I did here a few weeks ago, but I believe this makes more sense.

Should we make Track::addClip private?

No, we should have Track::addClip, Track::removeClip, clipAdded and clipRemoved signals, etc. From a design standpoint this seems more correct. I don't know if I did the correct design currently, so I want to change that. Though I still think you should be able to construct a Clip independently of a Track. That idea should stay.

Should Track::removeClip use a signal instead of being called from Clip?

Yeah, we should have a signal that is emitted when a Clip is removed from a Track. Clip should not call Track::removeClip directly however. When a Clip is being destroyed we can connect it to the clipRemoved signal to remove it from the Track.

Why should TrackContainer::addTrack be called from Track::create when Track::addClip is called from Track::createClip?

Yeah, its confusing. Track::create was there initially, but changing the code to do something else seemed a bit painful. I think there is no other way I can approach this PR without throwing away most of the original design.

I want to make this PR improve our livelihoods now, not just a simple fix I think, so the design would have to change.

I like the direction you are going with this and code looks good. Haven't tested it though.

I am still critical about some parts, but the general idea of being able to create a Clip or Track without needing a Track or TrackContainer as a parent in the constructor is crucial.