diff --git a/Core/GDCore/IDE/Events/ExpressionValidator.h b/Core/GDCore/IDE/Events/ExpressionValidator.h index 583159fd9335..07738e926bdb 100644 --- a/Core/GDCore/IDE/Events/ExpressionValidator.h +++ b/Core/GDCore/IDE/Events/ExpressionValidator.h @@ -191,6 +191,14 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker { RaiseTypeError( _("You entered a number, but a text was expected (in quotes)."), node.location); + } else if (parentType == Type::Variable || + parentType == Type::ObjectVariable || + parentType == Type::LegacyVariable) { + RaiseTypeError( + _("The variable name looks like you're building an expression or a " + "formula. You can only use this for structure or arrays, for " + "example: Score[3]."), + node.location); } else if (parentType != Type::Number && parentType != Type::NumberOrString) { RaiseTypeError(_("You entered a number, but this type was expected:") + @@ -204,6 +212,14 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker { if (parentType == Type::Number) { RaiseTypeError(_("You entered a text, but a number was expected."), node.location); + } else if (parentType == Type::Variable || + parentType == Type::ObjectVariable || + parentType == Type::LegacyVariable) { + RaiseTypeError( + _("The variable name looks like you're building an expression or a " + "formula. You can only use this for structure or arrays, for " + "example: Score[\"Player1\"]."), + node.location); } else if (parentType != Type::String && parentType != Type::NumberOrString) { RaiseTypeError(_("You entered a text, but this type was expected:") + diff --git a/Core/GDCore/IDE/InstructionValidator.cpp b/Core/GDCore/IDE/InstructionValidator.cpp index 07251afd550e..6c7c964848a5 100644 --- a/Core/GDCore/IDE/InstructionValidator.cpp +++ b/Core/GDCore/IDE/InstructionValidator.cpp @@ -66,27 +66,8 @@ ParameterValidationResult InstructionValidator::ValidateParameter( gd::ParameterMetadata::IsExpression("string", parameterType) || gd::ParameterMetadata::IsExpression("variable", parameterType)) { - // New object variable instructions require the variable to be - // declared while legacy ones don't. - // For legacy variable instruction, we pass an empty object name. - gd::String rootObjectName = ""; - if (parameterType == "objectvar") { - const auto &objectsContainersList = - projectScopedContainers.GetObjectsContainersList(); - rootObjectName = instruction.GetParameter(0).GetPlainString(); - - if (!gd::VariableInstructionSwitcher::IsSwitchableVariableInstruction( - instruction.GetType())) { - // Extensions still rely on legacy object variables instructions. - auto objectSourceType = - projectScopedContainers.GetObjectsContainersList() - .GetObjectsContainerSourceType(rootObjectName); - // Only child-object variable declarations are checked. - if (objectSourceType != gd::ObjectsContainer::SourceType::Object) { - rootObjectName = ""; - } - } - } + gd::String rootObjectName = InstructionValidator::GetObjectNameForParameter( + projectScopedContainers, instruction, parameterType); auto &expressionNode = *instruction.GetParameter(parameterIndex).GetRootNode(); ExpressionValidator expressionValidator(platform, projectScopedContainers, @@ -123,6 +104,35 @@ ParameterValidationResult InstructionValidator::ValidateParameter( return result; } +gd::String InstructionValidator::GetObjectNameForParameter( + const gd::ProjectScopedContainers projectScopedContainers, + const gd::Instruction &instruction, const gd::String ¶meterType) { + // New object variable instructions require the variable to be + // declared while legacy ones don't. + // For legacy variable instruction, we pass an empty object name. + gd::String rootObjectName = ""; + if (parameterType != "objectvar") { + return ""; + } + if (instruction.GetParametersCount() == 0) { + return ""; + } + rootObjectName = instruction.GetParameter(0).GetPlainString(); + + if (!gd::VariableInstructionSwitcher::IsSwitchableVariableInstruction( + instruction.GetType())) { + // Extensions still rely on legacy object variables instructions. + auto objectSourceType = + projectScopedContainers.GetObjectsContainersList() + .GetObjectsContainerSourceType(rootObjectName); + // Only child-object variable declarations are checked. + if (objectSourceType != gd::ObjectsContainer::SourceType::Object) { + return ""; + } + } + return rootObjectName; +} + bool InstructionValidator::IsParameterValid( const gd::Platform &platform, const gd::ProjectScopedContainers projectScopedContainers, diff --git a/Core/GDCore/IDE/InstructionValidator.h b/Core/GDCore/IDE/InstructionValidator.h index e2b8457ef232..4955a33b2977 100644 --- a/Core/GDCore/IDE/InstructionValidator.h +++ b/Core/GDCore/IDE/InstructionValidator.h @@ -63,6 +63,10 @@ class GD_CORE_API InstructionValidator { static gd::String GetRootVariableName(const gd::String &name); + static gd::String GetObjectNameForParameter( + const gd::ProjectScopedContainers projectScopedContainers, + const gd::Instruction &instruction, const gd::String ¶meterType); + private: static bool HasRequiredBehaviors(const gd::Instruction &instruction, diff --git a/Core/tests/ExpressionParser2.cpp b/Core/tests/ExpressionParser2.cpp index 70c234e2c387..b7d558452f5e 100644 --- a/Core/tests/ExpressionParser2.cpp +++ b/Core/tests/ExpressionParser2.cpp @@ -4033,7 +4033,9 @@ TEST_CASE("ExpressionParser2", "[common][events]") { node->Visit(validator); RequireFatalErrorsCount(validator, 1); REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == - "You entered a number, but this type was expected: variable"); + "The variable name looks like you're building an expression or a " + "formula. You can only use this for structure or arrays, for " + "example: Score[3]."); } SECTION("string instead") { auto node = parser.ParseExpression("\"text\""); @@ -4043,7 +4045,9 @@ TEST_CASE("ExpressionParser2", "[common][events]") { node->Visit(validator); RequireFatalErrorsCount(validator, 1); REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == - "You entered a text, but this type was expected: variable"); + "The variable name looks like you're building an expression or a " + "formula. You can only use this for structure or arrays, for " + "example: Score[\"Player1\"]."); } SECTION("Object variable with unary operator") { diff --git a/GDevelop.js/Bindings/Bindings.idl b/GDevelop.js/Bindings/Bindings.idl index ce2195fc4966..3d4908504079 100644 --- a/GDevelop.js/Bindings/Bindings.idl +++ b/GDevelop.js/Bindings/Bindings.idl @@ -3128,6 +3128,10 @@ interface InstructionValidator { [Const, Ref] Instruction instruction, [Const, Ref] InstructionMetadata metadata, long parameterIndex); + [Const, Value] DOMString STATIC_GetObjectNameForParameter( + [Const, Ref] ProjectScopedContainers projectScopedContainers, + [Const, Ref] Instruction instruction, + [Const] DOMString parameterType); }; interface ObjectTools { diff --git a/GDevelop.js/Bindings/Wrapper.cpp b/GDevelop.js/Bindings/Wrapper.cpp index b5c05a6f5061..9b4b33b42b4b 100644 --- a/GDevelop.js/Bindings/Wrapper.cpp +++ b/GDevelop.js/Bindings/Wrapper.cpp @@ -746,6 +746,7 @@ typedef std::vector VectorPropertyDescriptorChoice #define STATIC_FillBehaviorParameters FillBehaviorParameters #define STATIC_ValidateParameter ValidateParameter #define STATIC_IsParameterValid IsParameterValid +#define STATIC_GetObjectNameForParameter GetObjectNameForParameter #define STATIC_FixInvalidRequiredBehaviorProperties \ FixInvalidRequiredBehaviorProperties #define STATIC_RemoveLayerInScene RemoveLayerInScene diff --git a/GDevelop.js/types.d.ts b/GDevelop.js/types.d.ts index d55b282fc3b9..6124894de6a9 100644 --- a/GDevelop.js/types.d.ts +++ b/GDevelop.js/types.d.ts @@ -2252,6 +2252,7 @@ export class ParameterValidationResult extends EmscriptenObject { export class InstructionValidator extends EmscriptenObject { static validateParameter(platform: Platform, projectScopedContainers: ProjectScopedContainers, instruction: Instruction, metadata: InstructionMetadata, parameterIndex: number): ParameterValidationResult; static isParameterValid(platform: Platform, projectScopedContainers: ProjectScopedContainers, instruction: Instruction, metadata: InstructionMetadata, parameterIndex: number): boolean; + static getObjectNameForParameter(projectScopedContainers: ProjectScopedContainers, instruction: Instruction, parameterType: string): string; } export class ObjectTools extends EmscriptenObject { diff --git a/GDevelop.js/types/gdinstructionvalidator.js b/GDevelop.js/types/gdinstructionvalidator.js index b9c79fee8998..c7e884564952 100644 --- a/GDevelop.js/types/gdinstructionvalidator.js +++ b/GDevelop.js/types/gdinstructionvalidator.js @@ -2,6 +2,7 @@ declare class gdInstructionValidator { static validateParameter(platform: gdPlatform, projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, metadata: gdInstructionMetadata, parameterIndex: number): gdParameterValidationResult; static isParameterValid(platform: gdPlatform, projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, metadata: gdInstructionMetadata, parameterIndex: number): boolean; + static getObjectNameForParameter(projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, parameterType: string): string; delete(): void; ptr: number; }; \ No newline at end of file diff --git a/newIDE/app/src/EventsSheet/ParameterFields/AnyVariableField.js b/newIDE/app/src/EventsSheet/ParameterFields/AnyVariableField.js index c8f31e03e27c..1b996add7d07 100644 --- a/newIDE/app/src/EventsSheet/ParameterFields/AnyVariableField.js +++ b/newIDE/app/src/EventsSheet/ParameterFields/AnyVariableField.js @@ -103,10 +103,8 @@ export default (React.forwardRef( return ( ( return ( ( return ( { expressionType, parameterMetadata, expressionNode, + '', showDeprecatedInstructionWarning ); const extraErrorText = onExtractAdditionalErrors diff --git a/newIDE/app/src/EventsSheet/ParameterFields/GlobalVariableField.js b/newIDE/app/src/EventsSheet/ParameterFields/GlobalVariableField.js index cc9f0915c28c..7a552116cb6e 100644 --- a/newIDE/app/src/EventsSheet/ParameterFields/GlobalVariableField.js +++ b/newIDE/app/src/EventsSheet/ParameterFields/GlobalVariableField.js @@ -49,7 +49,6 @@ export default (React.forwardRef( return ( ( return ( ( return ( , getVariableSourceFromIdentifier: ( identifier: string, projectScopedContainers: gdProjectScopedContainers ) => VariablesContainer_SourceType, enumerateVariables: () => Array, - forceDeclaration?: boolean, openVariableEditorDialog: (VariableDialogOpeningProps => void) | null, editEventsFunctionParameter: (VariableDialogOpeningProps => void) | null, openEventsBasedEntityPropertyEditorDialog: @@ -84,26 +81,11 @@ type Props = { | null, }; -type VariableNameQuickAnalyzeResult = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7; - export type VariableFieldInterface = {| ...ParameterFieldInterface, updateAutocompletions: () => void, |}; -export const VariableNameQuickAnalyzeResults: { - [string]: VariableNameQuickAnalyzeResult, -} = { - OK: 0, - WRONG_QUOTE: 1, - WRONG_SPACE: 2, - WRONG_EXPRESSION: 3, - UNDECLARED_VARIABLE: 4, - NAME_COLLISION_WITH_OBJECT: 5, - PARAMETER_WITH_CHILD: 6, - PROPERTY_WITH_CHILD: 7, -}; - export const getRootVariableName = (name: string): string => { const dotPosition = name.indexOf('.'); const squareBracketPosition = name.indexOf('['); @@ -130,83 +112,6 @@ const isRootVariableDeclared = ( ); }; -// TODO: the entire VariableField could be reworked to be a "real" GenericExpressionField -// (of type: "variable" or the legacy: "scenevar", "globalvar" or "objectvar"). This will -// ensure we 100% validate and can autocomplete what is entered (and we can have also a simpler -// selector that offers the variables in the scope). -export const quicklyAnalyzeVariableName = ( - name: string, - variablesContainers?: Array, - getVariableSourceFromIdentifier?: ( - identifier: string, - projectScopedContainers: gdProjectScopedContainers - ) => VariablesContainer_SourceType | null, - projectScopedContainersAccessor?: ProjectScopedContainersAccessor, - isObjectVariable: boolean = false -): VariableNameQuickAnalyzeResult => { - if (!name) return VariableNameQuickAnalyzeResults.OK; - - for (let i = 0; i < name.length; ++i) { - const character = name[i]; - - if (character === '[') { - // This probably starts an expression, so stop the analysis. - break; - } else if (character === ' ') { - return VariableNameQuickAnalyzeResults.WRONG_SPACE; - } else if (character === '"') { - return VariableNameQuickAnalyzeResults.WRONG_QUOTE; - } else if ( - character === '(' || - character === '+' || - character === '-' || - character === '/' || - character === '*' - ) { - return VariableNameQuickAnalyzeResults.WRONG_EXPRESSION; - } - } - - const rootVariableName = getRootVariableName(name); - // Check at least the name of the root variable, it's the best we can do. - if (!isRootVariableDeclared(rootVariableName, variablesContainers)) { - return VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE; - } - - if (!projectScopedContainersAccessor) { - return VariableNameQuickAnalyzeResults.OK; - } - const projectScopedContainers = projectScopedContainersAccessor.get(); - - if ( - !isObjectVariable && - projectScopedContainers - .getObjectsContainersList() - .hasObjectOrGroupNamed(rootVariableName) - ) { - return VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT; - } - - if ( - name.length !== rootVariableName.length && - getVariableSourceFromIdentifier - ) { - const variableSource = getVariableSourceFromIdentifier( - rootVariableName, - projectScopedContainers - ); - - if (variableSource === gd.VariablesContainer.Parameters) { - return VariableNameQuickAnalyzeResults.PARAMETER_WITH_CHILD; - } - if (variableSource === gd.VariablesContainer.Properties) { - return VariableNameQuickAnalyzeResults.PROPERTY_WITH_CHILD; - } - } - - return VariableNameQuickAnalyzeResults.OK; -}; - export const getVariableSourceIcon = ( variableSourceType: VariablesContainer_SourceType ): any => { @@ -247,6 +152,11 @@ export const getVariableTypeIcon = (variableType: Variable_Type): any => { } }; +// TODO: the entire VariableField could be reworked to be a "real" GenericExpressionField +// (of type: "variable" or the legacy: "scenevar", "globalvar" or "objectvar"). This will +// ensure we 100% validate and can autocomplete what is entered (and we can have also a simpler +// selector that offers the variables in the scope). + export default (React.forwardRef( function VariableField(props: Props, ref) { const { @@ -255,7 +165,6 @@ export default (React.forwardRef( variablesContainers, enumerateVariables, instruction, - forceDeclaration, value, onChange, isInline, @@ -264,7 +173,6 @@ export default (React.forwardRef( onApply, id, onInstructionTypeChanged, - isObjectVariable, getVariableSourceFromIdentifier, openVariableEditorDialog, editEventsFunctionParameter, @@ -436,68 +344,78 @@ export default (React.forwardRef( ? parameterMetadata.getDescription() : undefined; - const quicklyAnalysisResult = quicklyAnalyzeVariableName( - value, - variablesContainers, - getVariableSourceFromIdentifier, - projectScopedContainersAccessor, - isObjectVariable + const [errorText, setErrorText] = React.useState(null); + const inputValue = React.useRef(null); + const doValidation = React.useCallback( + () => { + if (!project || !parameterMetadata || !instruction) return null; + + // Parsing can be time consuming (~1ms for simple expression, + // a few milliseconds for complex ones). + + const parser = new gd.ExpressionParser2(); + const expressionNode = parser + .parseExpression(inputValue.current || value) + .get(); + const expressionType = parameterMetadata + .getValueTypeMetadata() + .getName(); + + const objectName = gd.InstructionValidator.getObjectNameForParameter( + projectScopedContainersAccessor.get(), + instruction, + expressionType + ); + const { errorText } = extractErrors( + gd.JsPlatform.get(), + project, + projectScopedContainersAccessor, + expressionType, + parameterMetadata, + expressionNode, + objectName, + 'no' + ); + + parser.delete(); + + setErrorText(errorText); + }, + [ + instruction, + parameterMetadata, + project, + projectScopedContainersAccessor, + value, + ] ); - const errorText = - quicklyAnalysisResult === VariableNameQuickAnalyzeResults.WRONG_QUOTE ? ( - - It seems you entered a name with a quote. Variable names should not be - quoted. - - ) : quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.WRONG_SPACE ? ( - - The variable name contains a space - this is not recommended. Prefer - to use underscores or uppercase letters to separate words. - - ) : quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.WRONG_EXPRESSION ? ( - - The variable name looks like you're building an expression or a - formula. You can only use this for structure or arrays. For example: - Score[3]. - - ) : forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE ? ( - - This variable does not exist.{' '} - - Click to add it. - - - ) : forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT ? ( - - This variable has the same name as an object. Consider renaming one or - the other. - - ) : forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.PARAMETER_WITH_CHILD ? ( - Parameters can't have children. - ) : forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.PROPERTY_WITH_CHILD ? ( - Properties can't have children. - ) : null; - const warningTranslatableText = - !forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE - ? t`This variable is not declared. It's recommended to use the *variables editor* to add it.` - : !forceDeclaration && - quicklyAnalysisResult === - VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT - ? t`This variable has the same name as an object. Consider renaming one or the other.` - : null; + const enqueueValidation = useDebounce(() => { + doValidation(); + }, 250); + + React.useEffect( + () => { + enqueueValidation(); + }, + [enqueueValidation] + ); + + const handleValueChange = React.useCallback( + (value: string) => { + inputValue.current = null; + onChange(value); + }, + [onChange] + ); + + const handleInputValueChange = React.useCallback( + (value: string) => { + inputValue.current = value; + enqueueValidation(); + }, + [enqueueValidation] + ); const isSwitchableInstruction = instruction && @@ -561,16 +479,15 @@ export default (React.forwardRef( margin={isInline ? 'none' : 'dense'} floatingLabelText={description} helperMarkdownText={ - warningTranslatableText - ? i18n._(warningTranslatableText) - : parameterMetadata + parameterMetadata ? parameterMetadata.getLongDescription() : undefined } errorText={errorText} fullWidth value={value} - onChange={onChange} + onChange={handleValueChange} + onInputValueChange={handleInputValueChange} onRequestClose={onRequestClose} onApply={onApply} filterOptionById={filterOptionById} @@ -763,9 +680,6 @@ export const renderVariableWithIcon = ( title={tooltip} className={classNames({ [nameAndIconContainer]: true, - [instructionWarningParameter]: - quicklyAnalyzeVariableName(value) !== - VariableNameQuickAnalyzeResults.OK, })} > diff --git a/newIDE/app/src/EventsSheet/ParameterFields/VariableField.spec.js b/newIDE/app/src/EventsSheet/ParameterFields/VariableField.spec.js deleted file mode 100644 index 6e4bca2eab3a..000000000000 --- a/newIDE/app/src/EventsSheet/ParameterFields/VariableField.spec.js +++ /dev/null @@ -1,52 +0,0 @@ -// @flow -import { - quicklyAnalyzeVariableName, - VariableNameQuickAnalyzeResults, -} from './VariableField'; - -describe('VariableField', () => { - it('can quickly analyze if a variable name or expression looks good', () => { - expect(quicklyAnalyzeVariableName('Test')).toBe( - VariableNameQuickAnalyzeResults.OK - ); - expect(quicklyAnalyzeVariableName('Test123')).toBe( - VariableNameQuickAnalyzeResults.OK - ); - expect(quicklyAnalyzeVariableName('Hello world')).toBe( - VariableNameQuickAnalyzeResults.WRONG_SPACE - ); - expect(quicklyAnalyzeVariableName(' "Test"')).toBe( - VariableNameQuickAnalyzeResults.WRONG_SPACE - ); - expect(quicklyAnalyzeVariableName('"Test"')).toBe( - VariableNameQuickAnalyzeResults.WRONG_QUOTE - ); - expect(quicklyAnalyzeVariableName('VariableString(MySubVariable)')).toBe( - VariableNameQuickAnalyzeResults.WRONG_EXPRESSION - ); - expect(quicklyAnalyzeVariableName('Test+2')).toBe( - VariableNameQuickAnalyzeResults.WRONG_EXPRESSION - ); - expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable')).toBe( - VariableNameQuickAnalyzeResults.OK - ); - expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable["Test"]')).toBe( - VariableNameQuickAnalyzeResults.OK - ); - expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable[1 + 2]')).toBe( - VariableNameQuickAnalyzeResults.OK - ); - expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable+2')).toBe( - VariableNameQuickAnalyzeResults.WRONG_EXPRESSION - ); - expect( - quicklyAnalyzeVariableName('MyVariable[VariableString(AnotherVariable)]') - ).toBe(VariableNameQuickAnalyzeResults.OK); - expect( - quicklyAnalyzeVariableName('MyVariable[Variable(AnotherVariable)]') - ).toBe(VariableNameQuickAnalyzeResults.OK); - expect( - quicklyAnalyzeVariableName('MyVariable[Variable(AnotherVariable) + 2]') - ).toBe(VariableNameQuickAnalyzeResults.OK); - }); -});