Fix correctly flagging non-existing behaviors for free behaviors property fields (#9203)

Core/GDCore/IDE/InstructionValidator.cpp

@@ -10,6 +10,7 @@
 #include "GDCore/Extensions/Metadata/BehaviorMetadata.h"
 #include "GDCore/Extensions/Metadata/InstructionMetadata.h"
 #include "GDCore/Extensions/Metadata/MetadataProvider.h"
+#include "GDCore/Extensions/Metadata/ParameterMetadataTools.h"
 #include "GDCore/Extensions/Platform.h"
 #include "GDCore/IDE/Events/ExpressionValidator.h"
 #include "GDCore/IDE/VariableInstructionSwitcher.h"
@@ -94,6 +95,29 @@ ParameterValidationResult InstructionValidator::ValidateParameter(
              parameterMetadata.GetExtraInfo()) &&
         InstructionValidator::HasRequiredBehaviors(
             instruction, metadata, parameterIndex, objectsContainersList);
+  } else if (gd::ParameterMetadata::IsBehavior(parameterType)) {
+    const auto objectParameterIndex =
+        gd::ParameterMetadataTools::GetObjectParameterIndexFor(
+            metadata.GetParameters(), parameterIndex);
+    if (objectParameterIndex == gd::String::npos) {
+      return result;  // No object to check against: consider it valid.
+    }
+    const auto &objectOrGroupName =
+        instruction.GetParameter(objectParameterIndex).GetPlainString();
+    const auto &objectsContainersList =
+        projectScopedContainers.GetObjectsContainersList();
+    const auto &behaviorType = parameterMetadata.GetExtraInfo();
+    // The object must have a behavior with this name and, when the parameter
+    // declares a behavior type (most behavior instructions), of this type.
+    // Untyped behavior parameters (like "De/activate a behavior") accept any
+    // behavior of the object.
+    result.isValid =
+        behaviorType.empty()
+            ? objectsContainersList.HasBehaviorInObjectOrGroup(
+                  objectOrGroupName, value)
+            : objectsContainersList.GetTypeOfBehaviorInObjectOrGroup(
+                  objectOrGroupName, value,
+                  /** searchInGroups = */ true) == behaviorType;
   } else if (gd::ParameterMetadata::IsExpression("resource", parameterType)) {
     const auto &resourceName =
         instruction.GetParameter(parameterIndex).GetPlainString();
@@ -191,6 +215,7 @@ bool InstructionValidator::HasRequiredBehaviors(
     }
     const auto &behaviorType = behaviorParameter.GetExtraInfo();
     if (behaviorType.empty()) {
+      // The behavior name is checked by the behavior parameter itself.
       continue;
     }
     if (index >= instruction.GetParametersCount()) {

GDevelop.js/__tests__/Core.js

@@ -2822,6 +2822,73 @@ describe('libGD.js', function () {
       expect(validateVolumeParameter('1 +')).toBe(false);
       expect(validateVolumeParameter('"Not a number"')).toBe(false);
     });
+
+    it('checks that the object has the behavior of an untyped behavior parameter', function () {
+      // `ActivateBehavior` accepts any behavior: its "behavior" parameter has
+      // no behavior type, but the object must still have the behavior.
+      const objectWithBehavior = layout
+        .getObjects()
+        .insertNewObject(project, 'Sprite', 'ObjectWithBehavior', 0);
+      objectWithBehavior.addNewBehavior(
+        project,
+        'PlatformBehavior::PlatformerObjectBehavior',
+        'PlatformerObject'
+      );
+      layout
+        .getObjects()
+        .insertNewObject(project, 'Sprite', 'ObjectWithoutBehavior', 1);
+
+      const validateParameter = (objectName, behaviorName, parameterIndex) => {
+        const action = new gd.Instruction();
+        action.setType('ActivateBehavior');
+        action.setParametersCount(3);
+        action.setParameter(0, objectName);
+        action.setParameter(1, behaviorName);
+        action.setParameter(2, 'yes');
+        const result = gd.InstructionValidator.validateParameter(
+          gd.JsPlatform.get(),
+          projectScopedContainers,
+          action,
+          gd.MetadataProvider.getActionMetadata(
+            gd.JsPlatform.get(),
+            'ActivateBehavior'
+          ),
+          parameterIndex
+        );
+        const isValid = result.isValid();
+        action.delete();
+        return isValid;
+      };
+      const behaviorParameterIndex = 1;
+      const objectParameterIndex = 0;
+
+      expect(
+        validateParameter(
+          'ObjectWithBehavior',
+          'PlatformerObject',
+          behaviorParameterIndex
+        )
+      ).toBe(true);
+      expect(
+        validateParameter(
+          'ObjectWithoutBehavior',
+          'PlatformerObject',
+          behaviorParameterIndex
+        )
+      ).toBe(false);
+      expect(
+        validateParameter('ObjectWithBehavior', '', behaviorParameterIndex)
+      ).toBe(false);
+
+      // The error is on the behavior parameter: the object stays valid.
+      expect(
+        validateParameter(
+          'ObjectWithoutBehavior',
+          'PlatformerObject',
+          objectParameterIndex
+        )
+      ).toBe(true);
+    });
   });
 
   describe('EventsRefactorer', function () {

newIDE/app/src/EventsSheet/ParameterFields/BehaviorField.js

@@ -54,9 +54,6 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
 
     const { parameterMetadata } = props;
 
-    const [errorText, setErrorText] = React.useState<?string>(null);
-    const [behaviorNames, setBehaviorNames] = React.useState<Array<string>>([]);
-
     const description = parameterMetadata
       ? parameterMetadata.getDescription()
       : undefined;
@@ -69,58 +66,54 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
       ? parameterMetadata.getExtraInfo()
       : '';
 
-    const updateBehaviorsList = React.useCallback(
-      () => {
-        const {
-          instructionMetadata,
-          instruction,
-          expressionMetadata,
-          expression,
-          parameterIndex,
-          projectScopedContainersAccessor,
-        } = props;
-        const objectName = getLastObjectParameterValue({
-          instructionMetadata,
-          instruction,
-          expressionMetadata,
-          expression,
-          parameterIndex,
-        });
-        if (!objectName) return;
+    // Computed synchronously (and not in an effect) so that the first render
+    // already knows the behaviors of the object: otherwise the field would
+    // briefly display an error before the list is filled.
+    const objectName = getLastObjectParameterValue({
+      instructionMetadata: props.instructionMetadata,
+      instruction: props.instruction,
+      expressionMetadata: props.expressionMetadata,
+      expression: props.expression,
+      parameterIndex: props.parameterIndex,
+    });
+    const behaviorNames = React.useMemo(
+      () =>
+        objectName
+          ? getSelectableBehavior(
+              props.projectScopedContainersAccessor,
+              objectName,
+              allowedBehaviorType
+            )
+          : [],
+      // Recompute on each props change: the object (not in the props) may
+      // have been changed in the instruction, or a behavior may have been
+      // added to it (from the instruction editor dialog).
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+      [props, objectName, allowedBehaviorType]
+    );
 
-        const newBehaviorNames = getSelectableBehavior(
-          projectScopedContainersAccessor,
-          objectName,
-          allowedBehaviorType
-        );
-        setBehaviorNames(newBehaviorNames);
+    React.useEffect(
+      () => {
         if (
+          objectName &&
           !allowedBehaviorType &&
           !!props.value &&
-          newBehaviorNames.length === 0
+          behaviorNames.length === 0
         ) {
           // Force emptying the current value if there is no behavior.
           // Useful when the object is changed to one without behaviors.
           props.onChange('');
         }
       },
-      [props, allowedBehaviorType]
+      [props, objectName, allowedBehaviorType, behaviorNames]
     );
 
-    const getError = (value?: string) => {
-      if (!value && !props.value) return null;
-
-      const isValidChoice =
-        behaviorNames.filter(choice => props.value === choice).length !== 0;
-
-      if (!isValidChoice) return 'This behavior is not attached to the object';
-
-      return null;
-    };
-
-    const doValidation = (value?: string) => {
-      setErrorText(getError(value));
-    };
+    // Derived from the current value so that the error is visible as soon as
+    // the field is displayed (and not only after it lost focus).
+    const errorText =
+      !!props.value && !behaviorNames.includes(props.value)
+        ? 'This behavior is not attached to the object'
+        : null;
 
     const forceChooseBehavior = React.useCallback(
       () => {
@@ -145,13 +138,6 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
       [forceChooseBehavior]
     );
 
-    React.useEffect(
-      () => {
-        updateBehaviorsList();
-      },
-      [updateBehaviorsList]
-    );
-
     const noBehaviorErrorText = allowedBehaviorType ? (
       <Trans>
         The behavior is not attached to this object. Please select another
@@ -178,9 +164,6 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
         onChange={props.onChange}
         onRequestClose={props.onRequestClose}
         onApply={props.onApply}
-        onBlur={event => {
-          doValidation(event.currentTarget.value);
-        }}
         dataSource={behaviorNames.map(behaviorName => ({
           text: behaviorName,
           value: behaviorName,