Plug : Fix bad interaction between addChild() and reorderChildren()#7014
Merged
Merged
Conversation
johnhaddon
force-pushed
the
catalogueDuplicateFix
branch
from
146ede6 to
fd3926b
Compare
`Plug::parentChanging()` and `Plug::childrenReordered()` both need to do work to mirror the changes onto their output plugs, so that changes to promoted plugs are reflected on the internal plugs. Likewise, they both need to avoid doubling-up this work during undo/redo, because by default the undo system will have recorded the mirroring and it won't need doing a second time. `parentChanging()` avoided the doubling by checking `ScriptNode::actionStage()`, which is what we do generally in other spots. But `childrenReordered()` was taking a different approach : using `UndoScope( Disabled )` to prevent the mirroring being recorded in the undo queue, and just doing the work itself every time. I actually think I prefer this latter approach, because it avoids unnecessary bloat in the undo queue. But it was the cause of a bug when adding a child and reordering in the same UndoScope. The issue was that when `UndoScope( Disabled )` closes, it calls `ScriptNode::popUndoScope()`. When we're in `undo()`, that causes ScriptNode to set `m_actionStage` to `Invalid`, which in turn means that `parentChanging()` will double up on the work. Which leads to _two_ attempts to remove a child, with the second one failing because it has already been removed. Longer term I think it's worth investigating making `UndoScope( Disabled )` work better for this situation, and moving more code to use it. Or possibly even doing it all magically, by having ScriptNode not record actions that occur while an outer action is running. But for the short term the safest fix is to bring `childrenReordered()` into line with all our other methods that modify the graph in response to signals.
murraystevenson
force-pushed
the
catalogueDuplicateFix
branch
from
fd3926b to
eaa07b0
Compare
murraystevenson
approved these changes
Jul 9, 2026
murraystevenson
left a comment
murraystevenson
left a comment
Contributor
There was a problem hiding this comment.
Thanks John. LGTM. I've rebased this to fix the Changes.md merge conflict.
github-project-automation
Bot
moved this from Pending Review
to Pending release
in Work in Progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Plug::parentChanging()andPlug::childrenReordered()both need to do work to mirror the changes onto their output plugs, so that changes to promoted plugs are reflected on the internal plugs. Likewise, they both need to avoid doubling-up this work during undo/redo, because by default the undo system will have recorded the mirroring and it won't need doing a second time.parentChanging()avoided the doubling by checkingScriptNode::actionStage(), which is what we do generally in other spots. ButchildrenReordered()was taking a different approach : usingUndoScope( Disabled )to prevent the mirroring being recorded in the undo queue, and just doing the work itself every time. I actually think I prefer this latter approach, because it avoids unnecessary bloat in the undo queue. But it was the cause of a bug when adding a child and reordering in the same UndoScope.The issue was that when
UndoScope( Disabled )closes, it callsScriptNode::popUndoScope(). When we're inundo(), that causes ScriptNode to setm_actionStagetoInvalid, which in turn means thatparentChanging()will double up on the work. Which leads to two attempts to remove a child, with the second one failing because it has already been removed.Longer term I think it's worth investigating making
UndoScope( Disabled )work better for this situation, and moving more code to use it. Or possibly even doing it all magically, by having ScriptNode not record actions that occur while an outer action is running. But for the short term the safest fix is to bringchildrenReordered()into line with all our other methods that modify the graph in response to signals.