Multiple fixes for move commands

[bmatherly]

• Fix moving multiple clips twice can result in misaligned clips
• Fix moving a previously grouped clips regroups the clips

As reported here: https://forum.shotcut.org/t/beta-version-24-01-now-available-to-test/42498/7 https://forum.shotcut.org/t/beta-version-24-01-now-available-to-test/42498/8

Posing this change in a PR for consideration. Maybe this change is too large to risk when a release is eminent. It passes all my test cases. But I am afraid there are cases that I am not considering.

Fundamentally, this changes to things that were causing issues: 1) UUID should be attached to the clip, not the parent producer because multiple clips can share a parent 2) Parameters of moved clips were attached to the parent producer as properties. Again, multiple clips can share a parent. Also, those parameters can be reused by future operations that look for those same properties.

This change saves the UUID on the cut, and it saves the clip parameters in the undo command rather than the producer properties.

[ddennedy]

multiple clips can share a parent

How is that introduced? Shotcut is supposed to always create a unique producer for each clip except when a clip extends into a transition where the transition references the neighbor clips.

[bmatherly]

Shotcut is supposed to always create a unique producer for each clip except when a clip extends into a transition where the transition references the neighbor clips.

That was an incorrect assessment on my part - trying to explain how this fixes the problem. I think that the problem is not related to sharing a parent. I think it is in the retention and possible change to the attached properties that are created in the Move redo function. Consider this scenario:

1) Select clips 1 & 2 2) Move the clips - the position parameters will be set on 1 & 2 3) Select clips 1 & 3 4) Move the clips - the position parameters will be set on 1 & 3 - overwriting the previously set properties on clip 1 5) Undo twice 6) Redo - now the redo operation will try to use the wrong position parameters because step 4 overwrote them

Same for grouping... 1) Group 2 clips 2) Move the two clips - the group parameter will be set on both 3) Ungroup the clips 4) Move the clips again - the previously set group parameter will be reapplied in the redo function - causing the clips to be grouped again

This also brings up another question: should the UUID be set on the cut or the parent? I think it should be set on the cut because findClipByUuid returns the cut. And we need the undo helper to restore the UUID when it restores clips (which it does, but it currently restores the parent UUID, not the cut UUID). But maybe there is another reason to save a UUID on the parent?

[ddennedy]

should the UUID be set on the cut or the parent?

I do not think there is a rule. UUID is simply a way to tag and retrieve any MLT object without holding a reference. It seems for this purpose, cut is better. It is tricky because a "producer" can be a cut or parent producer.

we need the undo helper to restore the UUID when it restores clips (which it does, but it currently restores the parent UUID

I see now that commit 48755c1 for filter undo changed that from the cut to the parent.

[bmatherly]

I do not think there is a rule. UUID is simply a way to tag and retrieve any MLT object without holding a reference. It seems for this purpose, cut is better. It is tricky because a "producer" can be a cut or parent producer.

I notice that playlist commands and filter commands set the UUID on the parent. Maybe that is OK if some commands put UUIDs on the parent and some put UUIDs on the cut - as long as they don't depend on or interfere with each other. In the case of the UndoHelper, sometimes it replaces a previously removed clip. In that case, it is important that it restore the same UUID in the same place because another command might need the UUID to be maintained.

[bmatherly]

Maybe that is OK if some commands put UUIDs on the parent and some put UUIDs on the cut - as long as they don't depend on or interfere with each other.

I found an interaction. Here is a bug that is in current git master:

1) Add a clip to timeline 2) Add a filter 3) Modify the filter 4) Move the clip 5) Undo move the clip 6) Undo modify the filter The UNDO does not work because the move undo dropped the UUID from the clip parent and now the filter undo can not find the clip

It will be difficult to have the filter commands put UUIDs on the cut because we send the clip parent in the callback.