Skip to content

Commit

Permalink
feat: Replace getValue check with getVariable for variable fields.
Browse files Browse the repository at this point in the history
Getting a variable using getValue applies only to FieldVariable. There
was a note to indicate this; this replaces it with the relevant check.

Also make getVariable available on fields, matching the existing referencesVariables/refreshVariableName prototypes.

This ensures that variables can be used with fields other than
dropdowns, as seems to be the intention.
  • Loading branch information
laurensvalk committed Nov 10, 2023
1 parent 925a7b9 commit bee5103
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 18 deletions.
30 changes: 14 additions & 16 deletions core/block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1113,8 +1113,10 @@ export class Block implements IASTNodeLocation, IDeletable {
for (let i = 0, input; (input = this.inputList[i]); i++) {
for (let j = 0, field; (field = input.fieldRow[j]); j++) {
if (field.referencesVariables()) {
// NOTE: This only applies to `FieldVariable`, a `Field<string>`
vars.push(field.getValue() as string);
const variable = field.getVariable();
if (variable) {
vars.push(variable.getId());
}
}
}
}
Expand All @@ -1129,18 +1131,11 @@ export class Block implements IASTNodeLocation, IDeletable {
*/
getVarModels(): VariableModel[] {
const vars = [];
for (let i = 0, input; (input = this.inputList[i]); i++) {
for (let j = 0, field; (field = input.fieldRow[j]); j++) {
if (field.referencesVariables()) {
const model = this.workspace.getVariableById(
field.getValue() as string,
);
// Check if the variable actually exists (and isn't just a potential
// variable).
if (model) {
vars.push(model);
}
}
for (const id of this.getVars()) {
const model = this.workspace.getVariableById(id);
// Check if the variable exists (and isn't just a potential variable).
if (model) {
vars.push(model);
}
}
return vars;
Expand All @@ -1158,7 +1153,7 @@ export class Block implements IASTNodeLocation, IDeletable {
for (let j = 0, field; (field = input.fieldRow[j]); j++) {
if (
field.referencesVariables() &&
variable.getId() === field.getValue()
variable.getId() === field.getVariable()?.getId()
) {
field.refreshVariableName();
}
Expand All @@ -1177,7 +1172,10 @@ export class Block implements IASTNodeLocation, IDeletable {
renameVarById(oldId: string, newId: string) {
for (let i = 0, input; (input = this.inputList[i]); i++) {
for (let j = 0, field; (field = input.fieldRow[j]); j++) {
if (field.referencesVariables() && oldId === field.getValue()) {
if (
field.referencesVariables() &&
oldId === field.getVariable()?.getId()
) {
field.setValue(newId);
}
}
Expand Down
11 changes: 11 additions & 0 deletions core/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import * as utilsXml from './utils/xml.js';
import * as WidgetDiv from './widgetdiv.js';
import type {WorkspaceSvg} from './workspace_svg.js';
import {ISerializable} from './interfaces/i_serializable.js';
import {VariableModel} from './variable_model.js';

/**
* A function that is called to validate changes to the field's value before
Expand Down Expand Up @@ -1275,6 +1276,16 @@ export abstract class Field<T = any>
return false;
}

/**
* Get the variable model if this field refers to one.
*
* @returns The field's variable, or null if none exists.
* @internal
*/
getVariable(): VariableModel | null {
return null;
}

/**
* Refresh the variable name referenced by this field if this field references
* variables.
Expand Down
2 changes: 1 addition & 1 deletion core/field_variable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ export class FieldVariable extends FieldDropdown {
* @returns The selected variable, or null if none was selected.
* @internal
*/
getVariable(): VariableModel | null {
override getVariable(): VariableModel | null {
return this.variable;
}

Expand Down
5 changes: 4 additions & 1 deletion tests/mocha/field_variable_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ suite('Variable Fields', function () {
const fieldVariable = new Blockly.FieldVariable('name1');
chai.assert.equal(fieldVariable.getText(), '');
chai.assert.isNull(fieldVariable.getValue());
chai.assert.isNull(fieldVariable.getVariable());
});
});

Expand Down Expand Up @@ -571,10 +572,12 @@ suite('Variable Fields', function () {
},
this.workspace,
);
const variable = block.getField('VAR').getVariable();
const field = block.getField('VAR');
const variable = field.getVariable();
chai.assert.equal(variable.name, 'test');
chai.assert.equal(variable.type, '');
chai.assert.equal(variable.getId(), 'id1');
chai.assert.equal(variable.getId(), field.getValue());
});

test('Name, untyped', function () {
Expand Down

0 comments on commit bee5103

Please sign in to comment.