diff --git a/packages/scratch-gui/src/lib/define-dynamic-block.js b/packages/scratch-gui/src/lib/define-dynamic-block.js index fb43f7aab8d..dee3c7fb4df 100644 --- a/packages/scratch-gui/src/lib/define-dynamic-block.js +++ b/packages/scratch-gui/src/lib/define-dynamic-block.js @@ -38,6 +38,25 @@ const buildInterpolationArgs = function (blockInfo) { // === Smalruby: Start of argumentsByMethod support === +/** + * Build a FieldVerticalSeparator instance compatible with both v1 and v2 + * scratch-blocks. v2 only registers the field via `fieldRegistry`, so the + * v1-style `new ScratchBlocks.FieldVerticalSeparator()` constructor call + * throws "is not a constructor" at runtime. + * @param {object} ScratchBlocks - the ScratchBlocks namespace. + * @returns {object|null} the field instance, or null if unavailable. + */ +const makeVerticalSeparator = function (ScratchBlocks) { + if (typeof ScratchBlocks.FieldVerticalSeparator === 'function') { + return new ScratchBlocks.FieldVerticalSeparator(); + } + if (ScratchBlocks.fieldRegistry && + typeof ScratchBlocks.fieldRegistry.fromJson === 'function') { + return ScratchBlocks.fieldRegistry.fromJson({type: 'field_vertical_separator'}); + } + return null; +}; + /** * Parse block text template into an array of components. * Each component is either {type: 'label', text} or {type: 'arg', name}. @@ -181,7 +200,8 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks label.src, label.width, label.height )); } else if (label.fieldVerticalSeparator) { - input.appendField(new ScratchBlocks.FieldVerticalSeparator()); + const sep = makeVerticalSeparator(ScratchBlocks); + if (sep) input.appendField(sep); } } pendingLabels = []; @@ -226,7 +246,8 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks label.src, label.width, label.height )); } else if (label.fieldVerticalSeparator) { - dummyInput.appendField(new ScratchBlocks.FieldVerticalSeparator()); + const sep = makeVerticalSeparator(ScratchBlocks); + if (sep) dummyInput.appendField(sep); } } } @@ -234,8 +255,31 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks /** * Rebuild the block's inputs based on new blockInfo. - * Follows the procedures_call updateDisplay_ pattern: - * pause rendering → disconnect → remove → create → render. + * Follows the procedures_call updateDisplay_ pattern (scratch-blocks v2): + * disconnect → remove → create → dispose orphans. **Do NOT toggle + * `block.rendered` and do NOT call `block.initSvg()` / `block.render()` + * manually** — Blockly v12's `Input.appendField` is now responsible for + * initialising fresh fields based on the block state: + * + * appendField(field, name) { + * field.setSourceBlock(this.sourceBlock); + * this.sourceBlock.initialized && this.initField(field); + * this.sourceBlock.rendered && this.sourceBlock.queueRender(); + * } + * initField(field) { + * this.sourceBlock.rendered ? field.init() : field.initModel(); + * } + * + * If we set `block.rendered = false` before `appendField` runs, only + * `field.initModel()` is called and the SVG `` element of a fresh + * `FieldDropdown` is never created. The follow-up `block.initSvg()` is + * a no-op because `block.initialized` is already `true` (initSvg has a + * one-shot guard), so the missing text element is never repaired and + * the next render throws `Field.getTextContent()` "The text content is + * null." (issue #634). + * + * scratch-blocks v2's own `procedures.ts updateDisplay_` does exactly + * the no-rendered-toggle pattern below. * @param {object} block - the scratch-blocks Block instance. * @param {object} newBlockInfo - the new blockInfo to build from. * @param {object} ScratchBlocks - the ScratchBlocks namespace. @@ -243,9 +287,6 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks * @param {object} categoryInfo - extension category info (for resolving dynamic menus). */ const updateBlockDisplay = function (block, newBlockInfo, ScratchBlocks, skipShadows, categoryInfo) { - const wasRendered = block.rendered; - block.rendered = false; - const connectionMap = disconnectOldBlocks(block); removeAllInputs(block); createAllInputs(block, newBlockInfo, connectionMap, ScratchBlocks, skipShadows, categoryInfo); @@ -260,10 +301,21 @@ const updateBlockDisplay = function (block, newBlockInfo, ScratchBlocks, skipSha } } - block.rendered = wasRendered; - if (wasRendered && !block.isInsertionMarker()) { + // For freshly-built blocks (block.initialized === false), the + // appendField calls above did NOT trigger field.init() because + // Blockly v12's `Input.initField` checks `block.initialized` first. + // Call `block.initSvg()` to walk inputList and run `field.init()` + // for every field, which creates each field's SVG `` element + // (the missing `getTextContent()` target — issue #634). + // + // `initSvg()` is idempotent via its one-shot guard, so calling it on + // an already-initialized block is a no-op. `render()` then re-measures + // and lays out the block with the new inputs/fields. + if (block.initSvg && !block.isInsertionMarker()) { block.initSvg(); - block.render(); + if (block.render) { + block.render(); + } } }; diff --git a/packages/scratch-gui/src/lib/ruby-generator/smalruby-ruby.js b/packages/scratch-gui/src/lib/ruby-generator/smalruby-ruby.js index b073b056fb1..737a7c8c30d 100644 --- a/packages/scratch-gui/src/lib/ruby-generator/smalruby-ruby.js +++ b/packages/scratch-gui/src/lib/ruby-generator/smalruby-ruby.js @@ -16,7 +16,15 @@ export default function (Generator) { if (isBang) { const varName = Generator.getFieldValue(block, 'RECEIVER') || ''; - receiver = Generator.variableNameByName(varName) || 'nil'; + // Array bang methods (sort!, reverse!) reference the LIST + // variant; string bang methods reference the SCALAR variant. + // target-applier.js (issue #634) drops the eager SCALAR + // entry when an array literal also creates a same-name LIST, + // so try both stores. + receiver = + Generator.variableNameByName(varName) || + Generator.listNameByName(varName) || + 'nil'; } else { receiver = Generator.valueToCode(block, 'RECEIVER', order) || diff --git a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/target-applier.js b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/target-applier.js index aaf437f74da..04fdaa63703 100644 --- a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/target-applier.js +++ b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/target-applier.js @@ -54,11 +54,32 @@ const TargetApplier = { // Map of old variable IDs to new IDs (for reusing existing variables) const variableIdMap = {}; - ['variables', 'lists', 'localVariables'].forEach(storeName => { + // === Smalruby (issue #634): Skip the second occurrence of any + // (name, scope) pair across stores. The converter's `_onVasgn` + // eagerly creates a SCALAR in `_context.localVariables` before + // visiting the RHS, so an array-literal handler that subsequently + // creates a LIST in `_context.lists` ends up with two variables + // sharing the same name (e.g. `_a_1_`) but different types and + // ids. target-applier would push both to the VM target, and + // scratch-blocks v2 then fails to parse the workspace XML for + // any block referencing the list. Iterate `lists` first so list + // wins over the eager scalar; the second iteration of + // localVariables drops the duplicate. === + const seenVarKeys = new Set(); + ['lists', 'variables', 'localVariables'].forEach(storeName => { Object.keys(this._context[storeName]).forEach(name => { const variable = this._context[storeName][name]; if (variable.isArgument) return; + const dedupeKey = `${variable.scope}:${variable.name}`; + if (seenVarKeys.has(dedupeKey)) { + // Older entry from another store wins; remap any later + // references through variableIdMap so block fields stay + // consistent. + return; + } + seenVarKeys.add(dedupeKey); + const oldId = variable.id; let existingVar = null;