refactor: deprecate and clean up variable-related methods. (#8415)

* refactor: deprecate and clean up variable-related methods. * chore: Add deprecation JSDoc.
2026-01-08 09:30:06 +01:00 · 2024-07-22 17:13:20 -07:00
parent fb82c9c9bb
commit 91892ac303
6 changed files with 91 additions and 89 deletions
--- a/core/field_variable.ts
+++ b/core/field_variable.ts
@@ -420,7 +420,7 @@ export class FieldVariable extends FieldDropdown {
    if (variableTypes === null) {
      // If variableTypes is null, return all variable types.
      if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) {
-        return this.sourceBlock_.workspace.getVariableTypes();
+        return this.sourceBlock_.workspace.getVariableMap().getTypes();
      }
    }
    variableTypes = variableTypes || [''];
--- a/core/variable_map.ts
+++ b/core/variable_map.ts
@@ -84,14 +84,9 @@ export class VariableMap
      // The IDs may match if the rename is a simple case change (name1 ->
      // Name1).
      if (!conflictVar || conflictVar.getId() === variable.getId()) {
-        this.renameVariableAndUses_(variable, newName, blocks);
+        this.renameVariableAndUses(variable, newName, blocks);
      } else {
-        this.renameVariableWithConflict_(
-          variable,
-          newName,
-          conflictVar,
-          blocks,
-        );
+        this.renameVariableWithConflict(variable, newName, conflictVar, blocks);
      }
    } finally {
      eventUtils.setGroup(existingGroup);
@@ -120,10 +115,17 @@ export class VariableMap
   * Rename a variable by updating its name in the variable map. Identify the
   * variable to rename with the given ID.
   *
+   * @deprecated v12, use VariableMap.renameVariable.
   * @param id ID of the variable to rename.
   * @param newName New variable name.
   */
  renameVariableById(id: string, newName: string) {
+    deprecation.warn(
+      'VariableMap.renameVariableById',
+      'v12',
+      'v13',
+      'VariableMap.renameVariable',
+    );
    const variable = this.getVariableById(id);
    if (!variable) {
      throw Error("Tried to rename a variable that didn't exist. ID: " + id);
@@ -140,7 +142,7 @@ export class VariableMap
   * @param newName New variable name.
   * @param blocks The list of all blocks in the workspace.
   */
-  private renameVariableAndUses_(
+  private renameVariableAndUses(
    variable: IVariableModel<IVariableState>,
    newName: string,
    blocks: Block[],
@@ -165,7 +167,7 @@ export class VariableMap
   * @param conflictVar The variable that was already using newName.
   * @param blocks The list of all blocks in the workspace.
   */
-  private renameVariableWithConflict_(
+  private renameVariableWithConflict(
    variable: IVariableModel<IVariableState>,
    newName: string,
    conflictVar: IVariableModel<IVariableState>,
@@ -176,7 +178,7 @@ export class VariableMap

    if (newName !== oldCase) {
      // Simple rename to change the case and update references.
-      this.renameVariableAndUses_(conflictVar, newName, blocks);
+      this.renameVariableAndUses(conflictVar, newName, blocks);
    }

    // These blocks now refer to a different variable.
@@ -295,9 +297,10 @@ export class VariableMap
  }

  /**
-   * @deprecated v12 - Delete a variables by the passed in ID and all of its
-   * uses from this workspace. May prompt the user for confirmation.
+   * Delete a variables by the passed in ID and all of its uses from this
+   * workspace. May prompt the user for confirmation.
   *
+   * @deprecated v12, use Blockly.Variables.deleteVariable.
   * @param id ID of variable to delete.
   */
  deleteVariableById(id: string) {
@@ -378,29 +381,6 @@ export class VariableMap
    return [...this.variableMap.keys()];
  }

-  /**
-   * Return all variable and potential variable types.  This list always
-   * contains the empty string.
-   *
-   * @param ws The workspace used to look for potential variables. This can be
-   *     different than the workspace stored on this object if the passed in ws
-   *     is a flyout workspace.
-   * @returns List of variable types.
-   * @internal
-   */
-  getVariableTypes(ws: Workspace | null): string[] {
-    const variableTypes = new Set<string>(this.variableMap.keys());
-    if (ws && ws.getPotentialVariableMap()) {
-      for (const key of ws.getPotentialVariableMap()!.getTypes()) {
-        variableTypes.add(key);
-      }
-    }
-    if (!variableTypes.has('')) {
-      variableTypes.add('');
-    }
-    return Array.from(variableTypes.values());
-  }
-
  /**
   * Return all variables of all types.
   *
@@ -417,9 +397,16 @@ export class VariableMap
  /**
   * Returns all of the variable names of all types.
   *
+   * @deprecated v12, use Blockly.Variables.getAllVariables.
   * @returns All of the variable names of all types.
   */
  getAllVariableNames(): string[] {
+    deprecation.warn(
+      'VariableMap.getAllVariableNames',
+      'v12',
+      'v13',
+      'Blockly.Variables.getAllVariables',
+    );
    const names: string[] = [];
    for (const variables of this.variableMap.values()) {
      for (const variable of variables.values()) {
@@ -430,8 +417,9 @@ export class VariableMap
  }

  /**
-   * @deprecated v12 - Find all the uses of a named variable.
+   * Find all the uses of a named variable.
   *
+   * @deprecated v12, use Blockly.Variables.getVariableUsesById.
   * @param id ID of the variable to find.
   * @returns Array of block usages.
   */
--- a/core/variable_model.ts
+++ b/core/variable_model.ts
@@ -140,21 +140,6 @@ export class VariableModel implements IVariableModel<IVariableState> {
    workspace.getVariableMap().addVariable(variable);
    eventUtils.fire(new (eventUtils.get(eventUtils.VAR_CREATE))(variable));
  }
-
-  /**
-   * A custom compare function for the VariableModel objects.
-   *
-   * @param var1 First variable to compare.
-   * @param var2 Second variable to compare.
-   * @returns -1 if name of var1 is less than name of var2, 0 if equal, and 1 if
-   *     greater.
-   * @internal
-   */
-  static compareByName(var1: VariableModel, var2: VariableModel): number {
-    return var1
-      .getName()
-      .localeCompare(var2.getName(), undefined, {sensitivity: 'base'});
-  }
 }

 registry.register(
--- a/core/workspace.ts
+++ b/core/workspace.ts
@@ -25,6 +25,7 @@ import type {IConnectionChecker} from './interfaces/i_connection_checker.js';
 import {Options} from './options.js';
 import * as registry from './registry.js';
 import * as arrayUtils from './utils/array.js';
+import * as deprecation from './utils/deprecation.js';
 import * as idGenerator from './utils/idgenerator.js';
 import * as math from './utils/math.js';
 import type * as toolbox from './utils/toolbox.js';
@@ -381,13 +382,19 @@ export class Workspace implements IASTNodeLocation {

  /* Begin functions that are just pass-throughs to the variable map. */
  /**
-   * Rename a variable by updating its name in the variable map. Identify the
-   * variable to rename with the given ID.
+   * @deprecated v12 - Rename a variable by updating its name in the variable
+   * map. Identify the variable to rename with the given ID.
   *
   * @param id ID of the variable to rename.
   * @param newName New variable name.
   */
  renameVariableById(id: string, newName: string) {
+    deprecation.warn(
+      'Blockly.Workspace.renameVariableById',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().renameVariable',
+    );
    const variable = this.variableMap.getVariableById(id);
    if (!variable) return;
    this.variableMap.renameVariable(variable, newName);
@@ -396,6 +403,7 @@ export class Workspace implements IASTNodeLocation {
  /**
   * Create a variable with a given name, optional type, and optional ID.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().createVariable.
   * @param name The name of the variable. This must be unique across variables
   *     and procedures.
   * @param opt_type The type of the variable like 'int' or 'string'.
@@ -409,6 +417,12 @@ export class Workspace implements IASTNodeLocation {
    opt_type?: string | null,
    opt_id?: string | null,
  ): IVariableModel<IVariableState> {
+    deprecation.warn(
+      'Blockly.Workspace.createVariable',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().createVariable',
+    );
    return this.variableMap.createVariable(
      name,
      opt_type ?? undefined,
@@ -419,10 +433,17 @@ export class Workspace implements IASTNodeLocation {
  /**
   * Find all the uses of the given variable, which is identified by ID.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getVariableUsesById
   * @param id ID of the variable to find.
   * @returns Array of block usages.
   */
  getVariableUsesById(id: string): Block[] {
+    deprecation.warn(
+      'Blockly.Workspace.getVariableUsesById',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getVariableUsesById',
+    );
    return Variables.getVariableUsesById(this, id);
  }

@@ -430,9 +451,16 @@ export class Workspace implements IASTNodeLocation {
   * Delete a variables by the passed in ID and all of its uses from this
   * workspace. May prompt the user for confirmation.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().deleteVariable.
   * @param id ID of variable to delete.
   */
  deleteVariableById(id: string) {
+    deprecation.warn(
+      'Blockly.Workspace.deleteVariableById',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().deleteVariable',
+    );
    const variable = this.variableMap.getVariableById(id);
    if (!variable) {
      console.warn(`Can't delete non-existent variable: ${id}`);
@@ -445,6 +473,7 @@ export class Workspace implements IASTNodeLocation {
   * Find the variable by the given name and return it. Return null if not
   * found.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getVariable.
   * @param name The name to check for.
   * @param opt_type The type of the variable.  If not provided it defaults to
   *     the empty string, which is a specific type.
@@ -454,6 +483,12 @@ export class Workspace implements IASTNodeLocation {
    name: string,
    opt_type?: string,
  ): IVariableModel<IVariableState> | null {
+    deprecation.warn(
+      'Blockly.Workspace.getVariable',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getVariable',
+    );
    // TODO (#1559): Possibly delete this function after resolving #1559.
    return this.variableMap.getVariable(name, opt_type);
  }
@@ -461,10 +496,17 @@ export class Workspace implements IASTNodeLocation {
  /**
   * Find the variable by the given ID and return it. Return null if not found.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getVariableById.
   * @param id The ID to check for.
   * @returns The variable with the given ID.
   */
  getVariableById(id: string): IVariableModel<IVariableState> | null {
+    deprecation.warn(
+      'Blockly.Workspace.getVariableById',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getVariableById',
+    );
    return this.variableMap.getVariableById(id);
  }

@@ -472,44 +514,50 @@ export class Workspace implements IASTNodeLocation {
   * Find the variable with the specified type. If type is null, return list of
   *     variables with empty string type.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getVariablesOfType.
   * @param type Type of the variables to find.
   * @returns The sought after variables of the passed in type. An empty array
   *     if none are found.
   */
  getVariablesOfType(type: string | null): IVariableModel<IVariableState>[] {
-    return this.variableMap.getVariablesOfType(type ?? '');
-  }
-
-  /**
-   * Return all variable types.
-   *
-   * @returns List of variable types.
-   * @internal
-   */
-  getVariableTypes(): string[] {
-    const variableTypes = new Set<string>(this.variableMap.getTypes());
-    (this.potentialVariableMap?.getTypes() ?? []).forEach((t) =>
-      variableTypes.add(t),
+    deprecation.warn(
+      'Blockly.Workspace.getVariablesOfType',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getVariablesOfType',
    );
-    variableTypes.add('');
-    return Array.from(variableTypes.values());
+    return this.variableMap.getVariablesOfType(type ?? '');
  }

  /**
   * Return all variables of all types.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getAllVariables.
   * @returns List of variable models.
   */
  getAllVariables(): IVariableModel<IVariableState>[] {
+    deprecation.warn(
+      'Blockly.Workspace.getAllVariables',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getAllVariables',
+    );
    return this.variableMap.getAllVariables();
  }

  /**
   * Returns all variable names of all types.
   *
+   * @deprecated v12, use Blockly.Workspace.getVariableMap().getAllVariables.
   * @returns List of all variable names of all types.
   */
  getAllVariableNames(): string[] {
+    deprecation.warn(
+      'Blockly.Workspace.getAllVariableNames',
+      'v12',
+      'v13',
+      'Blockly.Workspace.getVariableMap().getAllVariables',
+    );
    return this.variableMap.getAllVariables().map((v) => v.getName());
  }
  /* End functions that are just pass-throughs to the variable map. */
--- a/tests/mocha/field_variable_test.js
+++ b/tests/mocha/field_variable_test.js
@@ -388,8 +388,7 @@ suite('Variable Fields', function () {
      fieldVariable.variableTypes = null;

      const resultTypes = fieldVariable.getVariableTypes();
-      // The empty string is always one of the options.
-      assert.deepEqual(resultTypes, ['type1', 'type2', '']);
+      assert.deepEqual(resultTypes, ['type1', 'type2']);
    });
    test('variableTypes is the empty list', function () {
      const fieldVariable = new Blockly.FieldVariable('name1');
--- a/tests/mocha/variable_map_test.js
+++ b/tests/mocha/variable_map_test.js
@@ -187,24 +187,6 @@ suite('Variable Map', function () {
    });
  });

-  suite('getVariableTypes', function () {
-    test('Trivial', function () {
-      this.variableMap.createVariable('name1', 'type1', 'id1');
-      this.variableMap.createVariable('name2', 'type1', 'id2');
-      this.variableMap.createVariable('name3', 'type2', 'id3');
-      this.variableMap.createVariable('name4', 'type3', 'id4');
-      const resultArray = this.variableMap.getVariableTypes();
-      // The empty string is always an option.
-      assert.deepEqual(resultArray, ['type1', 'type2', 'type3', '']);
-    });
-
-    test('None', function () {
-      // The empty string is always an option.
-      const resultArray = this.variableMap.getVariableTypes();
-      assert.deepEqual(resultArray, ['']);
-    });
-  });
-
  suite('getVariablesOfType', function () {
    test('Trivial', function () {
      const var1 = this.variableMap.createVariable('name1', 'type1', 'id1');