Fix event annotation lifecycle and persistence after the sexp_tree refactor#7595
Merged
Goober5000 merged 3 commits intoJul 11, 2026
Merged
Conversation
Goober5000
added
fix
wookieejedi
approved these changes
Jul 11, 2026
github-project-automation
Bot
moved this from Work In Progress (PRs)
to In Review (PRs)
in qtFRED2
Goober5000
force-pushed
the
fix/annotations_post_refactor
branch
2 times, most recently
from
July 11, 2026 03:48
866c760 to
e1826de
Compare
Post-refactor successor of fix/annotations, rebuilt for the shared sexp tree model introduced by the sexp_tree refactor (scp-fs2open#7301). The refactor re-keyed annotations from UI item handles to tree_nodes[] indices, which fixed most of the original decoupling by construction and already prunes regular-node annotations in free_node2(). Two gaps remained, plus one qtFRED regression: - free_node2() only removed annotations keyed by node index, never a root-label annotation keyed rootKey(formula). Deleting an event left that annotation live, so a later event whose formula reused the slot inherited the deleted event's annotation. Remove the root key too; since the global Event_annotations are only rewritten at save, the removal still survives a Cancel. - Inserting an operator above a labeled root changes the event's formula node, orphaning the root-label annotation keyed to the old index. Re-key it in insert_operator() so it follows the event. (This is the only such flow: replace_operator() reuses the node in place.) - qtFRED reorderByRootFormulaOrder() reloaded annotations from the global state, discarding any annotation edits made since the dialog was opened. A root reorder changes neither node indices nor formulas, so drop the reload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Annotation +Path: values written by pre-refactor FRED were built over the UI tree, where each event's root item is the event-name label and every element after the event index is a child position at successive levels below it: [event] is the label, [event, 0] the top operator (the label's only child), [event, 0, k...] its descendants. The refactored SexpAnnotationModel built paths over tree_nodes[], which represents the label's single child directly as the formula node, so paths came out one level shallower: existing missions' annotations resolved one level too deep (and were re-saved wrong), and a top-operator annotation produced the length-1 path that decodes as a root-label key, silently migrating onto the event label. Emit and consume the label-level child position in buildPath/resolveFromPath to preserve the on-disk contract. Child 0 resolves to the formula node and any other index fails resolution, matching the old traverse_path behavior of walking off the label's sibling list. Annotations on the event label itself are unaffected: they remain the length-1 path handled by the root-key branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Root-label annotations (event-name comments/colors) that lose their node — e.g. because their event was deleted — could survive indefinitely and surface on the wrong items: - saveToGlobal's root-key branch kept an unresolvable annotation in Event_annotations with an empty path instead of marking it for pruning, so once a zombie was persisted in a mission file it survived every save/load cycle. - getByKey(-1) matched annotations whose node_index had been reset to the unresolved sentinel, so such a zombie's comment could display on any item that has no annotation key of its own. Guard findByKey against the -1 sentinel, and prune any annotation whose rebuilt path is empty at save time. (The complementary in-session fixes — removing a root-label annotation when its event is deleted and re-keying it when insert-operator changes the formula node — are handled in the shared model and actions by 'Keep event annotations attached to their node across tree mutations'.) Also restore root-label comments in the FRED2 help box: update_help resolved comments by model node index, which is -1 for labels. Route it through a get_item_comment virtual that the event editor's tree overrides with its annotation key lookup, matching pre-refactor behavior. (qtFRED renders comments per-item via NoteRole, which already covers labels; its help box does not show comments by design.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Goober5000
force-pushed
the
fix/annotations_post_refactor
branch
from
July 11, 2026 04:46
e1826de to
5aabccc
Compare
Goober5000
enabled auto-merge (rebase)
July 11, 2026 04:48
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.
New version of #7538 subsequent to the sexp_tree refactor.
Follow-up to the sexp_tree refactor (#7301). The refactor changed how event annotations are tracked internally, which fixed most of the old detachment problems — but annotations on the event name itself could still be lost or misplaced, and annotations saved by pre-refactor FRED no longer loaded correctly. This PR fixes those cases in both FRED2 and qtFRED: