From e8f02735160b7a487ffc9da47567f234f3aad5c6 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Mon, 7 Sep 2026 12:49:18 -0400 Subject: [PATCH 1/3] Fix QtFRED root notifications when inserting nested operators Only emit rootNodeFormulaChanged when the wrapped node is a formula root. Inserting above a nested expression previously reported a child as an event, goal, or cutscene root, causing assertions and out-of-bounds accesses in the receiving dialog model. Co-Authored-By: OpenAI Codex --- qtfred/src/ui/widgets/sexp_tree_view.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qtfred/src/ui/widgets/sexp_tree_view.cpp b/qtfred/src/ui/widgets/sexp_tree_view.cpp index 2da22b6355e..ce885d3361d 100644 --- a/qtfred/src/ui/widgets/sexp_tree_view.cpp +++ b/qtfred/src/ui/widgets/sexp_tree_view.cpp @@ -1815,9 +1815,10 @@ void sexp_tree_view::insertOperatorAction(int op) { auto* old_item = tree_item_handle(tree_nodes[item_index]); auto* root_parent = old_item ? old_item->parent() : nullptr; const int old_item_index = item_index; + const bool is_formula_root = tree_nodes[old_item_index].parent == -1; const int node = _actions.insert_operator(op, root_parent); - if (_interface->getFlags()[TreeFlags::LabeledRoot] && root_parent != nullptr) { + if (is_formula_root && _interface->getFlags()[TreeFlags::LabeledRoot] && root_parent != nullptr) { rootNodeFormulaChanged(old_item_index, node); root_parent->setData(0, FormulaDataRole, node); } From e239d83479fc0c0f710fb5651ccad64205b3e121 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Mon, 7 Sep 2026 12:53:14 -0400 Subject: [PATCH 2/3] Give each QtFRED ship independently owned arrival and departure cues Pass the tree model to the cue update handlers and serialize a separate expression for each eligible ship. Sharing one formula across selected ships caused double frees on subsequent edits and left other ships referencing freed nodes when only one ship was changed. This also avoids allocating unused expressions when cue updates are disabled or no selected ships are eligible. Co-Authored-By: OpenAI Codex --- .../ShipEditor/ShipEditorDialogModel.cpp | 19 +++++++++++-------- .../ShipEditor/ShipEditorDialogModel.h | 6 ++++-- .../dialogs/ShipEditor/ShipEditorDialog.cpp | 4 ++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.cpp b/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.cpp index dd8cfa18bcf..a0765e8ff5d 100644 --- a/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.cpp +++ b/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.cpp @@ -14,6 +14,7 @@ #include "mission/missionmessage.h" #include "mission/missionparse.h" #include "missioneditor/common.h" +#include "missioneditor/sexp_tree_model.h" #include #include @@ -1332,12 +1333,11 @@ bool ShipEditorDialogModel::getArrivalCue() const return _updateArrival; } -void ShipEditorDialogModel::setArrivalTreeDirty(int formula) +void ShipEditorDialogModel::setArrivalTreeDirty(const SexpTreeModel& tree) { if (_multiEdit && !_updateArrival) return; - _arrivalTreeFormula = formula; _updateArrival = true; for (auto* ptr = GET_FIRST(&obj_used_list); ptr != END_OF_LIST(&obj_used_list); ptr = GET_NEXT(ptr)) { @@ -1345,9 +1345,11 @@ void ShipEditorDialogModel::setArrivalTreeDirty(int formula) auto i = ptr->instance; if (Ships[i].wingnum >= 0) continue; - if (Ships[i].arrival_cue >= 0 && Ships[i].arrival_cue != formula) + if (Ships[i].arrival_cue >= 0) free_sexp2(Ships[i].arrival_cue); - Ships[i].arrival_cue = formula; + // Each ship owns its cue, so serialize a separate expression for each one. + Ships[i].arrival_cue = tree.save_tree(); + _arrivalTreeFormula = Ships[i].arrival_cue; } } @@ -1494,12 +1496,11 @@ bool ShipEditorDialogModel::getDepartureCue() const return _updateDeparture; } -void ShipEditorDialogModel::setDepartureTreeDirty(int formula) +void ShipEditorDialogModel::setDepartureTreeDirty(const SexpTreeModel& tree) { if (_multiEdit && !_updateDeparture) return; - _departureTreeFormula = formula; _updateDeparture = true; for (auto* ptr = GET_FIRST(&obj_used_list); ptr != END_OF_LIST(&obj_used_list); ptr = GET_NEXT(ptr)) { @@ -1507,9 +1508,11 @@ void ShipEditorDialogModel::setDepartureTreeDirty(int formula) auto i = ptr->instance; if (Ships[i].wingnum >= 0) continue; - if (Ships[i].departure_cue >= 0 && Ships[i].departure_cue != formula) + if (Ships[i].departure_cue >= 0) free_sexp2(Ships[i].departure_cue); - Ships[i].departure_cue = formula; + // Each ship owns its cue, so serialize a separate expression for each one. + Ships[i].departure_cue = tree.save_tree(); + _departureTreeFormula = Ships[i].departure_cue; } } diff --git a/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.h b/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.h index 550ec24e958..b69ab2034d1 100644 --- a/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.h +++ b/qtfred/src/mission/dialogs/ShipEditor/ShipEditorDialogModel.h @@ -4,6 +4,8 @@ #include "mission/util.h" #include "ship/ship.h" +class SexpTreeModel; + namespace fso::fred::dialogs { class ShipEditorDialogModel : public AbstractDialogModel { @@ -84,7 +86,7 @@ class ShipEditorDialogModel : public AbstractDialogModel { void setArrivalCue(bool updateCue); bool getArrivalCue() const; - void setArrivalTreeDirty(int formula); + void setArrivalTreeDirty(const SexpTreeModel& tree); int getArrivalFormula() const; void setNoArrivalWarp(int state); @@ -107,7 +109,7 @@ class ShipEditorDialogModel : public AbstractDialogModel { void setDepartureCue(bool updateCue); bool getDepartureCue() const; - void setDepartureTreeDirty(int formula); + void setDepartureTreeDirty(const SexpTreeModel& tree); int getDepartureFormula() const; void setNoDepartureWarp(int state); int getNoDepartureWarp() const; diff --git a/qtfred/src/ui/dialogs/ShipEditor/ShipEditorDialog.cpp b/qtfred/src/ui/dialogs/ShipEditor/ShipEditorDialog.cpp index c52bbc03da1..0f649313f4c 100644 --- a/qtfred/src/ui/dialogs/ShipEditor/ShipEditorDialog.cpp +++ b/qtfred/src/ui/dialogs/ShipEditor/ShipEditorDialog.cpp @@ -862,7 +862,7 @@ void ShipEditorDialog::on_noArrivalWarpCheckBox_stateChanged(int state) } void ShipEditorDialog::on_arrivalTree_modified() { - _model->setArrivalTreeDirty(ui->arrivalTree->_model.save_tree()); + _model->setArrivalTreeDirty(ui->arrivalTree->_model); } void ShipEditorDialog::on_arrivalTree_helpChanged(const QString& help) { @@ -898,7 +898,7 @@ void ShipEditorDialog::on_updateDepartureCueCheckBox_toggled(bool value) } void fred::dialogs::ShipEditorDialog::on_departureTree_modified() { - _model->setDepartureTreeDirty(ui->departureTree->_model.save_tree()); + _model->setDepartureTreeDirty(ui->departureTree->_model); } void ShipEditorDialog::on_departureTree_helpChanged(const QString& help) { From f81bb68506534d351c260574b85d4fbcab4d73dc Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Mon, 7 Sep 2026 12:57:48 -0400 Subject: [PATCH 3/3] Synchronize QtFRED item editability after SEXP node changes Update native Qt editability alongside the shared model flags so variable and container references cannot be edited as plain text. Restore inline editing when a reference is replaced with ordinary data, and preserve item flags when moving or copying branches. Reject edit requests for protected items before setting the editing state, preventing later programmatic changes from being treated as user edits. Co-Authored-By: OpenAI Codex --- code/missioneditor/sexp_tree_actions.cpp | 34 ++++++++++++++---------- code/missioneditor/sexp_tree_actions.h | 3 +++ code/missioneditor/sexp_tree_model.h | 2 ++ qtfred/src/ui/widgets/sexp_tree_view.cpp | 13 +++++++++ qtfred/src/ui/widgets/sexp_tree_view.h | 1 + 5 files changed, 39 insertions(+), 14 deletions(-) diff --git a/code/missioneditor/sexp_tree_actions.cpp b/code/missioneditor/sexp_tree_actions.cpp index b5820663895..5807495cd1e 100644 --- a/code/missioneditor/sexp_tree_actions.cpp +++ b/code/missioneditor/sexp_tree_actions.cpp @@ -13,6 +13,12 @@ SexpTreeActions::SexpTreeActions(SexpTreeModel& model, ISexpTreeUI& ui) { } +void SexpTreeActions::set_node_flags(int node_index, int flags) +{ + _model.tree_nodes[node_index].flags = flags; + _ui.ui_set_item_editable(_model.tree_nodes[node_index].handle, (flags & EDITABLE) != 0); +} + // Free all children of a node from both the model and the UI widget. // After this call, the node has no children in either layer. void SexpTreeActions::clear_node_children(int node_index) @@ -52,7 +58,7 @@ void SexpTreeActions::replace_data(const char* data, int type) _ui.ui_set_item_text(h, data); NodeImage bmap = _model.get_data_image(node_idx); _ui.ui_set_item_image(h, bmap); - _model.tree_nodes[node_idx].flags = EDITABLE; + set_node_flags(node_idx, EDITABLE); // check remaining data beyond replaced data for validity verify_and_fix_arguments(_model.tree_nodes[node_idx].parent); @@ -82,7 +88,7 @@ void SexpTreeActions::replace_variable_data(int var_idx, int type) void* h = _model.tree_nodes[node_idx].handle; _ui.ui_set_item_text(h, buf); _ui.ui_set_item_image(h, NodeImage::VARIABLE); - _model.tree_nodes[node_idx].flags = NOT_EDITABLE; + set_node_flags(node_idx, NOT_EDITABLE); // check remaining data beyond replaced data for validity verify_and_fix_arguments(_model.tree_nodes[node_idx].parent); @@ -105,7 +111,7 @@ void SexpTreeActions::replace_container_name(const sexp_container& container) void* h = _model.tree_nodes[node_idx].handle; _ui.ui_set_item_image(h, NodeImage::CONTAINER_NAME); _ui.ui_set_item_text(h, container.container_name.c_str()); - _model.tree_nodes[node_idx].flags = NOT_EDITABLE; + set_node_flags(node_idx, NOT_EDITABLE); if (_model.modified) *_model.modified = 1; @@ -151,7 +157,7 @@ void SexpTreeActions::replace_container_data(const sexp_container& container, void* h = _model.tree_nodes[node_idx].handle; _ui.ui_set_item_image(h, NodeImage::CONTAINER_DATA); _ui.ui_set_item_text(h, container.container_name.c_str()); - _model.tree_nodes[node_idx].flags = NOT_EDITABLE; + set_node_flags(node_idx, NOT_EDITABLE); if (set_default_modifier) { add_default_modifier(container); @@ -174,7 +180,7 @@ void SexpTreeActions::replace_operator(const char* op) _model.set_node(node_idx, (SEXPT_OPERATOR | SEXPT_VALID), op); void* h = _model.tree_nodes[node_idx].handle; _ui.ui_set_item_text(h, op); - _model.tree_nodes[node_idx].flags = OPERAND; + set_node_flags(node_idx, OPERAND); if (_model.modified) *_model.modified = 1; @@ -207,10 +213,10 @@ void SexpTreeActions::expand_operator(int node) Assertion(_model.tree_nodes[data].child == -1, "Child %d of node %d unexpectedly has its own children (child %d)", data, node, _model.tree_nodes[data].child); _ui.ui_set_item_text(h, _model.tree_nodes[node].text); - _model.tree_nodes[node].flags = OPERAND; + set_node_flags(node, OPERAND); NodeImage bmap = _model.get_data_image(data); _model.tree_nodes[data].handle = _ui.ui_insert_item(_model.tree_nodes[data].text, bmap, h, nullptr); - _model.tree_nodes[data].flags = EDITABLE; + set_node_flags(data, EDITABLE); _ui.ui_expand_item(h); } } @@ -232,7 +238,7 @@ int SexpTreeActions::add_data(const char* data, int type) _model.set_node(node, type, data); NodeImage bmap = _model.get_data_image(node); _model.tree_nodes[node].handle = _ui.ui_insert_item(data, bmap, _model.tree_nodes[node_idx].handle, nullptr); - _model.tree_nodes[node].flags = EDITABLE; + set_node_flags(node, EDITABLE); if (_model.modified) *_model.modified = 1; return node; @@ -252,7 +258,7 @@ int SexpTreeActions::add_variable_data(const char* data, int type) int node = _model.allocate_node(node_idx); _model.set_node(node, type, data); _model.tree_nodes[node].handle = _ui.ui_insert_item(data, NodeImage::VARIABLE, _model.tree_nodes[node_idx].handle, nullptr); - _model.tree_nodes[node].flags = NOT_EDITABLE; + set_node_flags(node, NOT_EDITABLE); if (_model.modified) *_model.modified = 1; return node; @@ -275,7 +281,7 @@ int SexpTreeActions::add_container_name(const char* container_name) _model.set_node(node, (SEXPT_VALID | SEXPT_CONTAINER_NAME | SEXPT_STRING), container_name); _model.tree_nodes[node].handle = _ui.ui_insert_item(container_name, NodeImage::CONTAINER_NAME, _model.tree_nodes[node_idx].handle, nullptr); - _model.tree_nodes[node].flags = NOT_EDITABLE; + set_node_flags(node, NOT_EDITABLE); if (_model.modified) *_model.modified = 1; return node; @@ -296,7 +302,7 @@ void SexpTreeActions::add_container_data(const char* container_name) _model.set_node(node, (SEXPT_VALID | SEXPT_CONTAINER_DATA | SEXPT_STRING), container_name); _model.tree_nodes[node].handle = _ui.ui_insert_item(container_name, NodeImage::CONTAINER_DATA, _model.tree_nodes[node_idx].handle, nullptr); - _model.tree_nodes[node].flags = NOT_EDITABLE; + set_node_flags(node, NOT_EDITABLE); _model.item_index = node; if (_model.modified) *_model.modified = 1; @@ -322,7 +328,7 @@ void SexpTreeActions::add_operator(const char* op, void* parent_handle) _model.tree_nodes[node].handle = _ui.ui_insert_item(op, NodeImage::OPERATOR, _model.tree_nodes[_model.item_index].handle, nullptr); } - _model.tree_nodes[node].flags = OPERAND; + set_node_flags(node, OPERAND); _model.item_index = node; if (_model.modified) *_model.modified = 1; @@ -391,7 +397,7 @@ void SexpTreeActions::add_or_replace_operator(int op, int replace_flag) if (i < 0) { _model.set_node(_model.item_index, (SEXPT_OPERATOR | SEXPT_VALID), Operators[op].text.c_str()); _ui.ui_set_item_text(_model.tree_nodes[_model.item_index].handle, Operators[op].text.c_str()); - _model.tree_nodes[_model.item_index].flags = OPERAND; + set_node_flags(_model.item_index, OPERAND); return; } } @@ -498,7 +504,6 @@ int SexpTreeActions::insert_operator(int op, void* root_parent_handle) const int node = _model.allocate_node(parent_node, wrapped_node); _model.set_node(node, (SEXPT_OPERATOR | SEXPT_VALID), Operators[op].text.c_str()); - _model.tree_nodes[node].flags = node_flags; void* parent_handle = nullptr; if (parent_node >= 0) { @@ -521,6 +526,7 @@ int SexpTreeActions::insert_operator(int op, void* root_parent_handle) } _model.tree_nodes[node].handle = _ui.ui_insert_item(Operators[op].text.c_str(), NodeImage::OPERATOR, parent_handle, wrapped_handle); + set_node_flags(node, node_flags); _ui.ui_move_branch(wrapped_node, node); _model.item_index = node; diff --git a/code/missioneditor/sexp_tree_actions.h b/code/missioneditor/sexp_tree_actions.h index 24109550617..5d5914bea48 100644 --- a/code/missioneditor/sexp_tree_actions.h +++ b/code/missioneditor/sexp_tree_actions.h @@ -125,6 +125,9 @@ class SexpTreeActions { bool rename_container_nodes(const SCP_string& old_name, const SCP_string& new_name); private: + // Update model flags and synchronize the widget's inline editability. + void set_node_flags(int node_index, int flags); + // Delete all UI children of a node and free their model data. // Resets the model's child link to -1. void clear_node_children(int node_index); diff --git a/code/missioneditor/sexp_tree_model.h b/code/missioneditor/sexp_tree_model.h index 61b171e09f5..1a2fb7b8003 100644 --- a/code/missioneditor/sexp_tree_model.h +++ b/code/missioneditor/sexp_tree_model.h @@ -248,6 +248,8 @@ class ISexpTreeUI { virtual void ui_set_item_text(void* handle, const char* text) = 0; // Update the icon of a tree item virtual void ui_set_item_image(void* handle, NodeImage image) = 0; + // Sync native widget editability. FRED2 checks the model flags when editing instead. + virtual void ui_set_item_editable(void* /*handle*/, bool /*editable*/) {} // Return the first child handle of the given tree item, or nullptr if none virtual void* ui_get_child_item(void* handle) const = 0; // Return true if the tree item has any children diff --git a/qtfred/src/ui/widgets/sexp_tree_view.cpp b/qtfred/src/ui/widgets/sexp_tree_view.cpp index ce885d3361d..ac0205157a4 100644 --- a/qtfred/src/ui/widgets/sexp_tree_view.cpp +++ b/qtfred/src/ui/widgets/sexp_tree_view.cpp @@ -312,6 +312,13 @@ void sexp_tree_view::ui_set_item_image(void* handle, NodeImage image) applyNodeIcon(static_cast(handle), image); } +// Keep Qt inline editing consistent with the shared model after node type changes. +void sexp_tree_view::ui_set_item_editable(void* handle, bool editable) +{ + auto* item = static_cast(handle); + item->setFlags(item->flags().setFlag(Qt::ItemIsEditable, editable)); +} + // Returns the first child QTreeWidgetItem, or nullptr. Called by _actions to traverse the tree. void* sexp_tree_view::ui_get_child_item(void* handle) const { @@ -587,6 +594,7 @@ QTreeWidgetItem* sexp_tree_view::move_branch(QTreeWidgetItem* source, QTreeWidge // Create the destination item const auto icon = source->icon(0); QTreeWidgetItem* h = insertWithIcon(source->text(0), icon, parent, after); + h->setFlags(source->flags()); if (idx < tree_nodes.size()) { tree_nodes[idx].handle = h; } @@ -630,6 +638,7 @@ void sexp_tree_view::copy_branch(QTreeWidgetItem* source, QTreeWidgetItem* paren const auto icon = source->icon(0); QTreeWidgetItem* h = insertWithIcon(source->text(0), icon, parent, after); + h->setFlags(source->flags()); size_t idx = 0; for (; idx < tree_nodes.size(); ++idx) { if (tree_nodes[idx].handle == source) { @@ -1883,6 +1892,10 @@ void sexp_tree_view::replaceStringDataHandler() { // Sets the _currently_editing flag and calls Qt's editItem() to start inline text editing. // The flag ensures that handleItemChange() only processes intentional edits, not programmatic changes. void sexp_tree_view::beginItemEdit(QTreeWidgetItem* item) { + if (item == nullptr || !item->flags().testFlag(Qt::ItemIsEditable)) { + return; + } + _currently_editing = true; editItem(item); diff --git a/qtfred/src/ui/widgets/sexp_tree_view.h b/qtfred/src/ui/widgets/sexp_tree_view.h index a72c3692965..9e163d048d7 100644 --- a/qtfred/src/ui/widgets/sexp_tree_view.h +++ b/qtfred/src/ui/widgets/sexp_tree_view.h @@ -195,6 +195,7 @@ class sexp_tree_view: public QTreeWidget, public ISexpTreeUI { void ui_delete_item(void* handle) override; //!< Deletes a QTreeWidgetItem void ui_set_item_text(void* handle, const char* text) override; //!< Sets display text via setText() void ui_set_item_image(void* handle, NodeImage image) override; //!< Sets icon via setIcon() + void ui_set_item_editable(void* handle, bool editable) override; //!< Syncs Qt::ItemIsEditable with model flags void* ui_get_child_item(void* handle) const override; //!< Returns first child item, or nullptr bool ui_has_children(void* handle) const override; //!< Returns true if childCount() > 0 void ui_expand_item(void* handle) override; //!< Expands a single item via setExpanded()