refactor!: Use IVariableMap instead of VariableMap (#8401)

* refactor: use IVariableMap in place of VariableMap. * refactor!: move variable deletion prompting out of VariableMap. * chore: Remove unused imports.
2026-01-07 00:50:27 +01:00 · 2024-07-22 09:17:40 -07:00
parent 294ef74d1b
commit 21c0a7d999
5 changed files with 162 additions and 103 deletions
--- a/core/names.ts
+++ b/core/names.ts
@@ -12,8 +12,11 @@
 // Former goog.module ID: Blockly.Names

 import {Msg} from './msg.js';
-// import * as Procedures from './procedures.js';
-import type {VariableMap} from './variable_map.js';
+import type {IVariableMap} from './interfaces/i_variable_map.js';
+import type {
+  IVariableModel,
+  IVariableState,
+} from './interfaces/i_variable_model.js';
 import * as Variables from './variables.js';
 import type {Workspace} from './workspace.js';

@@ -39,7 +42,8 @@ export class Names {
  /**
   * The variable map from the workspace, containing Blockly variable models.
   */
-  private variableMap: VariableMap | null = null;
+  private variableMap: IVariableMap<IVariableModel<IVariableState>> | null =
+    null;

  /**
   * @param reservedWordsList A comma-separated string of words that are illegal
@@ -70,7 +74,7 @@ export class Names {
   *
   * @param map The map to track.
   */
-  setVariableMap(map: VariableMap) {
+  setVariableMap(map: IVariableMap<IVariableModel<IVariableState>>) {
    this.variableMap = map;
  }

--- a/core/variable_map.ts
+++ b/core/variable_map.ts
@@ -17,10 +17,10 @@ import './events/events_var_delete.js';
 import './events/events_var_rename.js';

 import type {Block} from './block.js';
-import * as dialog from './dialog.js';
+import * as deprecation from './utils/deprecation.js';
 import * as eventUtils from './events/utils.js';
 import * as registry from './registry.js';
-import {Msg} from './msg.js';
+import * as Variables from './variables.js';
 import {Names} from './names.js';
 import * as idGenerator from './utils/idgenerator.js';
 import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js';
@@ -247,7 +247,6 @@ export class VariableMap
      this.variableMap.set(type, variables);
    }
    eventUtils.fire(new (eventUtils.get(eventUtils.VAR_CREATE))(variable));
-
    return variable;
  }

@@ -269,77 +268,12 @@ export class VariableMap

  /* Begin functions for variable deletion. */
  /**
-   * Delete a variable.
+   * Delete a variable and all of its uses without confirmation.
   *
   * @param variable Variable to delete.
   */
  deleteVariable(variable: IVariableModel<IVariableState>) {
-    const variables = this.variableMap.get(variable.getType());
-    if (!variables || !variables.has(variable.getId())) return;
-    variables.delete(variable.getId());
-    eventUtils.fire(new (eventUtils.get(eventUtils.VAR_DELETE))(variable));
-    if (variables.size === 0) {
-      this.variableMap.delete(variable.getType());
-    }
-  }
-
-  /**
-   * Delete a variables by the passed in ID and all of its uses from this
-   * workspace. May prompt the user for confirmation.
-   *
-   * @param id ID of variable to delete.
-   */
-  deleteVariableById(id: string) {
-    const variable = this.getVariableById(id);
-    if (variable) {
-      // Check whether this variable is a function parameter before deleting.
-      const variableName = variable.getName();
-      const uses = this.getVariableUsesById(id);
-      for (let i = 0, block; (block = uses[i]); i++) {
-        if (
-          block.type === 'procedures_defnoreturn' ||
-          block.type === 'procedures_defreturn'
-        ) {
-          const procedureName = String(block.getFieldValue('NAME'));
-          const deleteText = Msg['CANNOT_DELETE_VARIABLE_PROCEDURE']
-            .replace('%1', variableName)
-            .replace('%2', procedureName);
-          dialog.alert(deleteText);
-          return;
-        }
-      }
-
-      if (uses.length > 1) {
-        // Confirm before deleting multiple blocks.
-        const confirmText = Msg['DELETE_VARIABLE_CONFIRMATION']
-          .replace('%1', String(uses.length))
-          .replace('%2', variableName);
-        dialog.confirm(confirmText, (ok) => {
-          if (ok && variable) {
-            this.deleteVariableInternal(variable, uses);
-          }
-        });
-      } else {
-        // No confirmation necessary for a single block.
-        this.deleteVariableInternal(variable, uses);
-      }
-    } else {
-      console.warn("Can't delete non-existent variable: " + id);
-    }
-  }
-
-  /**
-   * Deletes a variable and all of its uses from this workspace without asking
-   * the user for confirmation.
-   *
-   * @param variable Variable to delete.
-   * @param uses An array of uses of the variable.
-   * @internal
-   */
-  deleteVariableInternal(
-    variable: IVariableModel<IVariableState>,
-    uses: Block[],
-  ) {
+    const uses = this.getVariableUsesById(variable.getId());
    const existingGroup = eventUtils.getGroup();
    if (!existingGroup) {
      eventUtils.setGroup(true);
@@ -348,11 +282,37 @@ export class VariableMap
      for (let i = 0; i < uses.length; i++) {
        uses[i].dispose(true);
      }
-      this.deleteVariable(variable);
+      const variables = this.variableMap.get(variable.getType());
+      if (!variables || !variables.has(variable.getId())) return;
+      variables.delete(variable.getId());
+      eventUtils.fire(new (eventUtils.get(eventUtils.VAR_DELETE))(variable));
+      if (variables.size === 0) {
+        this.variableMap.delete(variable.getType());
+      }
    } finally {
      eventUtils.setGroup(existingGroup);
    }
  }
+
+  /**
+   * @deprecated v12 - Delete a variables by the passed in ID and all of its
+   * uses from this workspace. May prompt the user for confirmation.
+   *
+   * @param id ID of variable to delete.
+   */
+  deleteVariableById(id: string) {
+    deprecation.warn(
+      'VariableMap.deleteVariableById',
+      'v12',
+      'v13',
+      'Blockly.Variables.deleteVariable',
+    );
+    const variable = this.getVariableById(id);
+    if (variable) {
+      Variables.deleteVariable(this.workspace, variable);
+    }
+  }
+
  /* End functions for variable deletion. */
  /**
   * Find the variable by the given name and type and return it.  Return null if
@@ -431,7 +391,7 @@ export class VariableMap
  getVariableTypes(ws: Workspace | null): string[] {
    const variableTypes = new Set<string>(this.variableMap.keys());
    if (ws && ws.getPotentialVariableMap()) {
-      for (const key of ws.getPotentialVariableMap()!.variableMap.keys()) {
+      for (const key of ws.getPotentialVariableMap()!.getTypes()) {
        variableTypes.add(key);
      }
    }
@@ -470,26 +430,19 @@ export class VariableMap
  }

  /**
-   * Find all the uses of a named variable.
+   * @deprecated v12 - Find all the uses of a named variable.
   *
   * @param id ID of the variable to find.
   * @returns Array of block usages.
   */
  getVariableUsesById(id: string): Block[] {
-    const uses = [];
-    const blocks = this.workspace.getAllBlocks(false);
-    // Iterate through every block and check the name.
-    for (let i = 0; i < blocks.length; i++) {
-      const blockVariables = blocks[i].getVarModels();
-      if (blockVariables) {
-        for (let j = 0; j < blockVariables.length; j++) {
-          if (blockVariables[j].getId() === id) {
-            uses.push(blocks[i]);
-          }
-        }
-      }
-    }
-    return uses;
+    deprecation.warn(
+      'VariableMap.getVariableUsesById',
+      'v12',
+      'v13',
+      'Blockly.Variables.getVariableUsesById',
+    );
+    return Variables.getVariableUsesById(this.workspace, id);
  }
 }

--- a/core/variables.ts
+++ b/core/variables.ts
@@ -6,6 +6,7 @@

 // Former goog.module ID: Blockly.Variables

+import type {Block} from './block.js';
 import {Blocks} from './blocks.js';
 import * as dialog from './dialog.js';
 import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js';
@@ -683,6 +684,74 @@ export function compareByName(
    .localeCompare(var2.getName(), undefined, {sensitivity: 'base'});
 }

+/**
+ * Find all the uses of a named variable.
+ *
+ * @param workspace The workspace to search for the variable.
+ * @param id ID of the variable to find.
+ * @returns Array of block usages.
+ */
+export function getVariableUsesById(workspace: Workspace, id: string): Block[] {
+  const uses = [];
+  const blocks = workspace.getAllBlocks(false);
+  // Iterate through every block and check the name.
+  for (let i = 0; i < blocks.length; i++) {
+    const blockVariables = blocks[i].getVarModels();
+    if (blockVariables) {
+      for (let j = 0; j < blockVariables.length; j++) {
+        if (blockVariables[j].getId() === id) {
+          uses.push(blocks[i]);
+        }
+      }
+    }
+  }
+  return uses;
+}
+
+/**
+ * Delete a variable and all of its uses from the given workspace. May prompt
+ * the user for confirmation.
+ *
+ * @param workspace The workspace from which to delete the variable.
+ * @param variable The variable to delete.
+ */
+export function deleteVariable(
+  workspace: Workspace,
+  variable: IVariableModel<IVariableState>,
+) {
+  // Check whether this variable is a function parameter before deleting.
+  const variableName = variable.getName();
+  const uses = getVariableUsesById(workspace, variable.getId());
+  for (let i = 0, block; (block = uses[i]); i++) {
+    if (
+      block.type === 'procedures_defnoreturn' ||
+      block.type === 'procedures_defreturn'
+    ) {
+      const procedureName = String(block.getFieldValue('NAME'));
+      const deleteText = Msg['CANNOT_DELETE_VARIABLE_PROCEDURE']
+        .replace('%1', variableName)
+        .replace('%2', procedureName);
+      dialog.alert(deleteText);
+      return;
+    }
+  }
+
+  if (uses.length > 1) {
+    // Confirm before deleting multiple blocks.
+    const confirmText = Msg['DELETE_VARIABLE_CONFIRMATION']
+      .replace('%1', String(uses.length))
+      .replace('%2', variableName);
+    dialog.confirm(confirmText, (ok) => {
+      if (ok && variable) {
+        workspace.getVariableMap().deleteVariable(variable);
+      }
+    });
+  } else {
+    // No confirmation necessary for a single block.
+    workspace.getVariableMap().deleteVariable(variable);
+  }
+}
+
 export const TEST_ONLY = {
  generateUniqueNameInternal,
 };
--- a/core/workspace.ts
+++ b/core/workspace.ts
@@ -28,7 +28,8 @@ import * as arrayUtils from './utils/array.js';
 import * as idGenerator from './utils/idgenerator.js';
 import * as math from './utils/math.js';
 import type * as toolbox from './utils/toolbox.js';
-import {VariableMap} from './variable_map.js';
+import * as Variables from './variables.js';
+import type {IVariableMap} from './interfaces/i_variable_map.js';
 import type {
  IVariableModel,
  IVariableState,
@@ -110,7 +111,7 @@ export class Workspace implements IASTNodeLocation {
  protected redoStack_: Abstract[] = [];
  private readonly blockDB = new Map<string, Block>();
  private readonly typedBlocksDB = new Map<string, Block[]>();
-  private variableMap: VariableMap;
+  private variableMap: IVariableMap<IVariableModel<IVariableState>>;
  private procedureMap: IProcedureMap = new ObservableProcedureMap();

  /**
@@ -121,7 +122,9 @@ export class Workspace implements IASTNodeLocation {
   * these by tracking "potential" variables in the flyout.  These variables
   * become real when references to them are dragged into the main workspace.
   */
-  private potentialVariableMap: VariableMap | null = null;
+  private potentialVariableMap: IVariableMap<
+    IVariableModel<IVariableState>
+  > | null = null;

  /** @param opt_options Dictionary of options. */
  constructor(opt_options?: Options) {
@@ -147,6 +150,7 @@ export class Workspace implements IASTNodeLocation {
     * all of the named variables in the workspace, including variables that are
     * not currently in use.
     */
+    const VariableMap = this.getVariableMapClass();
    this.variableMap = new VariableMap(this);
  }

@@ -384,7 +388,9 @@ export class Workspace implements IASTNodeLocation {
   * @param newName New variable name.
   */
  renameVariableById(id: string, newName: string) {
-    this.variableMap.renameVariableById(id, newName);
+    const variable = this.variableMap.getVariableById(id);
+    if (!variable) return;
+    this.variableMap.renameVariable(variable, newName);
  }

  /**
@@ -417,7 +423,7 @@ export class Workspace implements IASTNodeLocation {
   * @returns Array of block usages.
   */
  getVariableUsesById(id: string): Block[] {
-    return this.variableMap.getVariableUsesById(id);
+    return Variables.getVariableUsesById(this, id);
  }

  /**
@@ -427,7 +433,12 @@ export class Workspace implements IASTNodeLocation {
   * @param id ID of variable to delete.
   */
  deleteVariableById(id: string) {
-    this.variableMap.deleteVariableById(id);
+    const variable = this.variableMap.getVariableById(id);
+    if (!variable) {
+      console.warn(`Can't delete non-existent variable: ${id}`);
+      return;
+    }
+    Variables.deleteVariable(this, variable);
  }

  /**
@@ -476,7 +487,12 @@ export class Workspace implements IASTNodeLocation {
   * @internal
   */
  getVariableTypes(): string[] {
-    return this.variableMap.getVariableTypes(this);
+    const variableTypes = new Set<string>(this.variableMap.getTypes());
+    (this.potentialVariableMap?.getTypes() ?? []).forEach((t) =>
+      variableTypes.add(t),
+    );
+    variableTypes.add('');
+    return Array.from(variableTypes.values());
  }

  /**
@@ -494,7 +510,7 @@ export class Workspace implements IASTNodeLocation {
   * @returns List of all variable names of all types.
   */
  getAllVariableNames(): string[] {
-    return this.variableMap.getAllVariableNames();
+    return this.variableMap.getAllVariables().map((v) => v.getName());
  }
  /* End functions that are just pass-throughs to the variable map. */
  /**
@@ -789,7 +805,9 @@ export class Workspace implements IASTNodeLocation {
   * @returns The potential variable map.
   * @internal
   */
-  getPotentialVariableMap(): VariableMap | null {
+  getPotentialVariableMap(): IVariableMap<
+    IVariableModel<IVariableState>
+  > | null {
    return this.potentialVariableMap;
  }

@@ -799,6 +817,7 @@ export class Workspace implements IASTNodeLocation {
   * @internal
   */
  createPotentialVariableMap() {
+    const VariableMap = this.getVariableMapClass();
    this.potentialVariableMap = new VariableMap(this);
  }

@@ -807,7 +826,7 @@ export class Workspace implements IASTNodeLocation {
   *
   * @returns The variable map.
   */
-  getVariableMap(): VariableMap {
+  getVariableMap(): IVariableMap<IVariableModel<IVariableState>> {
    return this.variableMap;
  }

@@ -817,7 +836,7 @@ export class Workspace implements IASTNodeLocation {
   * @param variableMap The variable map.
   * @internal
   */
-  setVariableMap(variableMap: VariableMap) {
+  setVariableMap(variableMap: IVariableMap<IVariableModel<IVariableState>>) {
    this.variableMap = variableMap;
  }

@@ -866,4 +885,18 @@ export class Workspace implements IASTNodeLocation {
  static getAll(): Workspace[] {
    return common.getAllWorkspaces();
  }
+
+  protected getVariableMapClass(): new (
+    ...p1: any[]
+  ) => IVariableMap<IVariableModel<IVariableState>> {
+    const VariableMap = registry.getClassFromOptions(
+      registry.Type.VARIABLE_MAP,
+      this.options,
+      true,
+    );
+    if (!VariableMap) {
+      throw new Error('No variable map is registered.');
+    }
+    return VariableMap;
+  }
 }
--- a/tests/mocha/variable_map_test.js
+++ b/tests/mocha/variable_map_test.js
@@ -21,7 +21,7 @@ suite('Variable Map', function () {
  setup(function () {
    sharedTestSetup.call(this);
    this.workspace = new Blockly.Workspace();
-    this.variableMap = new Blockly.VariableMap(this.workspace);
+    this.variableMap = this.workspace.getVariableMap();
  });

  teardown(function () {