diff --git a/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp b/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp index 9649ecc8450a..2a88d290c4a9 100644 --- a/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp +++ b/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp @@ -41,6 +41,7 @@ class GD_CORE_API ExpressionPropertyReplacer const gd::Platform& platform_, const gd::ProjectScopedContainers& projectScopedContainers_, const gd::PropertiesContainer& targetPropertiesContainer_, + bool isParentTypeAVariable_, const std::unordered_map& oldToNewPropertyNames_, const std::unordered_set& removedPropertyNames_) : hasDoneRenaming(false), @@ -48,6 +49,7 @@ class GD_CORE_API ExpressionPropertyReplacer platform(platform_), projectScopedContainers(projectScopedContainers_), targetPropertiesContainer(targetPropertiesContainer_), + isParentTypeAVariable(isParentTypeAVariable_), oldToNewPropertyNames(oldToNewPropertyNames_), removedPropertyNames(removedPropertyNames_){}; virtual ~ExpressionPropertyReplacer(){}; @@ -69,16 +71,21 @@ class GD_CORE_API ExpressionPropertyReplacer void OnVisitNumberNode(NumberNode& node) override {} void OnVisitTextNode(TextNode& node) override {} void OnVisitVariableNode(VariableNode& node) override { + if (isParentTypeAVariable) { + // Do nothing, it's a variable. + if (node.child) node.child->Visit(*this); + return; + } + auto& propertiesContainersList = projectScopedContainers.GetPropertiesContainersList(); // The node represents a variable or an object name on which a variable // will be accessed, or a property with a child. - // Match the potential *new* name of the property, because refactorings are - // done after changes in the variables container. projectScopedContainers.MatchIdentifierWithName( - GetPotentialNewName(node.name), + // The property name is changed after the refactor operation. + node.name, [&]() { // Do nothing, it's an object variable. if (node.child) node.child->Visit(*this); @@ -100,16 +107,7 @@ class GD_CORE_API ExpressionPropertyReplacer // Do nothing, it's a parameter. if (node.child) node.child->Visit(*this); }, [&]() { - // This is something else - potentially a deleted property. - // Check if it's coming from the target container with - // properties to replace. - if (propertiesContainersList.HasPropertiesContainer( - targetPropertiesContainer)) { - // The node represents a property, that can come from the target - // (because the target is in the scope), replace or remove it: - RenameOrRemovePropertyOfTargetPropertyContainer(node.name); - } - + // Do nothing, it's something else. if (node.child) node.child->Visit(*this); }); } @@ -118,17 +116,24 @@ class GD_CORE_API ExpressionPropertyReplacer } void OnVisitVariableBracketAccessorNode( VariableBracketAccessorNode& node) override { + bool isGrandParentTypeAVariable = isParentTypeAVariable; + isParentTypeAVariable = false; node.expression->Visit(*this); + isParentTypeAVariable = isGrandParentTypeAVariable; if (node.child) node.child->Visit(*this); } void OnVisitIdentifierNode(IdentifierNode& node) override { + if (isParentTypeAVariable) { + // Do nothing, it's a variable. + return; + } + auto& propertiesContainersList = projectScopedContainers.GetPropertiesContainersList(); - // Match the potential *new* name of the property, because refactorings are - // done after changes in the variables container. projectScopedContainers.MatchIdentifierWithName( - GetPotentialNewName(node.identifierName), + // The property name is changed after the refactor operation + node.identifierName, [&]() { // Do nothing, it's an object variable. }, [&]() { @@ -145,22 +150,29 @@ class GD_CORE_API ExpressionPropertyReplacer }, [&]() { // Do nothing, it's a parameter. }, [&]() { - // This is something else - potentially a deleted property. - // Check if it's coming from the target container with - // properties to replace. - if (propertiesContainersList.HasPropertiesContainer( - targetPropertiesContainer)) { - // The node represents a property, that can come from the target - // (because the target is in the scope), replace or remove it: - RenameOrRemovePropertyOfTargetPropertyContainer(node.identifierName); - } + // Do nothing, it's something else. }); } void OnVisitObjectFunctionNameNode(ObjectFunctionNameNode& node) override {} - void OnVisitFunctionCallNode(FunctionCallNode& node) override { - for (auto& parameter : node.parameters) { - parameter->Visit(*this); + void OnVisitFunctionCallNode(FunctionCallNode &node) override { + bool isGrandParentTypeAVariable = isParentTypeAVariable; + for (auto ¶meter : node.parameters) { + const auto ¶meterMetadata = + gd::MetadataProvider::GetFunctionCallParameterMetadata( + platform, projectScopedContainers.GetObjectsContainersList(), + node, *parameter); + if (!parameterMetadata) { + continue; + } + const auto ¶meterTypeMetadata = + parameterMetadata->GetValueTypeMetadata(); + if (gd::EventsPropertyReplacer::CanContainProperty( + parameterTypeMetadata)) { + isParentTypeAVariable = parameterTypeMetadata.IsVariable(); + parameter->Visit(*this); + } } + isParentTypeAVariable = isGrandParentTypeAVariable; } void OnVisitEmptyNode(EmptyNode& node) override {} @@ -168,12 +180,6 @@ class GD_CORE_API ExpressionPropertyReplacer bool hasDoneRenaming; bool removedPropertyUsed; - const gd::String& GetPotentialNewName(const gd::String& oldName) { - return oldToNewPropertyNames.count(oldName) >= 1 - ? oldToNewPropertyNames.find(oldName)->second - : oldName; - } - bool RenameOrRemovePropertyOfTargetPropertyContainer( gd::String& propertyName) { if (oldToNewPropertyNames.count(propertyName) >= 1) { @@ -198,6 +204,7 @@ class GD_CORE_API ExpressionPropertyReplacer const std::unordered_set& removedPropertyNames; gd::String objectNameToUseForVariableAccessor; + bool isParentTypeAVariable; }; bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction, @@ -216,20 +223,16 @@ bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction, const gd::Expression& parameterValue, size_t parameterIndex, const gd::String& lastObjectName) { - const gd::String& type = parameterMetadata.GetType(); - - if (!gd::ParameterMetadata::IsExpression("variable", type) && - !gd::ParameterMetadata::IsExpression("number", type) && - !gd::ParameterMetadata::IsExpression("string", type)) - return; // Not an expression that can contain properties. - + if (!gd::EventsPropertyReplacer::CanContainProperty( + parameterMetadata.GetValueTypeMetadata())) { + return; + } auto node = parameterValue.GetRootNode(); if (node) { - ExpressionPropertyReplacer renamer(platform, - GetProjectScopedContainers(), - targetPropertiesContainer, - oldToNewPropertyNames, - removedPropertyNames); + ExpressionPropertyReplacer renamer( + platform, GetProjectScopedContainers(), targetPropertiesContainer, + parameterMetadata.GetValueTypeMetadata().IsVariable(), + oldToNewPropertyNames, removedPropertyNames); node->Visit(renamer); if (renamer.IsRemovedPropertyUsed()) { @@ -246,20 +249,16 @@ bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction, bool EventsPropertyReplacer::DoVisitEventExpression( gd::Expression& expression, const gd::ParameterMetadata& metadata) { - const gd::String& type = metadata.GetType(); - - if (!gd::ParameterMetadata::IsExpression("variable", type) && - !gd::ParameterMetadata::IsExpression("number", type) && - !gd::ParameterMetadata::IsExpression("string", type)) - return false; // Not an expression that can contain properties. - + if (!gd::EventsPropertyReplacer::CanContainProperty( + metadata.GetValueTypeMetadata())) { + return false; + } auto node = expression.GetRootNode(); if (node) { - ExpressionPropertyReplacer renamer(platform, - GetProjectScopedContainers(), - targetPropertiesContainer, - oldToNewPropertyNames, - removedPropertyNames); + ExpressionPropertyReplacer renamer( + platform, GetProjectScopedContainers(), targetPropertiesContainer, + metadata.GetValueTypeMetadata().IsVariable(), oldToNewPropertyNames, + removedPropertyNames); node->Visit(renamer); if (renamer.IsRemovedPropertyUsed()) { @@ -272,6 +271,12 @@ bool EventsPropertyReplacer::DoVisitEventExpression( return false; } +bool EventsPropertyReplacer::CanContainProperty( + const gd::ValueTypeMetadata &valueTypeMetadata) { + return valueTypeMetadata.IsVariable() || valueTypeMetadata.IsNumber() || + valueTypeMetadata.IsString(); +} + EventsPropertyReplacer::~EventsPropertyReplacer() {} } // namespace gd diff --git a/Core/GDCore/IDE/Events/EventsPropertyReplacer.h b/Core/GDCore/IDE/Events/EventsPropertyReplacer.h index c67b197e8471..4a362b93bd10 100644 --- a/Core/GDCore/IDE/Events/EventsPropertyReplacer.h +++ b/Core/GDCore/IDE/Events/EventsPropertyReplacer.h @@ -41,6 +41,8 @@ class GD_CORE_API EventsPropertyReplacer removedPropertyNames(removedPropertyNames_){}; virtual ~EventsPropertyReplacer(); + static bool CanContainProperty(const gd::ValueTypeMetadata &valueTypeMetadata); + private: bool DoVisitInstruction(gd::Instruction &instruction, bool isCondition) override; diff --git a/Core/tests/DummyPlatform.cpp b/Core/tests/DummyPlatform.cpp index 190c61e5fab0..f11d34801a05 100644 --- a/Core/tests/DummyPlatform.cpp +++ b/Core/tests/DummyPlatform.cpp @@ -265,9 +265,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project, extension ->AddAction("SetNumberVariable", - "Do something with number variables", - "This does something with variables", - "Do something with variables", + "Change variable value", + "Modify the number value of a variable.", + "the variable _PARAM0_", "", "", "") @@ -278,9 +278,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project, extension ->AddAction("SetStringVariable", - "Do something with string variables", - "This does something with variables", - "Do something with variables", + "Change text variable", + "Modify the text (string) of a variable.", + "the variable _PARAM0_", "", "", "") @@ -291,9 +291,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project, extension ->AddAction("SetBooleanVariable", - "Do something with boolean variables", - "This does something with variables", - "Do something with variables", + "Change boolean variable", + "Modify the boolean value of a variable.", + "Change the variable _PARAM0_: _PARAM1_", "", "", "") @@ -358,6 +358,17 @@ void SetupProjectWithDummyPlatform(gd::Project& project, .AddParameter("soundfile", "Parameter 3 (an audio resource)") .SetFunctionName("doSomethingWithResources"); + extension + ->AddAction("DoSomethingWithAnyVariable", + "Do something with variables", + "This does something with variables", + "Do something with variables please", + "", + "", + "") + .AddParameter("variable", "Any variable") + .SetFunctionName("doSomethingWithAnyVariable"); + extension ->AddAction("DoSomethingWithLegacyPreScopedVariables", "Do something with variables", diff --git a/Core/tests/WholeProjectRefactorer.cpp b/Core/tests/WholeProjectRefactorer.cpp index 75d0627d2f6e..5d6fba9985d7 100644 --- a/Core/tests/WholeProjectRefactorer.cpp +++ b/Core/tests/WholeProjectRefactorer.cpp @@ -91,6 +91,20 @@ CreateInstructionWithNumberParameter(gd::Project &project, return event.GetActions().Insert(instruction); } +const gd::Instruction & +CreateInstructionWithVariableParameter(gd::Project &project, + gd::EventsList &events, + const gd::String &expression) { + gd::StandardEvent &event = dynamic_cast( + events.InsertNewEvent(project, "BuiltinCommonInstructions::Standard")); + + gd::Instruction instruction; + instruction.SetType("MyExtension::DoSomethingWithAnyVariable"); + instruction.SetParametersCount(1); + instruction.SetParameter(0, expression); + return event.GetActions().Insert(instruction); +} + enum TestEvent { FreeFunctionAction, FreeFunctionWithExpression, @@ -3067,6 +3081,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { eventsExtension, eventsBasedBehavior); auto &instruction = CreateInstructionWithNumberParameter( project, behaviorAction.GetEvents(), "MyProperty"); + auto &instruction2 = CreateInstructionWithNumberParameter( + project, behaviorAction.GetEvents(), + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyProperty])"); gd::WholeProjectRefactorer::RenameEventsBasedBehaviorProperty( project, eventsExtension, eventsBasedBehavior, "MyProperty", @@ -3074,6 +3091,39 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { REQUIRE(instruction.GetParameter(0).GetPlainString() == "MyRenamedProperty"); + REQUIRE(instruction2.GetParameter(0).GetPlainString() == + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedProperty])"); + } + + SECTION("(Events based behavior) property not renamed (in variable parameter)") { + gd::Project project; + gd::Platform platform; + SetupProjectWithDummyPlatform(project, platform); + auto &eventsExtension = SetupProjectWithEventsFunctionExtension(project); + auto &eventsBasedBehavior = + eventsExtension.GetEventsBasedBehaviors().Get("MyEventsBasedBehavior"); + + auto &behaviorAction = + eventsBasedBehavior.GetEventsFunctions().InsertNewEventsFunction( + "MyBehaviorEventsFunction", 0); + gd::WholeProjectRefactorer::EnsureBehaviorEventsFunctionsProperParameters( + eventsExtension, eventsBasedBehavior); + // Properties can't actually be used in "variable" parameters. + auto &instruction = CreateInstructionWithVariableParameter( + project, behaviorAction.GetEvents(), "MyProperty"); + auto &instruction2 = CreateInstructionWithNumberParameter( + project, behaviorAction.GetEvents(), + "MyExtension::GetVariableAsNumber(MyProperty)"); + + gd::WholeProjectRefactorer::RenameEventsBasedBehaviorProperty( + project, eventsExtension, eventsBasedBehavior, "MyProperty", + "MyRenamedProperty"); + + // "variable" parameters are left untouched. + REQUIRE(instruction.GetParameter(0).GetPlainString() == + "MyProperty"); + REQUIRE(instruction2.GetParameter(0).GetPlainString() == + "MyExtension::GetVariableAsNumber(MyProperty)"); } SECTION("(Events based behavior) shared property renamed") { @@ -3142,6 +3192,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { eventsExtension, eventsBasedBehavior); auto &instruction = CreateInstructionWithNumberParameter( project, behaviorAction.GetEvents(), "MySharedProperty"); + auto &instruction2 = CreateInstructionWithNumberParameter( + project, behaviorAction.GetEvents(), + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MySharedProperty])"); gd::WholeProjectRefactorer::RenameEventsBasedBehaviorSharedProperty( project, eventsExtension, eventsBasedBehavior, "MySharedProperty", @@ -3149,6 +3202,8 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { REQUIRE(instruction.GetParameter(0).GetPlainString() == "MyRenamedSharedProperty"); + REQUIRE(instruction2.GetParameter(0).GetPlainString() == + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedSharedProperty])"); } SECTION("(Events based object) property renamed") { @@ -3196,6 +3251,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { eventsExtension, eventsBasedObject); auto &instruction = CreateInstructionWithNumberParameter( project, behaviorAction.GetEvents(), "MyProperty"); + auto &instruction2 = CreateInstructionWithNumberParameter( + project, behaviorAction.GetEvents(), + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyProperty])"); gd::WholeProjectRefactorer::RenameEventsBasedObjectProperty( project, eventsExtension, eventsBasedObject, "MyProperty", @@ -3203,6 +3261,39 @@ TEST_CASE("WholeProjectRefactorer", "[common]") { REQUIRE(instruction.GetParameter(0).GetPlainString() == "MyRenamedProperty"); + REQUIRE(instruction2.GetParameter(0).GetPlainString() == + "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedProperty])"); + } + + SECTION("(Events based object) property not renamed (in variable parameter)") { + gd::Project project; + gd::Platform platform; + SetupProjectWithDummyPlatform(project, platform); + auto &eventsExtension = SetupProjectWithEventsFunctionExtension(project); + auto &eventsBasedObject = + eventsExtension.GetEventsBasedObjects().Get("MyEventsBasedObject"); + + auto &behaviorAction = + eventsBasedObject.GetEventsFunctions().InsertNewEventsFunction( + "MyObjectEventsFunction", 0); + gd::WholeProjectRefactorer::EnsureObjectEventsFunctionsProperParameters( + eventsExtension, eventsBasedObject); + // Properties can't actually be used in "variable" parameters. + auto &instruction = CreateInstructionWithVariableParameter( + project, behaviorAction.GetEvents(), "MyProperty"); + auto &instruction2 = CreateInstructionWithNumberParameter( + project, behaviorAction.GetEvents(), + "MyExtension::GetVariableAsNumber(MyProperty)"); + + gd::WholeProjectRefactorer::RenameEventsBasedObjectProperty( + project, eventsExtension, eventsBasedObject, "MyProperty", + "MyRenamedProperty"); + + // "variable" parameters are left untouched. + REQUIRE(instruction.GetParameter(0).GetPlainString() == + "MyProperty"); + REQUIRE(instruction2.GetParameter(0).GetPlainString() == + "MyExtension::GetVariableAsNumber(MyProperty)"); } } // TODO: Check that this works when behaviors are attached to a child-object.