Fix lost connections when restoring automation clip

[szeli1]

this fixes #6829

Now when undeleting an automation clip, it's connections shouldn't be lost.

[zonkmachine]

Tested, fix confirmed!

[DomClark]

Do projects with a lot of automation still load correctly after this change? It looks like this may resolve IDs too early, so automation clips connected to objects which have not yet been loaded may never be reconnected.

[zonkmachine]

Do projects with a lot of automation still load correctly after this change? It looks like this may resolve IDs too early, so automation clips connected to objects which have not yet been loaded may never be reconnected.

I've tested the bigger projects like Greippi - Krem Kaakkuja, and EsoXLB - CPU They seem to open correctly.

[szeli1]

Do projects with a lot of automation still load correctly after this change? It looks like this may resolve IDs too early, so automation clips connected to objects which have not yet been loaded may never be reconnected.

resolveAllIDs gets run for every single restored clip so this shouldn't be a problem, but there is a chance it gets called unnecessarily

[szeli1]

This should make resolveAllIDs run only once for 1 automation track loading (not a perfect solution).
Song.cpp line 1192 is now unnecessarily, but I left it in.

[DomClark]

Here is an example project that doesn't load properly with this change. It contains an automation clip that is connected to a knob in a later track. When IDs are resolved after loading the automation track, the target knob doesn't yet exist, so the automation isn't reconnected.

AutomationClip::resolveAllIDs should run only once everything has been loaded. Perhaps consider putting it at the end of ProjectJournal::undo and ProjectJournal::redo?

[szeli1]

AutomationClip::resolveAllIDs should run only once everything has been loaded. Perhaps consider putting it at the end of ProjectJournal::undo and ProjectJournal::redo?

I thought about that and to me It looked out of place. In ProjectJournal how would I check if it is necessary to run AutomationClip::resolveAllIDs?

[szeli1]

AutomationClip::resolveAllIDs should run only once everything has been loaded. Perhaps consider putting it at the end of ProjectJournal::undo and ProjectJournal::redo?

I think I managed to move the AutomationClip::resolveAllIDs into ProjectJournal::undo. (I had issues with git, hopefully it did not cause issues here)

[DomClark]

I thought about that and to me It looked out of place.

Automation and undo history are some of the uglier bits of LMMS, so where the two intersect, you'll unfortunately have nasty things like this. We want to improve them, but it's a lot of work, there are lots of other things to do, and there's only so much contributor time to go around.

In ProjectJournal how would I check if it is necessary to run AutomationClip::resolveAllIDs?

I'm not sure whether it's worth checking at all, but if you do want to check, have a look at QDomDocument::elementsByTagName, which should provide a concise way to check for the existence of any automationclip tags.

[szeli1]

have a look at QDomDocument::elementsByTagName, which should provide a concise way to check for the existence of any automationclip tags.

implemented, tested it with your example project (and others), should I re request a review?

[zonkmachine]

This PR has conflicts with master.

[szeli1]

I can not see conflicts. I will mark this as resolved (if you agree).

[zonkmachine]

I mean this here:

If you check the 'Resolve conflicts' button it will show no conflicts but merging into master on the command line there really is unresolved stuff in src/core/Track.cpp. Odd. If you're happy with it as it is, though, then I'm happy.

[zonkmachine]

If you're happy with it as it is, though, then I'm happy.

The PR can't actually be merged as it is though. The merge button is greyed out on my side.

[szeli1]

I will resolve the 0 conflicts. I don't know why git is confused, I eventually deleted all the changes in Track.h in this pr.

[zonkmachine]

Git is happy now.

[Rossmaxx]

From the comments, looks like it was supposed to be merged last month.

[zonkmachine]

From the comments, looks like it was supposed to be merged last month.

No. It's awaiting a re-review from Dom.