diff --git a/src/blocks/mrc_class_method_def.ts b/src/blocks/mrc_class_method_def.ts index b63dbc9a..1b53d93b 100644 --- a/src/blocks/mrc_class_method_def.ts +++ b/src/blocks/mrc_class_method_def.ts @@ -35,6 +35,7 @@ import * as toolboxItems from '../toolbox/items'; import { getClassData } from './utils/python'; import { FunctionData } from './utils/python_json_types'; import { findConnectedBlocksOfType } from './utils/find_connected_blocks'; +import { makeLegalName } from './utils/validator'; import { NONCOPYABLE_BLOCK } from './noncopyable_block'; import { BLOCK_NAME as MRC_GET_PARAMETER_BLOCK_NAME } from './mrc_get_parameter'; import * as paramContainer from './mrc_param_container' @@ -285,10 +286,23 @@ const CLASS_METHOD_DEF = { // When the user changes the method name on the block, clear the mrcPythonMethodName field. this.mrcPythonMethodName = ''; - // Strip leading and trailing whitespace. - name = name.trim(); + if (this.isInFlyout) { + // Flyouts can have multiple methods with identical names. + return name; + } + + const otherNames: string[] = []; + this.workspace.getBlocksByType(BLOCK_NAME) + .filter(block => block.id !== this.id) + .forEach((block) => { + otherNames.push(block.getFieldValue(FIELD_METHOD_NAME)); + const classMethodDefBlock = block as ClassMethodDefBlock; + if (classMethodDefBlock.mrcPythonMethodName) { + otherNames.push(classMethodDefBlock.mrcPythonMethodName); + } + }); - const legalName = findLegalMethodName(name, this); + const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true); const oldName = nameField.getValue(); if (oldName && oldName !== name && oldName !== legalName) { // Rename any callers. @@ -366,61 +380,6 @@ const CLASS_METHOD_DEF = { }, }; -/** - * Ensure two identically-named methods don't exist. - * Take the proposed method name, and return a legal name i.e. one that - * is not empty and doesn't collide with other methods. - * - * @param name Proposed method name. - * @param block Block to disambiguate. - * @returns Non-colliding name. - */ -function findLegalMethodName(name: string, block: ClassMethodDefBlock): string { - if (block.isInFlyout) { - // Flyouts can have multiple methods called 'my_method'. - return name; - } - name = name || 'unnamed'; - while (isMethodNameUsed(name, block.workspace, block)) { - // Collision with another method. - const r = name.match(/^(.*?)(\d+)$/); - if (!r) { - name += '2'; - } else { - name = r[1] + (parseInt(r[2]) + 1); - } - } - return name; -} - -/** - * Return if the given name is already a method name. - * - * @param name The questionable name. - * @param workspace The workspace to scan for collisions. - * @param opt_exclude Optional block to exclude from comparisons (one doesn't - * want to collide with oneself). - * @returns True if the name is used, otherwise return false. - */ -function isMethodNameUsed( - name: string, workspace: Blockly.Workspace, opt_exclude?: Blockly.Block): boolean { - const nameLowerCase = name.toLowerCase(); - for (const block of workspace.getBlocksByType(BLOCK_NAME)) { - if (block === opt_exclude) { - continue; - } - if (nameLowerCase === block.getFieldValue(FIELD_METHOD_NAME).toLowerCase()) { - return true; - } - const classMethodDefBlock = block as ClassMethodDefBlock; - if (classMethodDefBlock.mrcPythonMethodName && - nameLowerCase === classMethodDefBlock.mrcPythonMethodName.toLowerCase()) { - return true; - } - } - return false; -} - export const setup = function () { Blockly.Blocks[BLOCK_NAME] = CLASS_METHOD_DEF; }; diff --git a/src/blocks/mrc_component.ts b/src/blocks/mrc_component.ts index 0b8f2f96..89b9e3bd 100644 --- a/src/blocks/mrc_component.ts +++ b/src/blocks/mrc_component.ts @@ -28,6 +28,7 @@ import { Editor } from '../editor/editor'; import { ExtendedPythonGenerator } from '../editor/extended_python_generator'; import { getModuleTypeForWorkspace } from './utils/workspaces'; import { getAllowedTypesForSetCheck, getClassData, getSubclassNames } from './utils/python'; +import { makeLegalName } from './utils/validator'; import * as toolboxItems from '../toolbox/items'; import * as storageModule from '../storage/module'; import * as storageModuleContent from '../storage/module_content'; @@ -164,10 +165,19 @@ const COMPONENT = { } }, mrcNameFieldValidator(this: ComponentBlock, nameField: Blockly.FieldTextInput, name: string): string { - // Strip leading and trailing whitespace. - name = name.trim(); + if (this.isInFlyout) { + // Flyouts can have multiple methods with identical names. + return name; + } + + const otherNames: string[] = []; + this.workspace.getBlocksByType(BLOCK_NAME) + .filter(block => block.id !== this.id) + .forEach((block) => { + otherNames.push(block.getFieldValue(FIELD_NAME)); + }); - const legalName = name; + const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true); const oldName = nameField.getValue(); if (oldName && oldName !== name && oldName !== legalName) { // Rename any callers. diff --git a/src/blocks/mrc_event.ts b/src/blocks/mrc_event.ts index 565f75fb..1ad077ec 100644 --- a/src/blocks/mrc_event.ts +++ b/src/blocks/mrc_event.ts @@ -23,6 +23,7 @@ import * as Blockly from 'blockly'; import { MRC_STYLE_EVENTS } from '../themes/styles' import { createFieldNonEditableText } from '../fields/FieldNonEditableText'; +import { makeLegalName } from './utils/validator'; import { Parameter } from './mrc_class_method_def'; import { Editor } from '../editor/editor'; import { ExtendedPythonGenerator } from '../editor/extended_python_generator'; @@ -185,10 +186,19 @@ const EVENT = { }); }, mrcNameFieldValidator(this: EventBlock, nameField: Blockly.FieldTextInput, name: string): string { - // Strip leading and trailing whitespace. - name = name.trim(); + if (this.isInFlyout) { + // Flyouts can have multiple methods with identical names. + return name; + } + + const otherNames: string[] = []; + this.workspace.getBlocksByType(BLOCK_NAME) + .filter(block => block.id !== this.id) + .forEach((block) => { + otherNames.push(block.getFieldValue(FIELD_EVENT_NAME)); + }); - const legalName = name; + const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ false); const oldName = nameField.getValue(); if (oldName && oldName !== name && oldName !== legalName) { // Rename any callers. diff --git a/src/blocks/mrc_mechanism.ts b/src/blocks/mrc_mechanism.ts index 5ebbfd6b..2bfa38ad 100644 --- a/src/blocks/mrc_mechanism.ts +++ b/src/blocks/mrc_mechanism.ts @@ -27,6 +27,7 @@ import { createFieldNonEditableText } from '../fields/FieldNonEditableText'; import { Editor } from '../editor/editor'; import { ExtendedPythonGenerator } from '../editor/extended_python_generator'; import { getAllowedTypesForSetCheck } from './utils/python'; +import { makeLegalName } from './utils/validator'; import * as toolboxItems from '../toolbox/items'; import * as storageModule from '../storage/module'; import * as storageModuleContent from '../storage/module_content'; @@ -167,7 +168,19 @@ const MECHANISM = { // Strip leading and trailing whitespace. name = name.trim(); - const legalName = name; + if (this.isInFlyout) { + // Flyouts can have multiple methods with identical names. + return name; + } + + const otherNames: string[] = []; + this.workspace.getBlocksByType(BLOCK_NAME) + .filter(block => block.id !== this.id) + .forEach((block) => { + otherNames.push(block.getFieldValue(FIELD_NAME)); + }); + + const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true); const oldName = nameField.getValue(); if (oldName && oldName !== name && oldName !== legalName) { // Rename any callers. diff --git a/src/blocks/mrc_mechanism_component_holder.ts b/src/blocks/mrc_mechanism_component_holder.ts index 8db2a310..35ca635b 100644 --- a/src/blocks/mrc_mechanism_component_holder.ts +++ b/src/blocks/mrc_mechanism_component_holder.ts @@ -22,7 +22,7 @@ import * as Blockly from 'blockly'; import { MRC_STYLE_MECHANISMS } from '../themes/styles'; -import { getLegalName } from './utils/python'; +import { makeLegalName } from './utils/validator'; import { Editor } from '../editor/editor'; import { ExtendedPythonGenerator } from '../editor/extended_python_generator'; import * as storageModule from '../storage/module'; @@ -220,14 +220,16 @@ const MECHANISM_COMPONENT_HOLDER = { */ setNameOfChildBlock(this: MechanismComponentHolderBlock, child: Blockly.Block): void { const otherNames: string[] = [] - const descendants = this.getDescendants(true); - descendants + this.getDescendants(true) .filter(descendant => descendant.id !== child.id) .forEach(descendant => { otherNames.push(descendant.getFieldValue('NAME')); }); const currentName = child.getFieldValue('NAME'); - const legalName = getLegalName(currentName, otherNames); + // mechanism and component names must be valid python identifiers. + // event names do not need to be valid python identifiers. + const mustBeValidPythonIdentifier = (child.type === MRC_MECHANISM_NAME || child.type === MRC_COMPONENT_NAME); + const legalName = makeLegalName(currentName, otherNames, mustBeValidPythonIdentifier); if (legalName !== currentName) { child.setFieldValue(legalName, 'NAME'); } diff --git a/src/blocks/mrc_param_container.ts b/src/blocks/mrc_param_container.ts index ed3de194..b49ece2b 100644 --- a/src/blocks/mrc_param_container.ts +++ b/src/blocks/mrc_param_container.ts @@ -21,7 +21,7 @@ */ import * as Blockly from 'blockly'; import { MRC_STYLE_CLASS_BLOCKS } from '../themes/styles'; -import { getLegalName } from './utils/python'; +import { makeLegalName } from './utils/validator'; export const PARAM_CONTAINER_BLOCK_NAME = 'mrc_param_container'; const PARAM_ITEM_BLOCK_NAME = 'mrc_param_item'; @@ -85,13 +85,16 @@ const PARAM_ITEM = { const rootBlock: Blockly.Block | null = this.getRootBlock(); if (rootBlock) { const otherNames: string[] = [] - rootBlock!.getDescendants(true)?.forEach(itemBlock => { - if (itemBlock != this) { - otherNames.push(itemBlock.getFieldValue(FIELD_NAME)); - } - }); + rootBlock.getDescendants(true) + .filter(descendant => descendant.type === PARAM_ITEM_BLOCK_NAME && descendant !== this) + .forEach(itemBlock => { + if (itemBlock != this) { + otherNames.push(itemBlock.getFieldValue(FIELD_NAME)); + } + }); const currentName = this.getFieldValue(FIELD_NAME); - this.setFieldValue(getLegalName(currentName, otherNames), FIELD_NAME); + const legalName = makeLegalName(currentName, otherNames, /* mustBeValidPythonIdentifier */ true); + this.setFieldValue(legalName, FIELD_NAME); updateMutatorFlyout(this.workspace); } }, diff --git a/src/blocks/utils/python.ts b/src/blocks/utils/python.ts index 5f76d953..800c66cd 100644 --- a/src/blocks/utils/python.ts +++ b/src/blocks/utils/python.ts @@ -281,30 +281,6 @@ export function getOutputCheck(type: string): string { return type; } -// Function to return a legal name based off of proposed names and making sure it doesn't conflict -// This is a legal name for python methods and variables. -export function getLegalName(proposedName: string, existingNames: string[]){ - let newName = proposedName.trim().replace(' ', '_'); - - // TODO: Allow the user to put numbers in the name. - - if (!/^[A-Za-z_]/.test(newName)){ - newName = "_" + newName; - } - - while (existingNames.includes(newName)){ - const match = /(.*?)(\d+)$/.exec(newName) - - if (match) { - let lastNumber = +match[2] - newName = match[1] + (lastNumber + 1) - } else { - newName += "2" - } - } - return newName; -} - export function isExistingPythonModule(moduleName: string): boolean { for (const pythonData of allPythonData) { // Process modules. diff --git a/src/blocks/utils/validator.ts b/src/blocks/utils/validator.ts new file mode 100644 index 00000000..47df3b0c --- /dev/null +++ b/src/blocks/utils/validator.ts @@ -0,0 +1,72 @@ +/** + * @license + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @author lizlooney@google.com (Liz Looney) + */ + +/** + * Take the proposed name, and return a legal name. + * A legal name is: + * 1. not empty. + * 2. doesn't collide with other names (case insensitive). + * 3. Optional: is a valid python identifier. + */ +export function makeLegalName(proposedName: string, otherNames: string[], mustBeValidPythonIdentifier: boolean): string { + const otherNamesLowerCase = otherNames.map(n => n.toLowerCase()); + + // Strip leading and trailing whitespace. + let name = proposedName.trim(); + + // Make the name non-empty. + name = name || 'unnamed'; + + if (mustBeValidPythonIdentifier) { + if (!name.match(/^[a-zA-Z_][a-zA-Z0-9_]*$/)) { + let identifier = ''; + const firstChar = name[0]; + if (/[a-zA-Z_]/.test(firstChar)) { + identifier += firstChar; + } else if (/[a-zA-Z0-9_]/.test(firstChar)) { + // If the first character would be valid as the second charactor, insert an underscore. + identifier += '_' + firstChar; + } else { + identifier += '_'; + } + for (let i = 1; i < name.length; i++) { + const char = name[i]; + if (/[a-zA-Z0-9_]/.test(char)) { + identifier += char; + } else { + identifier += '_'; + } + } + name = identifier; + } + } + + while (otherNamesLowerCase.includes(name.toLowerCase())) { + // Collision with another name. + const r = name.match(/^(.*?)(\d+)$/); + if (!r) { + name += '2'; + } else { + name = r[1] + (parseInt(r[2]) + 1); + } + } + return name; +}