CraterCrashGH-574 Strengthen validation for adding/renaming component…

… items

src/editor/component_panels/component_panel.cpp

@@ -69,7 +69,7 @@ void OrchestratorScriptComponentPanel::_tree_item_edited()
     ERR_FAIL_COND_MSG(!item, "Cannot edit item when no item selected");
 
     const String old_name = item->get_meta("__name");
-    const String new_name = item->get_text(0);
+    String new_name = item->get_text(0);
 
     // Nothing to edit if they're identical
     if (old_name.match(new_name))
@@ -78,7 +78,12 @@ void OrchestratorScriptComponentPanel::_tree_item_edited()
         return;
     }
 
-    _handle_item_renamed(old_name, new_name);
+    new_name = NameUtils::create_unique_name(new_name, _get_existing_names());
+    if (!_handle_item_renamed(old_name, new_name))
+    {
+        item->set_text(0, item->get_meta("__rollback_name"));
+        return;
+    }
 
     update();
 }
@@ -231,6 +236,23 @@ void OrchestratorScriptComponentPanel::_show_notification(const String& p_messag
     _notify->popup_centered();
 }
 
+void OrchestratorScriptComponentPanel::_show_invalid_name(const String& p_type, bool p_supports_friendly_names)
+{
+    String message = vformat("The %s name is not valid. Names must follow these requirements:\n\n", p_type);
+    message += "* Must start with a letter (A-Z, a-z) or an underscore ('_')\n";
+    message += "* Can include letters (A-Z, a-z), numbers (0-9), and underscores ('_')\n";
+    message += "* Should not start with a number (0-9)\n";
+    message += "* Cannot contain spaces or special characters\n";
+
+    if (p_supports_friendly_names)
+    {
+        message += vformat("\nIf you want a space to appear in the %s name, please use camel-case (MyName).\n", p_type);
+        message += "With friendly names enabled, the name will be rendered as 'My Name' automatically.";
+    }
+
+    _show_notification(message);
+}
+
 void OrchestratorScriptComponentPanel::_confirm_removal(TreeItem* p_item)
 {
     _confirm->set_text(_get_remove_confirm_text(p_item) + "\n\nDo you want to continue?");
@@ -270,7 +292,7 @@ bool OrchestratorScriptComponentPanel::_find_child_and_activate(const String& p_
                 {
                     Ref<SceneTreeTimer> timer = get_tree()->create_timer(0.1f);
                     if (timer.is_valid())
-                        timer->connect("timeout", callable_mp(_tree, &Tree::edit_selected).bind(true));
+                        timer->connect("timeout", callable_mp(this, &OrchestratorScriptComponentPanel::_edit_selected_tree_item));
                 }
 
                 return true;

src/editor/component_panels/component_panel.h

@@ -97,6 +97,11 @@ class OrchestratorScriptComponentPanel : public VBoxContainer
     /// @param p_message the text to notify
     void _show_notification(const String& p_message);
 
+    /// Shows a common dialog error about invalid identifier names.
+    /// @param p_type the type
+    /// @param p_supports_friendly_names true to show detail about friendly names
+    void _show_invalid_name(const String& p_type, bool p_supports_friendly_names = true);
+
     /// Presents the user a dialog, confirming the removal of the tree item.
     /// @param p_item the item to be removed, should not be null
     void _confirm_removal(TreeItem* p_item);

src/editor/component_panels/functions_panel.cpp

@@ -160,7 +160,15 @@ bool OrchestratorScriptFunctionsComponentPanel::_handle_item_renamed(const Strin
         return false;
     }
 
-    _orchestration->rename_function(p_old_name, p_new_name);
+    if (!p_new_name.is_valid_identifier())
+    {
+        _show_invalid_name("function");
+        return false;
+    }
+
+    if (!_orchestration->rename_function(p_old_name, p_new_name))
+        return false;
+
     emit_signal("graph_renamed", p_old_name, p_new_name);
     return true;
 }

src/editor/component_panels/graphs_panel.cpp

@@ -184,7 +184,15 @@ bool OrchestratorScriptGraphsComponentPanel::_handle_item_renamed(const String&
         return false;
     }
 
-    _orchestration->rename_graph(p_old_name, p_new_name);
+    if (!p_new_name.is_valid_identifier())
+    {
+        _show_invalid_name("graph");
+        return false;
+    }
+
+    if (!_orchestration->rename_graph(p_old_name, p_new_name))
+        return false;
+
     emit_signal("graph_renamed", p_old_name, p_new_name);
     return true;
 }

src/editor/component_panels/signals_panel.cpp

@@ -90,8 +90,13 @@ bool OrchestratorScriptSignalsComponentPanel::_handle_item_renamed(const String&
         return false;
     }
 
-    _orchestration->rename_custom_user_signal(p_old_name, p_new_name);
-    return true;
+    if (!p_new_name.is_valid_identifier())
+    {
+        _show_invalid_name("signal", false);
+        return false;
+    }
+
+    return _orchestration->rename_custom_user_signal(p_old_name, p_new_name);
 }
 
 void OrchestratorScriptSignalsComponentPanel::_handle_remove(TreeItem* p_item)

src/editor/component_panels/variables_panel.cpp

@@ -153,8 +153,13 @@ bool OrchestratorScriptVariablesComponentPanel::_handle_item_renamed(const Strin
         return false;
     }
 
-    _orchestration->rename_variable(p_old_name, p_new_name);
-    return true;
+    if (!p_new_name.is_valid_identifier())
+    {
+        _show_invalid_name("variable", false);
+        return false;
+    }
+
+    return _orchestration->rename_variable(p_old_name, p_new_name);
 }
 
 void OrchestratorScriptVariablesComponentPanel::_handle_remove(TreeItem* p_item)

src/orchestration/orchestration.cpp

@@ -514,6 +514,7 @@ Ref<OScriptGraph> Orchestration::create_graph(const StringName& p_name, int p_fl
 {
     ERR_FAIL_COND_V_MSG(has_graph(p_name), nullptr, "A graph with that name already exists: " + p_name);
     ERR_FAIL_COND_V_MSG(p_name.is_empty(), nullptr, "A name is required to create a graph.");
+    ERR_FAIL_COND_V_MSG(!p_name.is_valid_identifier(), nullptr, "The name is not a valid graph name.");
 
     Ref<OScriptGraph> graph(memnew(OScriptGraph));
     graph->_orchestration = this;
@@ -571,16 +572,23 @@ Ref<OScriptGraph> Orchestration::find_graph(const Ref<OScriptNode>& p_node)
     ERR_FAIL_V_MSG(nullptr, "No graph contains the node with the unique ID: " + itos(p_node->get_id()));
 }
 
-void Orchestration::rename_graph(const StringName& p_old_name, const StringName& p_new_name)
+bool Orchestration::rename_graph(const StringName& p_old_name, const StringName& p_new_name)
 {
-    ERR_FAIL_COND_MSG(!has_graph(p_old_name), "No graph exists with the old name: " + p_old_name);
-    ERR_FAIL_COND_MSG(has_graph(p_new_name), "A graph already exists with the new name: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(!has_graph(p_old_name), false, "No graph exists with the old name: " + p_old_name);
+    ERR_FAIL_COND_V_MSG(has_graph(p_new_name), false, "A graph already exists with the new name: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(!p_new_name.is_valid_identifier(), false, "The new graph name is not a valid.");
+
+    UtilityFunctions::print("Create Graph with name [", p_old_name, "] to [", p_new_name, "]");
 
     Ref<OScriptGraph> graph = get_graph(p_old_name);
+    if (!graph.is_valid())
+        return false;
+
     graph->set_graph_name(p_new_name);
 
     _graphs[p_new_name] = graph;
     _graphs.erase(p_old_name);
+    return true;
 }
 
 Vector<Ref<OScriptGraph>> Orchestration::get_graphs() const
@@ -675,31 +683,38 @@ Ref<OScriptFunction> Orchestration::find_function(const Guid& p_guid) const
     return nullptr;
 }
 
-void Orchestration::rename_function(const StringName& p_old_name, const StringName& p_new_name)
+bool Orchestration::rename_function(const StringName& p_old_name, const StringName& p_new_name)
 {
     // Ignore if the old/new names are the same
     if (p_old_name == p_new_name)
-        return;
+        return false;
 
-    ERR_FAIL_COND_MSG(_has_instances(), "Cannot rename function, instances exist.");
-    ERR_FAIL_COND_MSG(!has_function(p_old_name), "Cannot rename, no function found with old name: " + p_old_name);
-    ERR_FAIL_COND_MSG(has_function(p_new_name), "Cannot rename, a function already exists with new name: " + p_new_name);
-    ERR_FAIL_COND_MSG(!String(p_new_name).is_valid_identifier(), "New function name is invalid: " + p_new_name);
-    ERR_FAIL_COND_MSG(has_variable(p_new_name), "Cannot rename function, a variable with name already exists: " + p_new_name);
-    ERR_FAIL_COND_MSG(has_custom_signal(p_new_name), "Cannot rename function, a signal with the name already exists: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(_has_instances(), false, "Cannot rename function, instances exist.");
+    ERR_FAIL_COND_V_MSG(!has_function(p_old_name), false, "Cannot rename, no function found with old name: " + p_old_name);
+    ERR_FAIL_COND_V_MSG(has_function(p_new_name), false, "Cannot rename, a function already exists with new name: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(!String(p_new_name).is_valid_identifier(), false, "New function name is invalid: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(has_variable(p_new_name), false, "Cannot rename function, a variable with name already exists: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(has_custom_signal(p_new_name), false, "Cannot rename function, a signal with the name already exists: " + p_new_name);
 
     const Ref<OScriptFunction> function = _functions[p_old_name];
-    function->rename(p_new_name);
-
-    _functions.erase(p_old_name);
-    _functions[p_new_name] = function;
+    if (!function.is_valid() || !function->can_be_renamed())
+        return false;
 
     // Rename function graph, if found
     const Ref<OScriptGraph> function_graph = find_graph(p_old_name);
     if (function_graph.is_valid() && function_graph->get_flags().has_flag(OScriptGraph::GraphFlags::GF_FUNCTION))
-        rename_graph(p_old_name, p_new_name);
+    {
+        if (!rename_graph(p_old_name, p_new_name))
+            return false;
+    }
+
+    function->rename(p_new_name);
+
+    _functions.erase(p_old_name);
+    _functions[p_new_name] = function;
 
     _self->emit_signal("functions_changed");
+    return true;
 }
 
 PackedStringArray Orchestration::get_function_names() const
@@ -789,15 +804,15 @@ Ref<OScriptVariable> Orchestration::get_variable(const StringName& p_name)
     return has_variable(p_name) ? _variables[p_name] : nullptr;
 }
 
-void Orchestration::rename_variable(const StringName& p_old_name, const StringName& p_new_name)
+bool Orchestration::rename_variable(const StringName& p_old_name, const StringName& p_new_name)
 {
     if (p_old_name == p_new_name)
-        return;
+        return false;
 
-    ERR_FAIL_COND_MSG(_has_instances(), "Cannot rename variable, instances exist.");
-    ERR_FAIL_COND_MSG(!has_variable(p_old_name), "Cannot rename, no variable exists with the old name: " + p_old_name);
-    ERR_FAIL_COND_MSG(has_variable(p_new_name), "Cannot rename, a variable already exists with the new name: " + p_new_name);
-    ERR_FAIL_COND_MSG(!String(p_new_name).is_valid_identifier(), "Cannot rename, variable name is not valid: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(_has_instances(), false, "Cannot rename variable, instances exist.");
+    ERR_FAIL_COND_V_MSG(!has_variable(p_old_name), false, "Cannot rename, no variable exists with the old name: " + p_old_name);
+    ERR_FAIL_COND_V_MSG(has_variable(p_new_name), false, "Cannot rename, a variable already exists with the new name: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(!String(p_new_name).is_valid_identifier(), false, "Cannot rename, variable name is not valid: " + p_new_name);
 
     const Ref<OScriptVariable> variable = _variables[p_old_name];
     variable->set_variable_name(p_new_name);
@@ -812,6 +827,8 @@ void Orchestration::rename_variable(const StringName& p_old_name, const StringNa
     #ifdef TOOLS_ENABLED
     _update_placeholders();
     #endif
+
+    return true;
 }
 
 Vector<Ref<OScriptVariable>> Orchestration::get_variables() const
@@ -849,6 +866,7 @@ bool Orchestration::has_custom_signal(const StringName& p_name) const
 Ref<OScriptSignal> Orchestration::create_custom_signal(const StringName& p_name)
 {
     ERR_FAIL_COND_V_MSG(has_custom_signal(p_name), nullptr, "A custom signal already exists with the name: " + p_name);
+    ERR_FAIL_COND_V_MSG(!p_name.is_valid_identifier(), nullptr, "The name is not a valid signal name.");
 
     MethodInfo method;
     method.name = p_name;
@@ -894,15 +912,15 @@ Ref<OScriptSignal> Orchestration::find_custom_signal(const StringName& p_name) c
     return has_custom_signal(p_name) ? _signals[p_name] : nullptr;
 }
 
-void Orchestration::rename_custom_user_signal(const StringName& p_old_name, const StringName& p_new_name)
+bool Orchestration::rename_custom_user_signal(const StringName& p_old_name, const StringName& p_new_name)
 {
     if (p_old_name == p_new_name)
-        return;
+        return false;
 
-    ERR_FAIL_COND_MSG(_has_instances(), "Cannot rename custom signal, instances exist.");
-    ERR_FAIL_COND_MSG(!has_custom_signal(p_old_name), "No custom signal exists with the old name: " + p_old_name);
-    ERR_FAIL_COND_MSG(has_custom_signal(p_new_name), "A custom signal already exists with the new name: " + p_new_name);
-    ERR_FAIL_COND_MSG(!String(p_new_name).is_valid_identifier(), "The custom signal name is invalid: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(_has_instances(), false, "Cannot rename custom signal, instances exist.");
+    ERR_FAIL_COND_V_MSG(!has_custom_signal(p_old_name), false, "No custom signal exists with the old name: " + p_old_name);
+    ERR_FAIL_COND_V_MSG(has_custom_signal(p_new_name), false, "A custom signal already exists with the new name: " + p_new_name);
+    ERR_FAIL_COND_V_MSG(!String(p_new_name).is_valid_identifier(), false, "The custom signal name is invalid: " + p_new_name);
 
     const Ref<OScriptSignal> signal = find_custom_signal(p_old_name);
     signal->rename(p_new_name);
@@ -917,6 +935,8 @@ void Orchestration::rename_custom_user_signal(const StringName& p_old_name, cons
     #ifdef TOOLS_ENABLED
     _update_placeholders();
     #endif
+
+    return true;
 }
 
 Vector<Ref<OScriptSignal>> Orchestration::get_custom_signals() const

src/orchestration/orchestration.h

@@ -190,7 +190,7 @@ class Orchestration
     Ref<OScriptGraph> get_graph(const StringName& p_name) const;
     Ref<OScriptGraph> find_graph(const StringName& p_name) const;
     Ref<OScriptGraph> find_graph(const Ref<OScriptNode>& p_node);
-    void rename_graph(const StringName& p_old_name, const StringName& p_new_name);
+    bool rename_graph(const StringName& p_old_name, const StringName& p_new_name);
     Vector<Ref<OScriptGraph>> get_graphs() const;
     //~ End Graph Interface
 
@@ -200,7 +200,7 @@ class Orchestration
     void remove_function(const StringName& p_name);
     Ref<OScriptFunction> find_function(const StringName& p_name) const;
     Ref<OScriptFunction> find_function(const Guid& p_guid) const;
-    void rename_function(const StringName& p_old_name, const StringName& p_new_name);
+    bool rename_function(const StringName& p_old_name, const StringName& p_new_name);
     PackedStringArray get_function_names() const;
     int get_function_node_id(const StringName& p_name) const;
     Vector<Ref<OScriptFunction>> get_functions() const;
@@ -211,7 +211,7 @@ class Orchestration
     Ref<OScriptVariable> create_variable(const StringName& p_name, Variant::Type p_type = Variant::NIL);
     void remove_variable(const StringName& p_name);
     Ref<OScriptVariable> get_variable(const StringName& p_name);
-    void rename_variable(const StringName& p_old_name, const StringName& p_new_name);
+    bool rename_variable(const StringName& p_old_name, const StringName& p_new_name);
     Vector<Ref<OScriptVariable>> get_variables() const;
     PackedStringArray get_variable_names() const;
     bool can_remove_variable(const StringName& p_name) const;
@@ -223,7 +223,7 @@ class Orchestration
     void remove_custom_signal(const StringName& p_name);
     Ref<OScriptSignal> get_custom_signal(const StringName& p_name);
     Ref<OScriptSignal> find_custom_signal(const StringName& p_name) const;
-    void rename_custom_user_signal(const StringName& p_old_name, const StringName& p_new_name);
+    bool rename_custom_user_signal(const StringName& p_old_name, const StringName& p_new_name);
     Vector<Ref<OScriptSignal>> get_custom_signals() const;
     PackedStringArray get_custom_signal_names() const;
     bool can_remove_custom_signal(const StringName& p_name) const;