refactor!: Finish refactor of WorkspaceSvg VariableMap methods (#8946)

* docs: Make JSDoc @deprecated usage more consistent

* refactor(VariableMap)!: Refresh toolbox when map modified

  Delete the following methods from WorkspaceSvg:

  - renameVariableById
  - deleteVariableById
  - createVariable

  Modify the following methods on VariableMap to call
  this.workspace.refreshToolboxSelection() if this.workspace
  is a WorkspaceSvg, replicating the behaviour of the
  aforementioned deleted methods and additionally ensuring
  that that method is called following any change to the
  variable map:

  - renameVariable
  - changeVariableType
  - renameVariableAndUses
  - createVariable
  - addVariable
  - deleteVariable

  BREAKING CHANGE:

  This change ensures that the toolbox will be refreshed regardless
  of what route the VaribleMap was updated, rather than only being
  refreshed when it is updated via calls to methods on WorkspaceSvg.

  Overall this is much more likely to fix a bug (where the toolbox
  wasn't being refreshed when it should have been) than cause one
  (by refreshing the toolbox when it shouldn't be), but this is
  still a behaviour change which could _conceivably_ result an
  unexpected regression.

* refactor(VariableMap): Remove calls to deprecated getVariableUsesById

  Also refactor to use named imports core/variables.ts methods.

* refactor(Workspace): Use named imports for core/variables.ts methods

* refactor(FieldVariable): Remove call to deprecated getVariablesOfType

* refactor(variables): Remove calls to deprecated methods

* refactor(variables_dynamic): Remove call to deprecated getAllVariables

* refactor(xml): Remove calls to deprecated createVariable

* refactor(Events.VarCreate): Remove calls to deprecated methods

* refactor(Events.VarDelete): Remove calls to deprecated methods

* refactor(Events.VarRename): Remove calls to deprecated methods

Author: cpcallen

core/block_svg.ts

@@ -1083,13 +1083,15 @@ export class BlockSvg
   }
 
   /**
-   * @deprecated v11 - Set whether the block is manually enabled or disabled.
+   * Set whether the block is manually enabled or disabled.
+   *
    * The user can toggle whether a block is disabled from a context menu
    * option. A block may still be disabled for other reasons even if the user
    * attempts to manually enable it, such as when the block is in an invalid
    * location. This method is deprecated and setDisabledReason should be used
    * instead.
    *
+   * @deprecated v11: use setDisabledReason.
    * @param enabled True if enabled.
    */
   override setEnabled(enabled: boolean) {

core/events/events_var_create.ts

@@ -113,10 +113,12 @@ export class VarCreate extends VarBase {
           'the constructor, or call fromJson',
       );
     }
+    const variableMap = workspace.getVariableMap();
     if (forward) {
-      workspace.createVariable(this.varName, this.varType, this.varId);
+      variableMap.createVariable(this.varName, this.varType, this.varId);
     } else {
-      workspace.deleteVariableById(this.varId);
+      const variable = variableMap.getVariableById(this.varId);
+      if (variable) variableMap.deleteVariable(variable);
     }
   }
 }

core/events/events_var_delete.ts

@@ -106,10 +106,12 @@ export class VarDelete extends VarBase {
           'the constructor, or call fromJson',
       );
     }
+    const variableMap = workspace.getVariableMap();
     if (forward) {
-      workspace.deleteVariableById(this.varId);
+      const variable = variableMap.getVariableById(this.varId);
+      if (variable) variableMap.deleteVariable(variable);
     } else {
-      workspace.createVariable(this.varName, this.varType, this.varId);
+      variableMap.createVariable(this.varName, this.varType, this.varId);
     }
   }
 }

core/events/events_var_rename.ts

@@ -115,10 +115,12 @@ export class VarRename extends VarBase {
           'the constructor, or call fromJson',
       );
     }
+    const variableMap = workspace.getVariableMap();
+    const variable = variableMap.getVariableById(this.varId);
     if (forward) {
-      workspace.renameVariableById(this.varId, this.newName);
+      if (variable) variableMap.renameVariable(variable, this.newName);
     } else {
-      workspace.renameVariableById(this.varId, this.oldName);
+      if (variable) variableMap.renameVariable(variable, this.oldName);
     }
   }
 }

core/field_variable.ts

@@ -586,7 +586,9 @@ export class FieldVariable extends FieldDropdown {
       // doesn't modify the workspace's list.
       for (let i = 0; i < variableTypes.length; i++) {
         const variableType = variableTypes[i];
-        const variables = workspace.getVariablesOfType(variableType);
+        const variables = workspace
+          .getVariableMap()
+          .getVariablesOfType(variableType);
         variableModelList = variableModelList.concat(variables);
         if (workspace.isFlyout) {
           variableModelList = variableModelList.concat(

core/variable_map.ts

@@ -25,8 +25,9 @@ import {Names} from './names.js';
 import * as registry from './registry.js';
 import * as deprecation from './utils/deprecation.js';
 import * as idGenerator from './utils/idgenerator.js';
-import * as Variables from './variables.js';
+import {deleteVariable, getVariableUsesById} from './variables.js';
 import type {Workspace} from './workspace.js';
+import {WorkspaceSvg} from './workspace_svg.js';
 
 /**
  * Class for a variable map.  This contains a dictionary data structure with
@@ -92,6 +93,9 @@ export class VariableMap
     } finally {
       eventUtils.setGroup(existingGroup);
     }
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
     return variable;
   }
 
@@ -108,15 +112,17 @@ export class VariableMap
     if (!this.variableMap.has(newType)) {
       this.variableMap.set(newType, newTypeVariables);
     }
-
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
     return variable;
   }
 
   /**
    * 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.
+   * @deprecated v12: use VariableMap.renameVariable.
    * @param id ID of the variable to rename.
    * @param newName New variable name.
    */
@@ -155,6 +161,9 @@ export class VariableMap
     for (let i = 0; i < blocks.length; i++) {
       blocks[i].updateVarName(variable);
     }
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
   }
 
   /**
@@ -250,6 +259,9 @@ export class VariableMap
       this.variableMap.set(type, variables);
     }
     eventUtils.fire(new (eventUtils.get(EventType.VAR_CREATE))(variable));
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
     return variable;
   }
 
@@ -267,6 +279,9 @@ export class VariableMap
       );
     }
     this.variableMap.get(type)?.set(variable.getId(), variable);
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
   }
 
   /* Begin functions for variable deletion. */
@@ -276,7 +291,7 @@ export class VariableMap
    * @param variable Variable to delete.
    */
   deleteVariable(variable: IVariableModel<IVariableState>) {
-    const uses = this.getVariableUsesById(variable.getId());
+    const uses = getVariableUsesById(this.workspace, variable.getId());
     const existingGroup = eventUtils.getGroup();
     if (!existingGroup) {
       eventUtils.setGroup(true);
@@ -295,13 +310,16 @@ export class VariableMap
     } finally {
       eventUtils.setGroup(existingGroup);
     }
+    if (this.workspace instanceof WorkspaceSvg) {
+      this.workspace.refreshToolboxSelection();
+    }
   }
 
   /**
    * 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.
+   * @deprecated v12: use Blockly.Variables.deleteVariable.
    * @param id ID of variable to delete.
    */
   deleteVariableById(id: string) {
@@ -313,7 +331,7 @@ export class VariableMap
     );
     const variable = this.getVariableById(id);
     if (variable) {
-      Variables.deleteVariable(this.workspace, variable);
+      deleteVariable(this.workspace, variable);
     }
   }
 
@@ -398,7 +416,7 @@ export class VariableMap
   /**
    * Returns all of the variable names of all types.
    *
-   * @deprecated v12, use Blockly.Variables.getAllVariables.
+   * @deprecated v12: use Blockly.Variables.getAllVariables.
    * @returns All of the variable names of all types.
    */
   getAllVariableNames(): string[] {
@@ -420,7 +438,7 @@ export class VariableMap
   /**
    * Find all the uses of a named variable.
    *
-   * @deprecated v12, use Blockly.Variables.getVariableUsesById.
+   * @deprecated v12: use Blockly.Variables.getVariableUsesById.
    * @param id ID of the variable to find.
    * @returns Array of block usages.
    */
@@ -431,7 +449,7 @@ export class VariableMap
       'v13',
       'Blockly.Variables.getVariableUsesById',
     );
-    return Variables.getVariableUsesById(this.workspace, id);
+    return getVariableUsesById(this.workspace, id);
   }
 }
 

core/variables.ts

@@ -31,8 +31,9 @@ export const CATEGORY_NAME = 'VARIABLE';
 /**
  * Find all user-created variables that are in use in the workspace.
  * For use by generators.
+ *
  * To get a list of all variables on a workspace, including unused variables,
- * call Workspace.getAllVariables.
+ * call getAllVariables.
  *
  * @param ws The workspace to search for variables.
  * @returns Array of variable models.
@@ -61,6 +62,7 @@ export function allUsedVarModels(
 
 /**
  * Find all developer variables used by blocks in the workspace.
+ *
  * Developer variables are never shown to the user, but are declared as global
  * variables in the generated code.
  * To declare developer variables, define the getDeveloperVariables function on
@@ -146,7 +148,7 @@ export function flyoutCategory(
     },
     ...jsonFlyoutCategoryBlocks(
       workspace,
-      workspace.getVariablesOfType(''),
+      workspace.getVariableMap().getVariablesOfType(''),
       true,
     ),
   ];
@@ -268,7 +270,7 @@ function xmlFlyoutCategory(workspace: WorkspaceSvg): Element[] {
  * @returns Array of XML block elements.
  */
 export function flyoutCategoryBlocks(workspace: Workspace): Element[] {
-  const variableModelList = workspace.getVariablesOfType('');
+  const variableModelList = workspace.getVariableMap().getVariablesOfType('');
 
   const xmlList = [];
   if (variableModelList.length > 0) {
@@ -332,7 +334,10 @@ export function generateUniqueName(workspace: Workspace): string {
 function generateUniqueNameInternal(workspace: Workspace): string {
   return generateUniqueNameFromOptions(
     VAR_LETTER_OPTIONS.charAt(0),
-    workspace.getAllVariableNames(),
+    workspace
+      .getVariableMap()
+      .getAllVariables()
+      .map((v) => v.getName()),
   );
 }
 
@@ -415,7 +420,7 @@ export function createVariableButtonHandler(
       const existing = nameUsedWithAnyType(text, workspace);
       if (!existing) {
         // No conflict
-        workspace.createVariable(text, type);
+        workspace.getVariableMap().createVariable(text, type);
         if (opt_callback) opt_callback(text);
         return;
       }
@@ -478,7 +483,7 @@ export function renameVariable(
       );
       if (!existing && !procedure) {
         // No conflict.
-        workspace.renameVariableById(variable.getId(), newName);
+        workspace.getVariableMap().renameVariable(variable, newName);
         if (opt_callback) opt_callback(newName);
         return;
       }
@@ -762,6 +767,7 @@ function createVariable(
   opt_name?: string,
   opt_type?: string,
 ): IVariableModel<IVariableState> {
+  const variableMap = workspace.getVariableMap();
   const potentialVariableMap = workspace.getPotentialVariableMap();
   // Variables without names get uniquely named for this workspace.
   if (!opt_name) {
@@ -781,7 +787,7 @@ function createVariable(
     );
   } else {
     // In the main workspace, create a real variable.
-    variable = workspace.createVariable(opt_name, opt_type, id);
+    variable = variableMap.createVariable(opt_name, opt_type, id);
   }
   return variable;
 }

core/variables_dynamic.ts

@@ -148,7 +148,7 @@ export function flyoutCategory(
     },
     ...Variables.jsonFlyoutCategoryBlocks(
       workspace,
-      workspace.getAllVariables(),
+      workspace.getVariableMap().getAllVariables(),
       false,
       'variables_get_dynamic',
       'variables_set_dynamic',
@@ -203,7 +203,7 @@ function xmlFlyoutCategory(workspace: WorkspaceSvg): Element[] {
  * @returns Array of XML block elements.
  */
 export function flyoutCategoryBlocks(workspace: Workspace): Element[] {
-  const variableModelList = workspace.getAllVariables();
+  const variableModelList = workspace.getVariableMap().getAllVariables();
 
   const xmlList = [];
   if (variableModelList.length > 0) {