Add undo/redo for filters

[ddennedy]

I think this is working quite good now. Even though I could test more, I want to give you a chance to review it before merging. One thing missing I know is that pasting filters is not tracked. I will look into that, but the way it works currently is not very in line with the model and controller. Even though changes in keyframes factor into an undo command, this does not provide discreet undo commands for keyframe actions. It also does not provide discreet undo commands for each separate parameter. Rather, all of these simply apply to a single filter-changed command. It may be possible to extend it track changes to each parameter distinctly. I have worked on this off and on so long, I do not quite recall whether I had trouble with that.

In any case, one concern I have about adding more granularity is a concept I am calling "history bloat." Imagine all of the little changes that occur within keyframes or adjusting differing parameters within a filter adjustment session (editing a single filter on a single clip or track) including back-and-forth between parameters. Now, switch context to editing even a moderately complex composition on the timeline and trying to undo its history back to only a couple or few edits ago. If you are not watching the History panel closely, you might be needing to undo quite a lot if you do a lot of filter and key framing between edits. You might even think that undo is broken or at least find it very burdensome to find the previous point from a list of vague commands. Most people observe a visual change upon undo and fallback on History as an additional guide. In addition, the further one goes back in history, the less confidence they have and the greater the feeling of loss of work. Meanwhile, this new functionality needs to be solid, and trying to extend it should be an additional effort once this proves stable. Otherwise, imagine someone wants to undo to a couple of timeline edit decisions, they jump back, and some intervening filter commands completely broke some filters on some clips. yswim?

[bmatherly]

A couple of comments for your consideration. I do not know how hard these would be to implement:

1. In the undo history, it would be helpful if the history text would indicate which producer the change is applied to. For example, instead of "Add Brightness filter", it could say "Add Brightness filter to track 2". Also, for example, "Change Brightness filter on track 2".

2. When undoing a change, it would be nice if the UI would switch to display the filter panel for the filter that is being changed. That would require selecting the clip, track or Output that the filter belongs to.

[ddennedy]

it would be helpful if the history text would indicate which producer the change is applied to

That is possible except A) we are not doing that anywhere else and B) a producer's name can be changed and that is not reflected in History today. So, this would have to be a separate change that possibly adds it in other areas and that adds a command for rename.

it would be nice if the UI would switch to display the filter panel for the filter that is being changed. That would require selecting the clip, track or Output that the filter belongs to.

I do not think undo or redo of these should change selection. I intentionally avoided it, and that was a big part of the changes: to have the model not rely on its current producer. We generally do not track selection with the other playlist and timeline commands either. If you jump back in history many steps there could be a lot re-selection happening, and there is a cascade of signals upon selection. So then we would have to try to ensure to only select when the last command is on a filter command, and only select its producer. That requires 2 things we do not currently have: A) a way to emit a signal on only the new undo stack index and B) a reverse map from producer UUID to some indices such as model index for playlist or trackIndex,clipIndex for timeline.

[bmatherly]

Everything is working good for me and I do not experience any crashes.

it would be helpful if the history text would indicate which producer the change is applied to

That is possible except A) we are not doing that anywhere else and B) a producer's name can be changed and that is not reflected in History today.

I also ran into this with markers. I chose to put the marker name in the undo command text. But a user can change a marker name which will make the undo command text wrong in the history. I do not feel strongly about the comment. I just thought it might give the user a better clue about what they are about to "undo".

I do not think undo or redo of these should change selection.

I agree. The screen might be changing panels and selection in an unexpected way. I was just thinking about how undo works in a wordprocessor where it will move the cursor to where the text is being undone so the user can see what object is being modified.

I still think we should consider putting a limit on the undo history and discarding undo commands older than X commands.

There is one more quirk that I notice. I do not know if this should be addressed:

1. Start with an empty project
2. Open a clip
3. Add filter to the clip and make a change to it
4. Drag the clip to the timeline

At this point, the history is:

• Add filter
• Change filter
• Add video track
• Overwrite onto track

5. Hit undo twice

Now you are left with change and add filter commands in the history, but there are no clips.

6. Hit undo twice again

There is no crash, but also the undo commands do not do anything.

[ddennedy]

I still think we should consider putting a limit on the undo history

I agree and will do it. How about 100?

There is one more quirk that I notice.

I think we should not add to undo history filter operations on Source. We do not add to undo history for other operations in Source such as trimming and changing properties like we do in timeline.

[bmatherly]

I agree and will do it. How about 100?

I was thinking 10. Meet you in the middle at 50?

I think we should not add to undo history filter operations on Source.

Agreed.

I did not do any testing on playlist clips, but I think that should be OK to use undo.

[ddennedy]

All comments addressed except making undo descriptions more verbose. I want to leave that to a future version after adding a rename command so people can see the old and new names. Then, we can do more comprehensively. I did find some more problems when intermingling playlist commands with filter commands, and I will look into that further before merging.

[ddennedy]

The interaction of filter commands with playlist commands has been addressed but with basic testing I still see some weirdness and difficult crashes. So, I am inclined not to include this in the 21.12 release. I want to focus a bit more on stability for the next release and not introduce something possibly unstable like this. We have the avfilter change that is big enough to worry about.

[luzpaz]

Any progress here ?

[ddennedy]

@luzpaz stop it