Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: procedure blocks update models #6672

Merged
merged 12 commits into from
Jan 5, 2023
+167 −222
Merged

feat: procedure blocks update models #6672

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1802f5e
feat: procedure blocks have models
BeksOmega Nov 21, 2022
66b29ec
feat: add updating the name of the model
BeksOmega Nov 21, 2022
2ed1635
feat: add procedure defs updating the model enabled state
BeksOmega Nov 21, 2022
91abebc
feat: add procedure blocks updating parameters in the model
BeksOmega Nov 22, 2022
4c8c0f1
fix: add disposing of the procedure model
BeksOmega Nov 22, 2022
7679699
chore: updates test to check for identity of parameters
BeksOmega Dec 7, 2022
e15cded
chore: move statement handling into setStatement
BeksOmega Dec 7, 2022
d5ddf86
fix: make parameter IDs consistent
BeksOmega Dec 9, 2022
dc25b41
chore: un-only tests
BeksOmega Dec 9, 2022
63f9f23
chore: fixup tests
BeksOmega Dec 14, 2022
b3c8654
chore: revert validator to use Procedures.rename
BeksOmega Jan 5, 2023
616b4be
chore: cleanup typo
BeksOmega Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 111 additions & 61 deletions blocks/procedures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@ const {Block} = goog.requireType('Blockly.Block');
// TODO (6248): Properly import the BlockDefinition type.
/* eslint-disable-next-line no-unused-vars */
const BlockDefinition = Object;
const {ObservableProcedureModel} = goog.require('Blockly.procedures.ObservableProcedureModel');
const {ObservableParameterModel} = goog.require('Blockly.procedures.ObservableParameterModel');
const {config} = goog.require('Blockly.config');
/* eslint-disable-next-line no-unused-vars */
const {FieldLabel} = goog.require('Blockly.FieldLabel');
const {Msg} = goog.require('Blockly.Msg');
const {Mutator} = goog.require('Blockly.Mutator');
const {Names} = goog.require('Blockly.Names');
const serialization = goog.require('Blockly.serialization');
/* eslint-disable-next-line no-unused-vars */
const {VariableModel} = goog.requireType('Blockly.VariableModel');
/* eslint-disable-next-line no-unused-vars */
Expand Down Expand Up @@ -85,7 +88,8 @@ const blocks = createBlockDefinitionsFromJsonArray([
'procedure_def_var_mixin',
'procedure_def_update_shape_mixin',
'procedure_def_context_menu_mixin',
'procedure_def_get_legal_name_helper',
'procedure_def_onchange_mixin',
'procedure_def_validator_helper',
'procedure_defnoreturn_get_caller_block_mixin',
'procedure_defnoreturn_set_comment_helper',
],
Expand Down Expand Up @@ -160,7 +164,8 @@ const blocks = createBlockDefinitionsFromJsonArray([
'procedure_def_var_mixin',
'procedure_def_update_shape_mixin',
'procedure_def_context_menu_mixin',
'procedure_def_get_legal_name_helper',
'procedure_def_onchange_mixin',
'procedure_def_validator_helper',
'procedure_defreturn_get_caller_block_mixin',
'procedure_defreturn_set_comment_helper',
],
Expand Down Expand Up @@ -274,6 +279,12 @@ exports.blocks = blocks;
/** @this {Block} */
const procedureDefGetDefMixin = function() {
const mixin = {
model: null,

getProcedureModel() {
return this.model;
},

/**
* Return all variables referenced by this block.
* @return {!Array<string>} List of variable names.
Expand All @@ -282,6 +293,7 @@ const procedureDefGetDefMixin = function() {
getVars: function() {
return this.arguments_;
},

/**
* Return all variables referenced by this block.
* @return {!Array<!VariableModel>} List of variable models.
Expand All @@ -290,8 +302,20 @@ const procedureDefGetDefMixin = function() {
getVarModels: function() {
return this.argumentVarModels_;
},

/**
* Disposes of the data model for this procedure block when the block is
* disposed.
*/
destroy: function() {
this.workspace.getProcedureMap().delete(this.getProcedureModel().getId());
},
};

mixin.model = new ObservableProcedureModel(
this.workspace, this.getFieldValue('NAME'));
this.workspace.getProcedureMap().add(mixin.model);

this.mixin(mixin, true);
};
// Using register instead of registerMixin to avoid triggering warnings about
Expand Down Expand Up @@ -382,7 +406,18 @@ const procedureDefUpdateShapeMixin = {
if (this.getInput('RETURN')) {
this.moveInputBefore('STACK', 'RETURN');
}
// Restore the stack, if one was saved.
Mutator.reconnect(this.statementConnection_, this, 'STACK');
this.statementConnection_ = null;
} else {
// Save the stack, then disconnect it.
const stackConnection = this.getInput('STACK').connection;
this.statementConnection_ = stackConnection.targetConnection;
if (this.statementConnection_) {
const stackBlock = stackConnection.targetBlock();
stackBlock.unplug();
stackBlock.bumpNeighbours();
}
this.removeInput('STACK', true);
}
this.hasStatements_ = hasStatements;
Expand Down Expand Up @@ -435,13 +470,30 @@ Extensions.registerMixin(
'procedure_def_update_shape_mixin', procedureDefUpdateShapeMixin);

/** @this {Block} */
const procedureDefGetLegalNameHelper = function() {
const procedureDefValidatorHelper = function() {
const nameField = this.getField('NAME');
nameField.setValue(Procedures.findLegalName('', this));
nameField.setValidator(Procedures.rename);

/**
* Validates the input to the procedure name field.
* @param {string} newName The new name of the procedure.
* @return {string} The validated/legal name of the procedure.
* @this {Field}
*/
const validator = function(newName) {
// TODO: Remove call the rename?
Procedures.rename.call(this, newName);

const sourceBlock = this.getSourceBlock();
const legalName = Procedures.findLegalName(newName.trim(), sourceBlock);
sourceBlock.model.setName(legalName);
return legalName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Procedures.rename appears to already be finding and returning a legal name; additionally, it seems like setting the name on the model should be done by Procedures.rename in case it gets called through some other mechanism. Would it make sense to move that line into Procedures.rename() and revert this to just calling Procedures.rename directly like it was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! When I was writing this I wasn't sure if we wanted everything to be 100% backwards compatible or not. If not, I was planning on removing rename so I duplicated the logic. But given we do want it to be backwards compatible, this is unnecessary.

Skipped one test that was breaking because of the isProcedureBlock check I added. Will re-enable in the next PR once procedure definitions have a doProcedureUpdate method.

};

nameField.setValidator(validator);
};
Extensions.register(
'procedure_def_get_legal_name_helper', procedureDefGetLegalNameHelper);
'procedure_def_validator_helper', procedureDefValidatorHelper);

const procedureDefMutator = {
arguments_: [],
Expand Down Expand Up @@ -570,50 +622,39 @@ const procedureDefMutator = {
* @this {Block}
*/
decompose: function(workspace) {
/*
* Creates the following XML:
* <block type="procedures_mutatorcontainer">
* <statement name="STACK">
* <block type="procedures_mutatorarg">
* <field name="NAME">arg1_name</field>
* <next>etc...</next>
* </block>
* </statement>
* </block>
*/

const containerBlockNode = xmlUtils.createElement('block');
containerBlockNode.setAttribute('type', 'procedures_mutatorcontainer');
const statementNode = xmlUtils.createElement('statement');
statementNode.setAttribute('name', 'STACK');
containerBlockNode.appendChild(statementNode);
const containerBlockDef = {
'type': 'procedures_mutatorcontainer',
'inputs': {
'STACK': {},
},
};

let node = statementNode;
for (let i = 0; i < this.arguments_.length; i++) {
const argBlockNode = xmlUtils.createElement('block');
argBlockNode.setAttribute('type', 'procedures_mutatorarg');
const fieldNode = xmlUtils.createElement('field');
fieldNode.setAttribute('name', 'NAME');
const argumentName = xmlUtils.createTextNode(this.arguments_[i]);
fieldNode.appendChild(argumentName);
argBlockNode.appendChild(fieldNode);
const nextNode = xmlUtils.createElement('next');
argBlockNode.appendChild(nextNode);

node.appendChild(argBlockNode);
node = nextNode;
let connDef = containerBlockDef['inputs']['STACK'];
for (const param of this.getProcedureModel().getParameters()) {
connDef['block'] = {
'type': 'procedures_mutatorarg',
'id': param.getId(),
'fields': {
'NAME': param.getName(),
},
'next': {},
};
connDef = connDef['block']['next'];
}

const containerBlock = Xml.domToBlock(containerBlockNode, workspace);

const containerBlock =
serialization.blocks.append(
containerBlockDef, workspace, {recordUndo: false});

if (this.type === 'procedures_defreturn') {
containerBlock.setFieldValue(this.hasStatements_, 'STATEMENTS');
} else {
containerBlock.removeInput('STATEMENT_INPUT');
}

// Initialize procedure's callers with blank IDs.
// Update callers (called for backwards compatibility).
Procedures.mutateCallers(this);

return containerBlock;
},

Expand All @@ -627,11 +668,14 @@ const procedureDefMutator = {
this.arguments_ = [];
this.paramIds_ = [];
this.argumentVarModels_ = [];

// TODO: Remove old data handling logic?
let paramBlock = containerBlock.getInputTargetBlock('STACK');
while (paramBlock && !paramBlock.isInsertionMarker()) {
const varName = paramBlock.getFieldValue('NAME');
this.arguments_.push(varName);
const variable = this.workspace.getVariable(varName, '');
const variable = Variables.getOrCreateVariablePackage(
this.workspace, null, varName, '');
this.argumentVarModels_.push(variable);

this.paramIds_.push(paramBlock.id);
Expand All @@ -640,29 +684,25 @@ const procedureDefMutator = {
}
this.updateParams_();
Procedures.mutateCallers(this);
for (let i = this.model.getParameters().length; i >= 0; i--) {
this.model.deleteParameter(i);
}

let i = 0;
paramBlock = containerBlock.getInputTargetBlock('STACK');
while (paramBlock && !paramBlock.isInsertionMarker()) {
this.model.insertParameter(
new ObservableParameterModel(
this.workspace, paramBlock.getFieldValue('NAME'), paramBlock.id),
i);
paramBlock =
paramBlock.nextConnection && paramBlock.nextConnection.targetBlock();
i++;
}

// Show/hide the statement input.
let hasStatements = containerBlock.getFieldValue('STATEMENTS');
const hasStatements = containerBlock.getFieldValue('STATEMENTS');
if (hasStatements !== null) {
hasStatements = hasStatements === 'TRUE';
if (this.hasStatements_ !== hasStatements) {
if (hasStatements) {
this.setStatements_(true);
// Restore the stack, if one was saved.
Mutator.reconnect(this.statementConnection_, this, 'STACK');
this.statementConnection_ = null;
} else {
// Save the stack, then disconnect it.
const stackConnection = this.getInput('STACK').connection;
this.statementConnection_ = stackConnection.targetConnection;
if (this.statementConnection_) {
const stackBlock = stackConnection.targetBlock();
stackBlock.unplug();
stackBlock.bumpNeighbours();
}
this.setStatements_(false);
}
}
this.setStatements_(hasStatements === 'TRUE');
}
},
};
Expand Down Expand Up @@ -718,6 +758,16 @@ const procedureDefContextMenuMixin = {
Extensions.registerMixin(
'procedure_def_context_menu_mixin', procedureDefContextMenuMixin);

const procedureDefOnChangeMixin = {
onchange: function(e) {
if (e.type === Events.BLOCK_CHANGE && e.element == 'disabled') {
this.model.setEnabled(!e.newValue);
}
},
};
Extensions.registerMixin(
'procedure_def_onchange_mixin', procedureDefOnChangeMixin);

/** @this {Block} */
const procedureDefNoReturnSetCommentHelper = function() {
if ((this.workspace.options.comments ||
Expand Down
2 changes: 2 additions & 0 deletions core/procedures/observable_parameter_model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import type {IProcedureModel} from '../interfaces/i_procedure_model';
import {triggerProceduresUpdate} from './update_procedures.js';
import type {VariableModel} from '../variable_model.js';
import type {Workspace} from '../workspace.js';
import * as goog from '../../closure/goog/goog.js';
goog.declareModuleId('Blockly.procedures.ObservableParameterModel');


export class ObservableParameterModel implements IParameterModel {
Expand Down
2 changes: 2 additions & 0 deletions core/procedures/observable_procedure_model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import type {IProcedureModel} from '../interfaces/i_procedure_model.js';
import {isObservable} from '../interfaces/i_observable.js';
import {triggerProceduresUpdate} from './update_procedures.js';
import type {Workspace} from '../workspace.js';
import * as goog from '../../closure/goog/goog.js';
goog.declareModuleId('Blockly.procedures.ObservableProcedureModel');


export class ObservableProcedureModel implements IProcedureModel {
Expand Down
Loading