Rename a state: update animation keyframe references and clear stale error (#3383)#3393
Closed
vchelaru wants to merge 2 commits into
Closed
Rename a state: update animation keyframe references and clear stale error (#3383)#3393vchelaru wants to merge 2 commits into
vchelaru wants to merge 2 commits into
Conversation
…error (#3383) Renaming a state left animation keyframes pointing at the old name, producing a lingering 'references a state X which does not exist' error. Three root causes, all addressed: - MainStateAnimationPlugin.HandleStateRename refreshed the view model (recomputing/broadcasting errors against the still-old keyframe names) BEFORE rewriting the keyframes, and never recomputed afterward. Reordered: rewrite keyframes first, then RefreshViewModel recomputes errors and rebroadcasts. The post-rename name is seeded into AvailableStates before the rewrite so the keyframe ComboBox's two-way SelectedItem binding does not null the (now-absent) old selection. - RenameManager.HandleRename(StateSave) did not persist the .ganx (unlike the ElementSave overload), so even a corrected reference could be lost on reload. It now saves when a keyframe was rewritten. - RenameManager.HandleRename(StateSave) built the category prefix via StateSaveCategory.ToString() rather than .Name; switched to .Name to match the rest of the keyframe name handling. Tests: added RenameManager persistence coverage (saves on a referenced-state rename, does not save when nothing matches); promoted the categorized-state rename test to the #3383 ordering regression guard and removed the now-fixed stale-error characterization test. The WPF ComboBox-binding half is verified manually. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removed the explicit Save() added to RenameManager.HandleRename(StateSave). Assigning keyframe.StateName already raises PropertyChanged, which the plugin persists via its centralized HandleDataChange (AnyChange) save — the same single path the sibling InstanceSave overload relies on. A second explicit save wrote the .ganx twice per rename, which can trip external tools' file watchers (and Gum's own IFileWatchManager) on a change that already settled. Documented why the ElementSave overload still saves explicitly (element-file rename changes no keyframe property, so nothing triggers the implicit save there). Reverted the two RenameManager persistence tests that asserted the now-removed save. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Closing as redundant. #3383 was already fixed at the root by #3389 (merged onto the same base this branch was cut from, mid-session): the keyframe state ComboBox was made IsEditable with a Text binding, eliminating the StateName null-coercion entirely, plus the HandleStateRename ordering fix. This PR re-introduced the AvailableStates-seeding workaround that #3389 deliberately reverted, so merging it would regress that design. |
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
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.
Summary
Renaming a state did not reliably update animations that referenced it: the keyframe kept pointing at the old name, leaving a lingering
Keyframe at time … references a state X which does not existerror (surfaced on the tree node and Errors tab after #3293/#3382). Fixes #3383.The pre-fix symptom was deceptive: the error vanished the instant you renamed (the WPF binding nulled
StateName, which trips neither error branch), so it looked fixed — but the keyframe was never actually rewritten, and re-selecting/reloading the element rebuilt the original reference and the error returned.Root causes addressed
MainStateAnimationPlugin.HandleStateRenamecalledRefreshViewModel()(which recomputes/broadcasts errors against the still-old keyframe names) beforeRenameManagerrewrote the keyframes, and never recomputed afterward. Reordered: rewrite first, thenRefreshViewModelrecomputes + rebroadcasts.AvailableStatesremoved the old categorized name, so the keyframe ComboBox's two-waySelectedItem→StateNamebinding clearedStateNameto null before the rewrite could match it. The post-rename name is now seeded intoAvailableStatesbefore the rewrite, so the binding accepts the rewritten value instead of nulling it.ToString()— the category prefix was built fromStateSaveCategory.ToString()instead of.Name; switched to.Nameto match the rest of the keyframe-name handling.Persistence — single save path (no double write)
RenameManager.HandleRename(StateSave)rewrites the keyframe and does not save. Assigningkeyframe.StateNameraisesPropertyChanged, which the plugin already persists through its centralizedHandleDataChange(AnyChange) save — the same single path the siblingInstanceSaveoverload relies on. An explicit save here would write the.ganxtwice per rename and trip external (and Gum's own) file watchers on a change that already settled. TheElementSaveoverload still saves explicitly because renaming the element file changes no keyframe property, so nothing triggers the implicit save there.Tests
AnimationStateRenameErrorTests: promotedHandleRename_OfCategorizedState_RewritesKeyframeAndClearsErrorOnRefreshto the Renaming a state doesn't update animation keyframe references (leaves a stale missing-state error) #3383 ordering regression guard (rewrite-then-refresh leaves no stale error) and removed the now-fixedRenameSequence_AsThePluginRunsIt_LeavesStaleErrorcharacterization.RenameManagerTests: the existingHandleRename_State_RenamesMatchingKeyframeStateName/…StateCategory…already pin the rewrite.All 18
StateAnimationPlugintests green;GumFull.slnbuilds with 0 errors.Manual verification
The WPF ComboBox-binding half can't be unit-tested (needs a WPF host), so it's verified in the running tool: add a state (in a category), create an animation keyframe referencing it, rename the state → no missing-state error appears, the keyframe shows the new name, and it persists across reselect/reload.
Closes #3383
🤖 Generated with Claude Code