From 89194b2ead9e639867236b17396d1eb3ecd971ed Mon Sep 17 00:00:00 2001 From: Maribeth Moffatt Date: Mon, 7 Apr 2025 17:29:00 -0700 Subject: [PATCH] fix: check potential variables for flyout variable fields (#8873) * fix: check potential variables for flyout variable fields * fix: format * chore: move comment --- core/field_variable.ts | 52 +++++++++++++++------------ core/workspace.ts | 1 - tests/mocha/field_variable_test.js | 57 ++++++++++++++++++++++-------- 3 files changed, 72 insertions(+), 38 deletions(-) diff --git a/core/field_variable.ts b/core/field_variable.ts index ad9037e96..2af3c4d05 100644 --- a/core/field_variable.ts +++ b/core/field_variable.ts @@ -71,7 +71,8 @@ export class FieldVariable extends FieldDropdown { * field's value. Takes in a variable ID & returns a validated variable * ID, or null to abort the change. * @param variableTypes A list of the types of variables to include in the - * dropdown. Will only be used if config is not provided. + * dropdown. Pass `null` to include all types that exist on the + * workspace. Will only be used if config is not provided. * @param defaultType The type of variable to create if this field's value * is not explicitly set. Defaults to ''. Will only be used if config * is not provided. @@ -83,7 +84,7 @@ export class FieldVariable extends FieldDropdown { constructor( varName: string | null | typeof Field.SKIP_SETUP, validator?: FieldVariableValidator, - variableTypes?: string[], + variableTypes?: string[] | null, defaultType?: string, config?: FieldVariableConfig, ) { @@ -423,25 +424,27 @@ export class FieldVariable extends FieldDropdown { * Return a list of variable types to include in the dropdown. * * @returns Array of variable types. - * @throws {Error} if variableTypes is an empty array. */ private getVariableTypes(): string[] { - let variableTypes = this.variableTypes; - if (variableTypes === null) { - // If variableTypes is null, return all variable types. - if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) { - return this.sourceBlock_.workspace.getVariableMap().getTypes(); - } + if (this.variableTypes) return this.variableTypes; + + if (!this.sourceBlock_ || this.sourceBlock_.isDeadOrDying()) { + // We should include all types in the block's workspace, + // but the block is dead so just give up. + return ['']; } - variableTypes = variableTypes || ['']; - if (variableTypes.length === 0) { - // Throw an error if variableTypes is an empty list. - const name = this.getText(); - throw Error( - "'variableTypes' of field variable " + name + ' was an empty list', - ); + + // If variableTypes is null, return all variable types in the workspace. + let allTypes = this.sourceBlock_.workspace.getVariableMap().getTypes(); + if (this.sourceBlock_.isInFlyout) { + // If this block is in a flyout, we also need to check the potential variables + const potentialMap = + this.sourceBlock_.workspace.getPotentialVariableMap(); + if (!potentialMap) return allTypes; + allTypes = Array.from(new Set([...allTypes, ...potentialMap.getTypes()])); } - return variableTypes; + + return allTypes; } /** @@ -455,11 +458,15 @@ export class FieldVariable extends FieldDropdown { * value is not explicitly set. Defaults to ''. */ private setTypes(variableTypes: string[] | null = null, defaultType = '') { - // If you expected that the default type would be the same as the only entry - // in the variable types array, tell the Blockly team by commenting on - // #1499. - // Set the allowable variable types. Null means all types on the workspace. + const name = this.getText(); if (Array.isArray(variableTypes)) { + if (variableTypes.length === 0) { + // Throw an error if variableTypes is an empty list. + throw Error( + `'variableTypes' of field variable ${name} was an empty list. If you want to include all variable types, pass 'null' instead.`, + ); + } + // Make sure the default type is valid. let isInArray = false; for (let i = 0; i < variableTypes.length; i++) { @@ -477,8 +484,7 @@ export class FieldVariable extends FieldDropdown { } } else if (variableTypes !== null) { throw Error( - "'variableTypes' was not an array in the definition of " + - 'a FieldVariable', + `'variableTypes' was not an array or null in the definition of FieldVariable ${name}`, ); } // Only update the field once all checks pass. diff --git a/core/workspace.ts b/core/workspace.ts index 30238b91e..261da0f24 100644 --- a/core/workspace.ts +++ b/core/workspace.ts @@ -854,7 +854,6 @@ export class Workspace implements IASTNodeLocation { * These exist in the flyout but not in the workspace. * * @returns The potential variable map. - * @internal */ getPotentialVariableMap(): IVariableMap< IVariableModel diff --git a/tests/mocha/field_variable_test.js b/tests/mocha/field_variable_test.js index 221305d71..2dc8d35a5 100644 --- a/tests/mocha/field_variable_test.js +++ b/tests/mocha/field_variable_test.js @@ -309,6 +309,24 @@ suite('Variable Fields', function () { assert.deepEqual(field.variableTypes, ['Type1']); assert.equal(field.defaultType, 'Type1'); }); + test('Empty list of types', function () { + assert.throws(function () { + const fieldVariable = new Blockly.FieldVariable( + 'name1', + undefined, + [], + ); + }); + }); + test('invalid value for list of types', function () { + assert.throws(function () { + const fieldVariable = new Blockly.FieldVariable( + 'name1', + undefined, + 'not an array', + ); + }); + }); test('JSON Definition', function () { const field = Blockly.FieldVariable.fromJson({ variable: 'test', @@ -353,13 +371,6 @@ suite('Variable Fields', function () { this.workspace.createVariable('name1', 'type1'); this.workspace.createVariable('name2', 'type2'); }); - test('variableTypes is undefined', function () { - // Expect that since variableTypes is undefined, only type empty string - // will be returned (regardless of what types are available on the workspace). - const fieldVariable = new Blockly.FieldVariable('name1'); - const resultTypes = fieldVariable.getVariableTypes(); - assert.deepEqual(resultTypes, ['']); - }); test('variableTypes is explicit', function () { // Expect that since variableTypes is defined, it will be the return // value, regardless of what types are available on the workspace. @@ -377,6 +388,17 @@ suite('Variable Fields', function () { 'Default type was wrong', ); }); + test('variableTypes is undefined', function () { + // Expect all variable types in the workspace to be returned, same + // as if variableTypes is null. + const fieldVariable = new Blockly.FieldVariable('name1'); + const mockBlock = createTestBlock(); + mockBlock.workspace = this.workspace; + fieldVariable.setSourceBlock(mockBlock); + + const resultTypes = fieldVariable.getVariableTypes(); + assert.deepEqual(resultTypes, ['type1', 'type2']); + }); test('variableTypes is null', function () { // Expect all variable types to be returned. // The field does not need to be initialized to do this--it just needs @@ -390,16 +412,23 @@ suite('Variable Fields', function () { const resultTypes = fieldVariable.getVariableTypes(); assert.deepEqual(resultTypes, ['type1', 'type2']); }); - test('variableTypes is the empty list', function () { - const fieldVariable = new Blockly.FieldVariable('name1'); + test('variableTypes is null and variable is in the flyout', function () { + // Expect all variable types in the workspace and + // flyout workspace to be returned. + const fieldVariable = new Blockly.FieldVariable('name1', undefined, null); const mockBlock = createTestBlock(); mockBlock.workspace = this.workspace; - fieldVariable.setSourceBlock(mockBlock); - fieldVariable.variableTypes = []; - assert.throws(function () { - fieldVariable.getVariableTypes(); - }); + // Pretend this is a flyout workspace with potential variables + mockBlock.isInFlyout = true; + mockBlock.workspace.createPotentialVariableMap(); + mockBlock.workspace + .getPotentialVariableMap() + .createVariable('name3', 'type3'); + fieldVariable.setSourceBlock(mockBlock); + + const resultTypes = fieldVariable.getVariableTypes(); + assert.deepEqual(resultTypes, ['type1', 'type2', 'type3']); }); }); suite('Default types', function () {