chore: Use JSON objects for context menu callbackFactory (#7382)

* chore: use json object for context callbackFactory * Add test file for context menu callback function.
google · Sep 8, 2023 · 1b2e912 · 1b2e912
1 parent 0d2879b
commit 1b2e912
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 52 deletions.
diff --git a/blocks/loops.ts b/blocks/loops.ts
@@ -15,8 +15,6 @@ import type {
 } from '../core/contextmenu_registry.js';
 import * as Events from '../core/events/events.js';
 import * as Extensions from '../core/extensions.js';
-import * as Variables from '../core/variables.js';
-import * as xmlUtils from '../core/utils/xml.js';
 import {Msg} from '../core/msg.js';
 import {
   createBlockDefinitionsFromJsonArray,
@@ -272,15 +270,15 @@ const CUSTOM_CONTEXT_MENU_CREATE_VARIABLES_GET_MIXIN = {
     const variable = varField.getVariable()!;
     const varName = variable.name;
     if (!this.isCollapsed() && varName !== null) {
-      const xmlField = Variables.generateVariableFieldDom(variable);
-      const xmlBlock = xmlUtils.createElement('block');
-      xmlBlock.setAttribute('type', 'variables_get');
-      xmlBlock.appendChild(xmlField);
+      const getVarBlockState = {
+        type: 'variables_get',
+        fields: {VAR: varField.saveState(true)},
+      };
 
       options.push({
         enabled: true,
         text: Msg['VARIABLES_SET_CREATE_GET'].replace('%1', varName),
-        callback: ContextMenu.callbackFactory(this, xmlBlock),
+        callback: ContextMenu.callbackFactory(this, getVarBlockState),
       });
     }
   },

diff --git a/blocks/procedures.ts b/blocks/procedures.ts
@@ -454,34 +454,30 @@ const PROCEDURE_DEF_COMMON = {
     }
     // Add option to create caller.
     const name = this.getFieldValue('NAME');
-    const xmlMutation = xmlUtils.createElement('mutation');
-    xmlMutation.setAttribute('name', name);
-    for (let i = 0; i < this.arguments_.length; i++) {
-      const xmlArg = xmlUtils.createElement('arg');
-      xmlArg.setAttribute('name', this.arguments_[i]);
-      xmlMutation.appendChild(xmlArg);
-    }
-    const xmlBlock = xmlUtils.createElement('block');
-    xmlBlock.setAttribute('type', (this as AnyDuringMigration).callType_);
-    xmlBlock.appendChild(xmlMutation);
+    const callProcedureBlockState = {
+      type: (this as AnyDuringMigration).callType_,
+      extraState: {name: name, params: this.arguments_},
+    };
     options.push({
       enabled: true,
       text: Msg['PROCEDURES_CREATE_DO'].replace('%1', name),
-      callback: ContextMenu.callbackFactory(this, xmlBlock),
+      callback: ContextMenu.callbackFactory(this, callProcedureBlockState),
     });
 
     // Add options to create getters for each parameter.
     if (!this.isCollapsed()) {
       for (let i = 0; i < this.argumentVarModels_.length; i++) {
         const argVar = this.argumentVarModels_[i];
-        const argXmlField = Variables.generateVariableFieldDom(argVar);
-        const argXmlBlock = xmlUtils.createElement('block');
-        argXmlBlock.setAttribute('type', 'variables_get');
-        argXmlBlock.appendChild(argXmlField);
+        const getVarBlockState = {
+          type: 'variables_get',
+          fields: {
+            VAR: {name: argVar.name, id: argVar.getId(), type: argVar.type},
+          },
+        };
         options.push({
           enabled: true,
           text: Msg['VARIABLES_SET_CREATE_GET'].replace('%1', argVar.name),
-          callback: ContextMenu.callbackFactory(this, argXmlBlock),
+          callback: ContextMenu.callbackFactory(this, getVarBlockState),
         });
       }
     }

diff --git a/blocks/variables.ts b/blocks/variables.ts
@@ -9,7 +9,6 @@
 import * as ContextMenu from '../core/contextmenu.js';
 import * as Extensions from '../core/extensions.js';
 import * as Variables from '../core/variables.js';
-import * as xmlUtils from '../core/utils/xml.js';
 import type {Block} from '../core/block.js';
 import type {
   ContextMenuOption,
@@ -102,18 +101,16 @@ const CUSTOM_CONTEXT_MENU_VARIABLE_GETTER_SETTER_MIXIN = {
         contextMenuMsg = Msg['VARIABLES_SET_CREATE_GET'];
       }
 
-      const name = this.getField('VAR')!.getText();
-      const xmlField = xmlUtils.createElement('field');
-      xmlField.setAttribute('name', 'VAR');
-      xmlField.appendChild(xmlUtils.createTextNode(name));
-      const xmlBlock = xmlUtils.createElement('block');
-      xmlBlock.setAttribute('type', oppositeType);
-      xmlBlock.appendChild(xmlField);
+      const varField = this.getField('VAR')!;
+      const newVarBlockState = {
+        type: oppositeType,
+        fields: {VAR: varField.saveState(true)},
+      };
 
       options.push({
         enabled: this.workspace.remainingCapacity() > 0,
-        text: contextMenuMsg.replace('%1', name),
-        callback: ContextMenu.callbackFactory(this, xmlBlock),
+        text: contextMenuMsg.replace('%1', varField.getText()),
+        callback: ContextMenu.callbackFactory(this, newVarBlockState),
       });
       // Getter blocks have the option to rename or delete that variable.
     } else {

diff --git a/blocks/variables_dynamic.ts b/blocks/variables_dynamic.ts
@@ -9,7 +9,6 @@
 import * as ContextMenu from '../core/contextmenu.js';
 import * as Extensions from '../core/extensions.js';
 import * as Variables from '../core/variables.js';
-import * as xml from '../core/utils/xml.js';
 import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
 import type {Block} from '../core/block.js';
 import type {
@@ -95,9 +94,6 @@ const CUSTOM_CONTEXT_MENU_VARIABLE_GETTER_SETTER_MIXIN = {
     if (!this.isInFlyout) {
       let oppositeType;
       let contextMenuMsg;
-      const id = this.getFieldValue('VAR');
-      const variableModel = this.workspace.getVariableById(id);
-      const varType = variableModel!.type;
       if (this.type === 'variables_get_dynamic') {
         oppositeType = 'variables_set_dynamic';
         contextMenuMsg = Msg['VARIABLES_GET_CREATE_SET'];
@@ -106,19 +102,16 @@ const CUSTOM_CONTEXT_MENU_VARIABLE_GETTER_SETTER_MIXIN = {
         contextMenuMsg = Msg['VARIABLES_SET_CREATE_GET'];
       }
 
-      const name = this.getField('VAR')!.getText();
-      const xmlField = xml.createElement('field');
-      xmlField.setAttribute('name', 'VAR');
-      xmlField.setAttribute('variabletype', varType);
-      xmlField.appendChild(xml.createTextNode(name));
-      const xmlBlock = xml.createElement('block');
-      xmlBlock.setAttribute('type', oppositeType);
-      xmlBlock.appendChild(xmlField);
+      const varField = this.getField('VAR')!;
+      const newVarBlockState = {
+        type: oppositeType,
+        fields: {VAR: varField.saveState(true)},
+      };
 
       options.push({
         enabled: this.workspace.remainingCapacity() > 0,
-        text: contextMenuMsg.replace('%1', name),
-        callback: ContextMenu.callbackFactory(this, xmlBlock),
+        text: contextMenuMsg.replace('%1', varField.getText()),
+        callback: ContextMenu.callbackFactory(this, newVarBlockState),
       });
     } else {
       if (

diff --git a/core/contextmenu.ts b/core/contextmenu.ts
@@ -23,6 +23,7 @@ import {Msg} from './msg.js';
 import * as aria from './utils/aria.js';
 import {Coordinate} from './utils/coordinate.js';
 import {Rect} from './utils/rect.js';
+import * as serializationBlocks from './serialization/blocks.js';
 import * as svgMath from './utils/svg_math.js';
 import * as WidgetDiv from './widgetdiv.js';
 import {WorkspaceCommentSvg} from './workspace_comment_svg.js';
@@ -223,18 +224,28 @@ export function dispose() {
 
 /**
  * Create a callback function that creates and configures a block,
- *   then places the new block next to the original.
+ *   then places the new block next to the original and returns it.
  *
  * @param block Original block.
- * @param xml XML representation of new block.
+ * @param state XML or JSON object representation of the new block.
  * @returns Function that creates a block.
  */
-export function callbackFactory(block: Block, xml: Element): () => void {
+export function callbackFactory(
+  block: Block,
+  state: Element | serializationBlocks.State,
+): () => BlockSvg {
   return () => {
     eventUtils.disable();
-    let newBlock;
+    let newBlock: BlockSvg;
     try {
-      newBlock = Xml.domToBlockInternal(xml, block.workspace!) as BlockSvg;
+      if (state instanceof Element) {
+        newBlock = Xml.domToBlockInternal(state, block.workspace!) as BlockSvg;
+      } else {
+        newBlock = serializationBlocks.appendInternal(
+          state,
+          block.workspace,
+        ) as BlockSvg;
+      }
       // Move the new block next to the old block.
       const xy = block.getRelativeToSurfaceXY();
       if (block.RTL) {
@@ -251,6 +262,7 @@ export function callbackFactory(block: Block, xml: Element): () => void {
       eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(newBlock));
     }
     newBlock.select();
+    return newBlock;
   };
 }
 

diff --git a/tests/mocha/contextmenu_test.js b/tests/mocha/contextmenu_test.js
@@ -0,0 +1,70 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import {
+  sharedTestSetup,
+  sharedTestTeardown,
+} from './test_helpers/setup_teardown.js';
+
+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';
+
+suite('Context Menu', function () {
+  setup(function () {
+    sharedTestSetup.call(this);
+
+    // Creates a WorkspaceSVG
+    const toolbox = document.getElementById('toolbox-categories');
+    this.workspace = Blockly.inject('blocklyDiv', {toolbox: toolbox});
+  });
+
+  teardown(function () {
+    sharedTestTeardown.call(this);
+  });
+
+  suite('Callback Factory', function () {
+    setup(function () {
+      this.forLoopBlock = this.workspace.newBlock('controls_for');
+    });
+
+    test('callback with xml state creates block', function () {
+      const xmlField = Variables.generateVariableFieldDom(
+        this.forLoopBlock.getField('VAR').getVariable(),
+      );
+      const xmlBlock = xmlUtils.createElement('block');
+      xmlBlock.setAttribute('type', 'variables_get');
+      xmlBlock.appendChild(xmlField);
+
+      const callback = callbackFactory(this.forLoopBlock, xmlBlock);
+      const getVarBlock = callback();
+
+      chai.assert.equal(getVarBlock.type, 'variables_get');
+      chai.assert.equal(getVarBlock.workspace, this.forLoopBlock.workspace);
+      chai.assert.equal(
+        getVarBlock.getField('VAR').getVariable().getId(),
+        this.forLoopBlock.getField('VAR').getVariable().getId(),
+      );
+    });
+
+    test('callback with json state creates block', function () {
+      const jsonState = {
+        type: 'variables_get',
+        fields: {VAR: this.forLoopBlock.getField('VAR').saveState(true)},
+      };
+
+      const callback = callbackFactory(this.forLoopBlock, jsonState);
+      const getVarBlock = callback();
+
+      chai.assert.equal(getVarBlock.type, 'variables_get');
+      chai.assert.equal(getVarBlock.workspace, this.forLoopBlock.workspace);
+      chai.assert.equal(
+        getVarBlock.getField('VAR').getVariable().getId(),
+        this.forLoopBlock.getField('VAR').getVariable().getId(),
+      );
+    });
+  });
+});
diff --git a/tests/mocha/index.html b/tests/mocha/index.html
@@ -50,6 +50,7 @@
       import './connection_db_test.js';
       import './connection_test.js';
       import './contextmenu_items_test.js';
+      import './contextmenu_test.js';
       import './cursor_test.js';
       import './dropdowndiv_test.js';
       import './event_test.js';