UndoManager: check reentrancy


#1

Hi Jules,

I found the following method to be useful:

/** Returns this if an undo or redo is not already on the call stack, and nullptr
 otherwise.
 */
UndoManager* getIfNotReentrant()
{
    return reentrancyCheck ? nullptr : this;
}

// caller code:
target.setProperty (property, source[property], undo.getIfNotReentrant())

I have changes propagating from one ValueTree to another, and a case where the caller is not aware, before calling setProperty on the target tree, whether the property change in the source tree was triggered by an undo (or redo).

Is this a silly thing to be doing? If not, would you consider adding that function to the API?


#2

Hmm, seems a bit ropey to me - if I saw some code that used it, I’d be very sceptical about its correctness. Could probably cause some changes to be made without an UndoManager in a way that’d mess up the undo behaviour


#3

As usual you’re totally right. I’m new to the UndoManager, and still getting my head around its design and intended use. I had a ValueTree::setProperty that could trigger a Component::resize(), which in turn was trying to set a property. I just moved that last bit to some controller code where it belonged anyway.

Thanks a ton.


#4

I am running into this problem as well and any opinions would be very welcome.

The run into the reentry assertion check when performing undo with an undomanager that undoes changes to a large valuetree structure. This can easily happen if a valuetree propertychange leads to changing other properties. When undo is performed, the order the changes are undone is not necessarily well defined and therefore sometimes a triggered property change will trigger the reentry assert. Or it’s recorded as a new transaction right away which is also not desirable as it kills redo.

I am adding undo support to my large codebase and I didn’t consider this to be a problem until now. I understand that ideally all changes to the data model would come from the same source, but that’s not just not the case. Instead of rewriting a lot of code I’d like to have a method that tells me the undo manager is currently undoing changes. I know it will for sure undo the secondary changes to the correct values, but as said, maybe in the wrong order. What I now have done is add a flag so I can query whether undo is currently in progress, but why not have this in undomanager? Or maybe have the option to prevent undoable actions alltogether while undo is still in progress. If the state before the undo was valid, there should be no reason to add any new actions during undo (unless some stuff is missing from undo).

I understand this only happens if the code structure is not perfect, but I guess any larger project has areas where code is not as ideal as it could be and for those cases this would be very desirable.

How do others deal with this issue? How do you avoid valuetree changes that trigger other changes?


#5

Just ensure that undoable actions don’t trigger further undoable actions. Generally I try to restrict ValueTree listener methods to only updating the view, as opposed to triggering further operations on the data model. If you must set a property in the treeThatChanged from a ValueTree listener method, don’t use the undo manager. Luckily for me I discovered these things before my tree got very complex. I’m sure it’s a pain, but I’d consider refactoring so that you don’t need to check any flags.


#6

You are right. I did work on another project where things were like that and adding undo support was very easy. However the model I’m working on now is really complex and refactoring out all the dependent changes would lead to much more complicated code overall. Sometimes it’s just one parameter updating another one based on some conditions. If it’s part of one undo transaction, the end result of undo or doing the change again are the same and in those cases it’s just annoying having to refactor. It seems to make ValueTrees a lot less flexible as there are now some “invisible” rules to follow. IMHO most of that would go away if the UndoManager could just be told to not accept new actions during undo transactions. Well maybe I can subclass my own UndoManager for that.