From 23fb76b9f21987c849b92973f2ea002ccb9f4c1c Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Fri, 6 Jan 2023 23:32:04 +0000 Subject: [PATCH] fix: undoing and redoing parameter events (#6721) * fix: undoing and redoing parameters creating dupe parameters * chore: add tests for undoing and redoing adding parameters * fix: undoing and redoing renaming parameters * chore: change tests to tick clock * chore: format * chore: add tests for deleting procedure parameters * chore: fix tests * chore: unskip tests * chore: fix return type of saveExtraState * chore: increase mocha timeout --- blocks/procedures.js | 52 +-- core/procedures/observable_parameter_model.ts | 5 +- core/serialization/procedures.ts | 4 +- tests/mocha/blocks/procedures_test.js | 384 +++++++++++++----- tests/mocha/jso_serialization_test.js | 2 +- tests/mocha/webdriver.js | 2 +- 6 files changed, 316 insertions(+), 133 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index c91364f81..419631d52 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -528,11 +528,13 @@ const procedureDefMutator = { if (opt_paramIds) { container.setAttribute('name', this.getFieldValue('NAME')); } - for (let i = 0; i < this.argumentVarModels_.length; i++) { + + const params = this.getProcedureModel().getParameters(); + for (let i = 0; i < params.length; i++) { const parameter = xmlUtils.createElement('arg'); - const argModel = this.argumentVarModels_[i]; - parameter.setAttribute('name', argModel.name); - parameter.setAttribute('varid', argModel.getId()); + const varModel = params[i].getVariableModel(); + parameter.setAttribute('name', varModel.name); + parameter.setAttribute('varid', varModel.getId()); if (opt_paramIds && this.paramIds_) { parameter.setAttribute('paramId', this.paramIds_[i]); } @@ -556,10 +558,10 @@ const procedureDefMutator = { for (let i = 0; i < xmlElement.childNodes.length; i++) { const node = xmlElement.childNodes[i]; if (node.nodeName.toLowerCase() !== 'arg') continue; + const varId = node.getAttribute('varid'); this.getProcedureModel().insertParameter( new ObservableParameterModel( - this.workspace, node.getAttribute('name'), - node.getAttribute('varid')), + this.workspace, node.getAttribute('name'), undefined, varId), i); } @@ -596,20 +598,20 @@ const procedureDefMutator = { * parameters and statements. */ saveExtraState: function() { - if (!this.argumentVarModels_.length && this.hasStatements_) { - return null; - } + const params = this.getProcedureModel().getParameters(); + if (!params.length && this.hasStatements_) return null; + const state = Object.create(null); - if (this.argumentVarModels_.length) { - state['params'] = []; - for (let i = 0; i < this.argumentVarModels_.length; i++) { - state['params'].push({ - // We don't need to serialize the name, but just in case we decide - // to separate params from variables. - 'name': this.argumentVarModels_[i].name, - 'id': this.argumentVarModels_[i].getId(), - }); - } + if (params.length) { + state['params'] = params.map((p) => { + return { + 'name': p.getName(), + 'id': p.getVariableModel().getId(), + // Ideally this would be id, and the other would be varId, + // but backwards compatibility :/ + 'paramId': p.getId(), + }; + }); } if (!this.hasStatements_) { state['hasStatements'] = false; @@ -625,10 +627,9 @@ const procedureDefMutator = { loadExtraState: function(state) { if (state['params']) { for (let i = 0; i < state['params'].length; i++) { - const param = state['params'][i]; + const {name, id, paramId} = state['params'][i]; this.getProcedureModel().insertParameter( - new ObservableParameterModel(this.workspace, param.name, param.id), - i); + new ObservableParameterModel(this.workspace, name, paramId, id), i); } } @@ -719,7 +720,7 @@ const procedureDefMutator = { Procedures.mutateCallers(this); const model = this.getProcedureModel(); - const count = this.getProcedureModel().getParameters().length; + const count = model.getParameters().length; for (let i = count - 1; i >= 0; i--) { model.deleteParameter(i); } @@ -935,6 +936,7 @@ const validateProcedureParamMixin = { this.createdVariables_.push(model); } } + return varName; }, @@ -1210,6 +1212,10 @@ const procedureCallerUpdateShapeMixin = { this.updateName_(); this.updateEnabled_(); this.updateParameters_(); + + // Temporarily maintained for code that relies on arguments_ + this.arguments_ = + this.getProcedureModel().getParameters().map((p) => p.getName()); }, /** diff --git a/core/procedures/observable_parameter_model.ts b/core/procedures/observable_parameter_model.ts index 91f624f95..e5adb53ba 100644 --- a/core/procedures/observable_parameter_model.ts +++ b/core/procedures/observable_parameter_model.ts @@ -22,10 +22,11 @@ export class ObservableParameterModel implements IParameterModel { private procedureModel: IProcedureModel|null = null; constructor( - private readonly workspace: Workspace, name: string, id?: string) { + private readonly workspace: Workspace, name: string, id?: string, + varId?: string) { this.id = id ?? genUid(); this.variable = this.workspace.getVariable(name) ?? - workspace.createVariable(name, '', id); + workspace.createVariable(name, '', varId); } /** diff --git a/core/serialization/procedures.ts b/core/serialization/procedures.ts index 05f70b03b..8e18dda66 100644 --- a/core/serialization/procedures.ts +++ b/core/serialization/procedures.ts @@ -10,7 +10,7 @@ import type {ISerializer} from '../interfaces/i_serializer.js'; import {ObservableProcedureModel} from '../procedures/observable_procedure_model.js'; import {ObservableParameterModel} from '../procedures/observable_parameter_model.js'; import * as priorities from './priorities.js'; -// import * as serializationRegistry from './registry.js'; +import * as serializationRegistry from './registry.js'; import type {Workspace} from '../workspace.js'; @@ -171,4 +171,4 @@ export class ProcedureSerializer `); Blockly.Xml.domToWorkspace(xml, this.workspace); + this.clock.runAll(); + assertDefAndCallBlocks( this.workspace, ['unnamed'], ['unnamed2'], false); }); @@ -1521,6 +1688,8 @@ suite('Procedures', function() { `); Blockly.Xml.domToWorkspace(xml, this.workspace); + this.clock.runAll(); + assertDefAndCallBlocks( this.workspace, ['unnamed2'], ['unnamed'], false); }); @@ -1539,8 +1708,8 @@ suite('Procedures', function() { test('callnoreturn and callreturn (no def in xml)', function() { const xml = Blockly.Xml.textToDom(` - - + + `); Blockly.Xml.domToWorkspace(xml, this.workspace); this.clock.runAll(); @@ -1880,15 +2049,18 @@ suite('Procedures', function() { suite('Mutation', function() { setup(function() { - this.defBlock = this.workspace.newBlock(testSuite.defType); - this.defBlock.setFieldValue('proc name', 'NAME'); - this.callBlock = this.workspace.newBlock(testSuite.callType); - this.callBlock.setFieldValue('proc name', 'NAME'); - this.findParentStub = sinon.stub(Blockly.Mutator, 'findParentWs') - .returns(this.workspace); - }); - teardown(function() { - this.findParentStub.restore(); + this.defBlock = Blockly.serialization.blocks.append({ + 'type': testSuite.defType, + 'fields': { + 'NAME': 'proc name', + }, + }, this.workspace); + this.callBlock = Blockly.serialization.blocks.append({ + 'type': testSuite.callType, + 'extraState': { + 'name': 'proc name', + }, + }, this.workspace); }); suite('Composition', function() { suite('Statements', function() { @@ -1935,11 +2107,9 @@ suite('Procedures', function() { }); suite('Untyped Arguments', function() { function createMutator(argArray) { - this.mutatorWorkspace = new Blockly.Workspace( - new Blockly.Options({ - parentWorkspace: this.workspace, - })); - this.containerBlock = this.defBlock.decompose(this.mutatorWorkspace); + this.defBlock.mutator.setVisible(true); + this.mutatorWorkspace = this.defBlock.mutator.getWorkspace(); + this.containerBlock = this.mutatorWorkspace.getTopBlocks()[0]; this.connection = this.containerBlock.getInput('STACK').connection; for (let i = 0; i < argArray.length; i++) { this.argBlock = this.mutatorWorkspace.newBlock('procedures_mutatorarg'); @@ -1947,14 +2117,20 @@ suite('Procedures', function() { this.connection.connect(this.argBlock.previousConnection); this.connection = this.argBlock.nextConnection; } - this.defBlock.compose(this.containerBlock); + this.clock.runAll(); } function assertArgs(argArray) { - chai.assert.equal(this.defBlock.arguments_.length, argArray.length); + chai.assert.equal( + this.defBlock.arguments_.length, + argArray.length, + 'Expected the def to have the right number of arguments'); for (let i = 0; i < argArray.length; i++) { chai.assert.equal(this.defBlock.arguments_[i], argArray[i]); } - chai.assert.equal(this.callBlock.arguments_.length, argArray.length); + chai.assert.equal( + this.callBlock.arguments_.length, + argArray.length, + 'Expected the call to have the right number of arguments'); for (let i = 0; i < argArray.length; i++) { chai.assert.equal(this.callBlock.arguments_[i], argArray[i]); } diff --git a/tests/mocha/jso_serialization_test.js b/tests/mocha/jso_serialization_test.js index 10d15560b..1dd86ab59 100644 --- a/tests/mocha/jso_serialization_test.js +++ b/tests/mocha/jso_serialization_test.js @@ -795,7 +795,7 @@ suite('JSO Serialization', function() { }); }); - suite.skip('Procedures', function() { + suite('Procedures', function() { class MockProcedureModel { constructor() { this.id = Blockly.utils.idGenerator.genUid(); diff --git a/tests/mocha/webdriver.js b/tests/mocha/webdriver.js index e7eb99bc7..e46921234 100644 --- a/tests/mocha/webdriver.js +++ b/tests/mocha/webdriver.js @@ -53,7 +53,7 @@ async function runMochaTestsInBrowser() { const text = await elem.getAttribute('tests_failed'); return text !== 'unset'; }, { - timeout: 50000, + timeout: 100000, }); const elem = await browser.$('#failureCount');