refactor!: Use JSON instead of XML for defining dynamic toolbox categories. (#8658)

* refactor!: Use JSON instead of XML for defining dynamic toolbox categories. * chore: Fix tests. * chore: Remove unused import. * chore: Update docstrings. * chore: Revert removal of XML-based category functions. * chore: Add deprecation notices.
2026-01-10 02:17:09 +01:00 · 2025-01-08 11:50:18 -08:00
parent 4dcffa0914
commit 80a6d85c26
6 changed files with 362 additions and 83 deletions
--- a/core/procedures.ts
+++ b/core/procedures.ts
@@ -42,6 +42,8 @@ import {IProcedureModel} from './interfaces/i_procedure_model.js';
 import {Msg} from './msg.js';
 import {Names} from './names.js';
 import {ObservableProcedureMap} from './observable_procedure_map.js';
+import * as deprecation from './utils/deprecation.js';
+import type {FlyoutItemInfo} from './utils/toolbox.js';
 import * as utilsXml from './utils/xml.js';
 import * as Variables from './variables.js';
 import type {Workspace} from './workspace.js';
@@ -238,7 +240,7 @@ export function rename(this: Field, name: string): string {
 * @param workspace The workspace containing procedures.
 * @returns Array of XML block elements.
 */
-export function flyoutCategory(workspace: WorkspaceSvg): Element[] {
+function xmlFlyoutCategory(workspace: WorkspaceSvg): Element[] {
  const xmlList = [];
  if (Blocks['procedures_defnoreturn']) {
    // <block type="procedures_defnoreturn" gap="16">
@@ -322,6 +324,109 @@ export function flyoutCategory(workspace: WorkspaceSvg): Element[] {
  return xmlList;
 }

+/**
+ * Internal wrapper that returns the contents of the procedure category.
+ *
+ * @internal
+ * @param workspace The workspace to populate procedure blocks for.
+ */
+export function internalFlyoutCategory(
+  workspace: WorkspaceSvg,
+): FlyoutItemInfo[] {
+  return flyoutCategory(workspace, false);
+}
+
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: true,
+): Element[];
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: false,
+): FlyoutItemInfo[];
+/**
+ * Construct the blocks required by the flyout for the procedure category.
+ *
+ * @param workspace The workspace containing procedures.
+ * @param useXml True to return the contents as XML, false to use JSON.
+ * @returns List of flyout contents as either XML or JSON.
+ */
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml = true,
+): Element[] | FlyoutItemInfo[] {
+  if (useXml) {
+    deprecation.warn(
+      'The XML return value of Blockly.Procedures.flyoutCategory()',
+      'v12',
+      'v13',
+      'the same method, but handle a return type of FlyoutItemInfo[] (JSON) instead.',
+    );
+    return xmlFlyoutCategory(workspace);
+  }
+  const blocks = [];
+  if (Blocks['procedures_defnoreturn']) {
+    blocks.push({
+      'kind': 'block',
+      'type': 'procedures_defnoreturn',
+      'gap': 16,
+      'fields': {
+        'NAME': Msg['PROCEDURES_DEFNORETURN_PROCEDURE'],
+      },
+    });
+  }
+  if (Blocks['procedures_defreturn']) {
+    blocks.push({
+      'kind': 'block',
+      'type': 'procedures_defreturn',
+      'gap': 16,
+      'fields': {
+        'NAME': Msg['PROCEDURES_DEFRETURN_PROCEDURE'],
+      },
+    });
+  }
+  if (Blocks['procedures_ifreturn']) {
+    blocks.push({
+      'kind': 'block',
+      'type': 'procedures_ifreturn',
+      'gap': 16,
+    });
+  }
+  if (blocks.length) {
+    // Add slightly larger gap between system blocks and user calls.
+    blocks[blocks.length - 1]['gap'] = 24;
+  }
+
+  /**
+   * Creates JSON block definitions for each of the given procedures.
+   *
+   * @param procedureList A list of procedures, each of which is defined by a
+   *     three-element list of name, parameter list, and return value boolean.
+   * @param templateName The type of the block to generate.
+   */
+  function populateProcedures(
+    procedureList: ProcedureTuple[],
+    templateName: string,
+  ) {
+    for (const [name, args] of procedureList) {
+      blocks.push({
+        'kind': 'block',
+        'type': templateName,
+        'gap': 16,
+        'extraState': {
+          'name': name,
+          'params': args,
+        },
+      });
+    }
+  }
+
+  const tuple = allProcedures(workspace);
+  populateProcedures(tuple[0], 'procedures_callnoreturn');
+  populateProcedures(tuple[1], 'procedures_callreturn');
+  return blocks;
+}
+
 /**
 * Updates the procedure mutator's flyout so that the arg block is not a
 * duplicate of another arg.
--- a/core/variables.ts
+++ b/core/variables.ts
@@ -13,6 +13,8 @@ import {isLegacyProcedureDefBlock} from './interfaces/i_legacy_procedure_blocks.
 import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js';
 import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js';
 import {Msg} from './msg.js';
+import * as deprecation from './utils/deprecation.js';
+import type {BlockInfo, FlyoutItemInfo} from './utils/toolbox.js';
 import * as utilsXml from './utils/xml.js';
 import type {Workspace} from './workspace.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
@@ -84,6 +86,157 @@ export function allDeveloperVariables(workspace: Workspace): string[] {
  return Array.from(variables.values());
 }

+/**
+ * Internal wrapper that returns the contents of the variables category.
+ *
+ * @internal
+ * @param workspace The workspace to populate variable blocks for.
+ */
+export function internalFlyoutCategory(
+  workspace: WorkspaceSvg,
+): FlyoutItemInfo[] {
+  return flyoutCategory(workspace, false);
+}
+
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: true,
+): Element[];
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: false,
+): FlyoutItemInfo[];
+/**
+ * Construct the elements (blocks and button) required by the flyout for the
+ * variable category.
+ *
+ * @param workspace The workspace containing variables.
+ * @param useXml True to return the contents as XML, false to use JSON.
+ * @returns List of flyout contents as either XML or JSON.
+ */
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml = true,
+): Element[] | FlyoutItemInfo[] {
+  if (!Blocks['variables_set'] && !Blocks['variables_get']) {
+    console.warn(
+      'There are no variable blocks, but there is a variable category.',
+    );
+  }
+
+  if (useXml) {
+    deprecation.warn(
+      'The XML return value of Blockly.Variables.flyoutCategory()',
+      'v12',
+      'v13',
+      'the same method, but handle a return type of FlyoutItemInfo[] (JSON) instead.',
+    );
+    return xmlFlyoutCategory(workspace);
+  }
+
+  workspace.registerButtonCallback('CREATE_VARIABLE', function (button) {
+    createVariableButtonHandler(button.getTargetWorkspace());
+  });
+
+  return [
+    {
+      'kind': 'button',
+      'text': '%{BKY_NEW_VARIABLE}',
+      'callbackkey': 'CREATE_VARIABLE',
+    },
+    ...jsonFlyoutCategoryBlocks(
+      workspace,
+      workspace.getVariablesOfType(''),
+      true,
+    ),
+  ];
+}
+
+/**
+ * Returns the JSON definition for a variable field.
+ *
+ * @param variable The variable the field should reference.
+ * @returns JSON for a variable field.
+ */
+function generateVariableFieldJson(variable: IVariableModel<IVariableState>) {
+  return {
+    'VAR': {
+      'name': variable.getName(),
+      'type': variable.getType(),
+    },
+  };
+}
+
+/**
+ * Construct the blocks required by the flyout for the variable category.
+ *
+ * @internal
+ * @param workspace The workspace containing variables.
+ * @param variables List of variables to create blocks for.
+ * @param includeChangeBlocks True to include `change x by _` blocks.
+ * @param getterType The type of the variable getter block to generate.
+ * @param setterType The type of the variable setter block to generate.
+ * @returns JSON list of blocks.
+ */
+export function jsonFlyoutCategoryBlocks(
+  workspace: Workspace,
+  variables: IVariableModel<IVariableState>[],
+  includeChangeBlocks: boolean,
+  getterType = 'variables_get',
+  setterType = 'variables_set',
+): BlockInfo[] {
+  includeChangeBlocks &&= Blocks['math_change'];
+
+  const blocks = [];
+  const mostRecentVariable = variables.slice(-1)[0];
+  if (mostRecentVariable) {
+    // Show one setter block, with the name of the most recently created variable.
+    if (Blocks[setterType]) {
+      blocks.push({
+        kind: 'block',
+        type: setterType,
+        gap: includeChangeBlocks ? 8 : 24,
+        fields: generateVariableFieldJson(mostRecentVariable),
+      });
+    }
+
+    if (includeChangeBlocks) {
+      blocks.push({
+        'kind': 'block',
+        'type': 'math_change',
+        'gap': Blocks[getterType] ? 20 : 8,
+        'fields': generateVariableFieldJson(mostRecentVariable),
+        'inputs': {
+          'DELTA': {
+            'shadow': {
+              'type': 'math_number',
+              'fields': {
+                'NUM': 1,
+              },
+            },
+          },
+        },
+      });
+    }
+  }
+
+  if (Blocks[getterType]) {
+    // Show one getter block for each variable, sorted in alphabetical order.
+    blocks.push(
+      ...variables.sort(compareByName).map((variable) => {
+        return {
+          'kind': 'block',
+          'type': getterType,
+          'gap': 8,
+          'fields': generateVariableFieldJson(variable),
+        };
+      }),
+    );
+  }
+
+  return blocks;
+}
+
 /**
 * Construct the elements (blocks and button) required by the flyout for the
 * variable category.
@@ -91,12 +244,7 @@ export function allDeveloperVariables(workspace: Workspace): string[] {
 * @param workspace The workspace containing variables.
 * @returns Array of XML elements.
 */
-export function flyoutCategory(workspace: WorkspaceSvg): Element[] {
-  if (!Blocks['variables_set'] && !Blocks['variables_get']) {
-    console.warn(
-      'There are no variable blocks, but there is a variable category.',
-    );
-  }
+function xmlFlyoutCategory(workspace: WorkspaceSvg): Element[] {
  let xmlList = new Array<Element>();
  const button = document.createElement('button');
  button.setAttribute('text', '%{BKY_NEW_VARIABLE}');
--- a/core/variables_dynamic.ts
+++ b/core/variables_dynamic.ts
@@ -9,6 +9,8 @@
 import {Blocks} from './blocks.js';
 import type {FlyoutButton} from './flyout_button.js';
 import {Msg} from './msg.js';
+import * as deprecation from './utils/deprecation.js';
+import type {FlyoutItemInfo} from './utils/toolbox.js';
 import * as xml from './utils/xml.js';
 import * as Variables from './variables.js';
 import type {Workspace} from './workspace.js';
@@ -68,6 +70,92 @@ function colourButtonClickHandler(button: FlyoutButton) {
 // eslint-disable-next-line camelcase
 export const onCreateVariableButtonClick_Colour = colourButtonClickHandler;

+/**
+ * Internal wrapper that returns the contents of the dynamic variables category.
+ *
+ * @internal
+ * @param workspace The workspace to populate variable blocks for.
+ */
+export function internalFlyoutCategory(
+  workspace: WorkspaceSvg,
+): FlyoutItemInfo[] {
+  return flyoutCategory(workspace, false);
+}
+
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: true,
+): Element[];
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml: false,
+): FlyoutItemInfo[];
+/**
+ * Construct the elements (blocks and button) required by the flyout for the
+ * dynamic variables category.
+ *
+ * @param useXml True to return the contents as XML, false to use JSON.
+ * @returns List of flyout contents as either XML or JSON.
+ */
+export function flyoutCategory(
+  workspace: WorkspaceSvg,
+  useXml = true,
+): Element[] | FlyoutItemInfo[] {
+  if (!Blocks['variables_set_dynamic'] && !Blocks['variables_get_dynamic']) {
+    console.warn(
+      'There are no dynamic variable blocks, but there is a dynamic variable category.',
+    );
+  }
+
+  if (useXml) {
+    deprecation.warn(
+      'The XML return value of Blockly.VariablesDynamic.flyoutCategory()',
+      'v12',
+      'v13',
+      'the same method, but handle a return type of FlyoutItemInfo[] (JSON) instead.',
+    );
+    return xmlFlyoutCategory(workspace);
+  }
+
+  workspace.registerButtonCallback(
+    'CREATE_VARIABLE_STRING',
+    stringButtonClickHandler,
+  );
+  workspace.registerButtonCallback(
+    'CREATE_VARIABLE_NUMBER',
+    numberButtonClickHandler,
+  );
+  workspace.registerButtonCallback(
+    'CREATE_VARIABLE_COLOUR',
+    colourButtonClickHandler,
+  );
+
+  return [
+    {
+      'kind': 'button',
+      'text': Msg['NEW_STRING_VARIABLE'],
+      'callbackkey': 'CREATE_VARIABLE_STRING',
+    },
+    {
+      'kind': 'button',
+      'text': Msg['NEW_NUMBER_VARIABLE'],
+      'callbackkey': 'CREATE_VARIABLE_NUMBER',
+    },
+    {
+      'kind': 'button',
+      'text': Msg['NEW_COLOUR_VARIABLE'],
+      'callbackkey': 'CREATE_VARIABLE_COLOUR',
+    },
+    ...Variables.jsonFlyoutCategoryBlocks(
+      workspace,
+      workspace.getAllVariables(),
+      false,
+      'variables_get_dynamic',
+      'variables_set_dynamic',
+    ),
+  ];
+}
+
 /**
 * Construct the elements (blocks and button) required by the flyout for the
 * variable category.
@@ -75,12 +163,7 @@ export const onCreateVariableButtonClick_Colour = colourButtonClickHandler;
 * @param workspace The workspace containing variables.
 * @returns Array of XML elements.
 */
-export function flyoutCategory(workspace: WorkspaceSvg): Element[] {
-  if (!Blocks['variables_set_dynamic'] && !Blocks['variables_get_dynamic']) {
-    console.warn(
-      'There are no dynamic variable blocks, but there is a dynamic variable category.',
-    );
-  }
+function xmlFlyoutCategory(workspace: WorkspaceSvg): Element[] {
  let xmlList = new Array<Element>();
  let button = document.createElement('button');
  button.setAttribute('text', Msg['NEW_STRING_VARIABLE']);
--- a/core/workspace_svg.ts
+++ b/core/workspace_svg.ts
@@ -365,24 +365,24 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
    /** Manager in charge of markers and cursors. */
    this.markerManager = new MarkerManager(this);

-    if (Variables && Variables.flyoutCategory) {
+    if (Variables && Variables.internalFlyoutCategory) {
      this.registerToolboxCategoryCallback(
        Variables.CATEGORY_NAME,
-        Variables.flyoutCategory,
+        Variables.internalFlyoutCategory,
      );
    }

-    if (VariablesDynamic && VariablesDynamic.flyoutCategory) {
+    if (VariablesDynamic && VariablesDynamic.internalFlyoutCategory) {
      this.registerToolboxCategoryCallback(
        VariablesDynamic.CATEGORY_NAME,
-        VariablesDynamic.flyoutCategory,
+        VariablesDynamic.internalFlyoutCategory,
      );
    }

-    if (Procedures && Procedures.flyoutCategory) {
+    if (Procedures && Procedures.internalFlyoutCategory) {
      this.registerToolboxCategoryCallback(
        Procedures.CATEGORY_NAME,
-        Procedures.flyoutCategory,
+        Procedures.internalFlyoutCategory,
      );
      this.addChangeListener(Procedures.mutatorOpenListener);
    }
--- a/tests/mocha/contextmenu_test.js
+++ b/tests/mocha/contextmenu_test.js
@@ -6,7 +6,6 @@

 import {callbackFactory} from '../../build/src/core/contextmenu.js';
 import * as xmlUtils from '../../build/src/core/utils/xml.js';
-import * as Variables from '../../build/src/core/variables.js';
 import {assert} from '../../node_modules/chai/chai.js';
 import {
  sharedTestSetup,
@@ -32,9 +31,13 @@ suite('Context Menu', function () {
    });

    test('callback with xml state creates block', function () {
-      const xmlField = Variables.generateVariableFieldDom(
-        this.forLoopBlock.getField('VAR').getVariable(),
-      );
+      const variable = this.forLoopBlock.getField('VAR').getVariable();
+      const xmlField = document.createElement('field');
+      xmlField.setAttribute('name', 'VAR');
+      xmlField.setAttribute('id', variable.getId());
+      xmlField.setAttribute('variabletype', variable.getType());
+      xmlField.textContent = variable.getName();
+
      const xmlBlock = xmlUtils.createElement('block');
      xmlBlock.setAttribute('type', 'variables_get');
      xmlBlock.appendChild(xmlField);
--- a/tests/mocha/xml_test.js
+++ b/tests/mocha/xml_test.js
@@ -531,28 +531,6 @@ suite('XML', function () {
    teardown(function () {
      workspaceTeardown.call(this, this.workspace);
    });
-    suite('Dynamic Category Blocks', function () {
-      test('Untyped Variables', function () {
-        this.workspace.createVariable('name1', '', 'id1');
-        const blocksArray = Blockly.Variables.flyoutCategoryBlocks(
-          this.workspace,
-        );
-        for (let i = 0, xml; (xml = blocksArray[i]); i++) {
-          Blockly.Xml.domToBlock(xml, this.workspace);
-        }
-      });
-      test('Typed Variables', function () {
-        this.workspace.createVariable('name1', 'String', 'id1');
-        this.workspace.createVariable('name2', 'Number', 'id2');
-        this.workspace.createVariable('name3', 'Colour', 'id3');
-        const blocksArray = Blockly.VariablesDynamic.flyoutCategoryBlocks(
-          this.workspace,
-        );
-        for (let i = 0, xml; (xml = blocksArray[i]); i++) {
-          Blockly.Xml.domToBlock(xml, this.workspace);
-        }
-      });
-    });
    suite('Comments', function () {
      suite('Headless', function () {
        test('Text', function () {
@@ -910,42 +888,4 @@ suite('XML', function () {
      });
    });
  });
-  suite('generateVariableFieldDom', function () {
-    test('Case Sensitive', function () {
-      const varId = 'testId';
-      const type = 'testType';
-      const name = 'testName';
-
-      const mockVariableModel = {
-        type: type,
-        name: name,
-        getId: function () {
-          return varId;
-        },
-        getName: function () {
-          return name;
-        },
-        getType: function () {
-          return type;
-        },
-      };
-
-      const generatedXml = Blockly.Xml.domToText(
-        Blockly.Variables.generateVariableFieldDom(mockVariableModel),
-      );
-      const expectedXml =
-        '<field xmlns="https://developers.google.com/blockly/xml"' +
-        ' name="VAR"' +
-        ' id="' +
-        varId +
-        '"' +
-        ' variabletype="' +
-        type +
-        '"' +
-        '>' +
-        name +
-        '</field>';
-      assert.equal(generatedXml, expectedXml);
-    });
-  });
 });