Fix update of Undo history

[SuslikV]

This change prevents modification of the 'load' type action in the history by the wrong data swap. That itself later may cause null objects to be written during history usage.

The issue happens if you:

1. load saved project
2. clear the history
3. select the clip and click any property of it
4. try to Undo the action

The application hang happens if you:

1. select clip and modify any property of it
2. save the project
3. load project saved in p1
4. select the clip and click any property of it (do not modify, just single click)
5. try to Undo the action

Edit: designed to work with the Curve Editor: #3084

[ferdnyc]

I'm trying to follow the pathology here, because it seems like this should be fixable in the update manager itself.

When apply_last_action_to_history is called, it checks for a self.last_action first:

openshot-qt/src/classes/updates.py

Lines 341 to 345 in 5e90b39

	def apply_last_action_to_history(self, previous_value):
	""" Apply the last action to the history """
	if self.last_action:
	self.last_action.set_old_values(previous_value)
	self.actionHistory.append(self.last_action)

So if that call's unsafe when value_updated() hasn't been called, then something previously must NOT be resetting self.last_action back to None. Whenever self.last_action is consumed in the update manager, it should also be CLEARED so that repeat actions can't be committed to the history. Only when value_updated has been called, should self.last_action be not None.

But that's the problem. After __init__(), nothing ever sets self.last_action back to None.

If we fix that, then it should be safe to call apply_last_action_to_history whether or not value_updated has been called, which seems to be the intent of the code. Nothing will get pushed on to the history stack, and then the if len(self.actionHistory) > 0 test here in Undo will fail, also as intended:

openshot-qt/src/classes/updates.py

Lines 254 to 264 in 5e90b39

	def undo(self):
	""" Undo the last UpdateAction (and notify all listeners and watchers) """
	if len(self.actionHistory) > 0:
	# Get last action from history (remove)
	last_action = copy.deepcopy(self.actionHistory.pop())
	self.redoHistory.append(last_action)
	# Get reverse of last action and perform it
	reverse_action = self.get_reverse_action(last_action)
	self.dispatch_action(reverse_action)

We could fix this in the Properties table view, but I bet that's not the only place this bug can be triggered, and the real bug seems to be in the update manager's handling of its own self.last_action. Fixing it there will fix it for all of the code.

[ferdnyc]

Urgh. Except, we can't do that, because all over the update manager there's code like this:

openshot-qt/src/classes/updates.py

Lines 332 to 339 in 5e90b39

	def delete(self, key):
	""" Delete an item from the UpdateManager with an UpdateAction (this action will then be distributed to all listeners) """
	self.last_action = UpdateAction('delete', key)
	if not self.ignore_history:
	self.redoHistory.clear()
	self.actionHistory.append(self.last_action)
	self.dispatch_action(self.last_action)

If self.last_action is dispatched after being pushed onto the history stack, then it can't be cleared when it gets pushed. ...In fact, the way the code's written, it can't ever be cleared.

So, I think we need a new variable inside the update manager, instead. Rather than apply_last_action_to_history actually using self.last_action, the call should check a new self.pending_action, which is only set when ignore_history is enabled, and it's cleared after use. Because, not every self.last_action is eligible for insertion onto the history stack, the way the current apply_last_action_to_history code seems to assume. It's only if it's an action that DOESN'T already exist in the history, that it should be pushed.

And then the code would be (using delete as an example, though all of the functions would have to be changed):

    def delete(self, key):
        """ Delete an item from the UpdateManager with an UpdateAction
        (this action will then be distributed to all listeners) """

        self.last_action = UpdateAction('delete', key)
        if self.ignore_history:
            self.pending_action = self.last_action
        else:
            self.redoHistory.clear()
            self.actionHistory.append(self.last_action)
        self.dispatch_action(self.last_action)

    def apply_last_action_to_history(self, previous_value):
        """ Apply the last action to the history """
        if self.pending_action:
            self.pending_action.set_old_values(previous_value)
            self.actionHistory.append(self.pending_action)
            self.pending_action = None

At least, that's the way I read it. @jonoomph ?

[ferdnyc]

Whoops, one correction: Pushing to the stack should also clear self.pending_action. So:

    def delete(self, key):
        """ Delete an item from the UpdateManager with an UpdateAction
        (this action will then be distributed to all listeners) """

        self.last_action = UpdateAction('delete', key)
        if self.ignore_history:
            self.pending_action = self.last_action
        else:
            self.redoHistory.clear()
            self.pending_action = None
            self.actionHistory.append(self.last_action)
        self.dispatch_action(self.last_action)

[SuslikV]

It is funny thing that you using your own api in the wrong way. And instead of showing how it should be used right, you fixing the api itself...

[ferdnyc]

The issue happens if you:

1. load saved project
2. clear the history
3. select the clip and click any property of it
4. try to Undo the action

On this one there's actually a bigger issue, I've discovered: After you do Edit > Clear history, Undo is still enabled. It shouldn't be — there's nothing to Undo anymore! And it shouldn't re-enable until something gets pushed onto the history stack.

My pending_action change will prevent the destruction of the entire Timeline when a property is (not) modified after an Undo (PR incoming), but it doesn't fix the incorrect Undo state. (In fact, it makes things worse somehow — if you click Undo after selecting-but-not-changing a Property, now instead of the Timeline disappearing, Undo will disable, but Redo enable — implying there's still some action being pushed/popped among the history stacks, when they should've just been cleared.)

So now I have to go figure out what's wrong with the "Clear history" trigger...

[ferdnyc]

Aha. Simple really. Both reset() (which "Clear history" uses) and apply_last_action_to_history() needed to call update_watchers() to correctly disseminate the new state of the history stack.

[SuslikV]

Wow! You didn't read the current PRs list... I'm posting to space.

[SuslikV]

Right now, this PR doesn't breaks logic of the mentioned #3187 , so it will be here until something will be merged. By fixing the api - this program fix can be omitted.

[ferdnyc]

Wow! You didn't read the current PRs list... I'm posting to space.

I actually had read the list, it's just that "Fix main menu actions state (UI)" didn't immediately strike me as being an undo fix. 😉 (I hadn't yet read the contents of every pending PR.)

[SuslikV]

Let's speed up other solutions!

[ferdnyc]

@SuslikV Actually, if you could reop... oh, you deleted the branch. Never mind.

I was going to ask you to reopen, so I could merge this version. I'm reluctant to merge my own code without review, especially when it makes changes to central functionality like the update manager, so it might take a second or two for #3187 to make it in. But this fix I can merge, as it's pretty self-contained and we've both tested it.

Not a problem, I'll split the commit out of #3187 into its own PR and merge. Thanks!

[SuslikV]

@ferdnyc see, no longer needed as the #3187 merged. ^_^

[ferdnyc]

@SuslikV Well, yeah, but I'd already merged #3194 (my PR making the same undo/redo state fix as your #3180 — the PR I was actually thinking of, not this one) five weeks ago! 😆