From 1b2e91246e49ee234391c10618a652a31f9b756e Mon Sep 17 00:00:00 2001 From: John Nesky Date: Fri, 8 Sep 2023 16:16:59 -0700 Subject: [PATCH] chore: Use JSON objects for context menu callbackFactory (#7382) * chore: use json object for context callbackFactory * Add test file for context menu callback function. --- blocks/loops.ts | 12 +++--- blocks/procedures.ts | 28 ++++++------- blocks/variables.ts | 17 ++++---- blocks/variables_dynamic.ts | 21 ++++------ core/contextmenu.ts | 22 ++++++++--- tests/mocha/contextmenu_test.js | 70 +++++++++++++++++++++++++++++++++ tests/mocha/index.html | 1 + 7 files changed, 119 insertions(+), 52 deletions(-) create mode 100644 tests/mocha/contextmenu_test.js diff --git a/blocks/loops.ts b/blocks/loops.ts index 80e2bf305a6..e70fdae313b 100644 --- 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 index 058eb0cd324..46109fa27b6 100644 --- 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 index 81c91112c9f..8ac038fb2ce 100644 --- 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 index 3605339f6d0..e74cae423ab 100644 --- 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 index d2157be1fa3..78431c93f3a 100644 --- 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 new file mode 100644 index 00000000000..7af9c79186d --- /dev/null +++ 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 index 160c74b0b5a..1804a750481 100644 --- 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';