From cf7dc65d0a471305041024cd7257ffbaef3f4baf Mon Sep 17 00:00:00 2001 From: Kouji Takao Date: Mon, 4 May 2026 21:35:12 +0900 Subject: [PATCH 1/4] fix(ruby-converter): de-duplicate scalar/list variable for `a = [...]` literals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #634 (under feat/upstream-merge-2026-05). Three independent v2 incompatibilities surfaced together when converting `a = [1, 2, 3]; puts(a.max)` and similar Ruby code: 1. **`ScratchBlocks.FieldVerticalSeparator` removed in v2.** scratch-blocks v2 no longer exports the class on the namespace; it is registered via `fieldRegistry`. The old `new ScratchBlocks.FieldVerticalSeparator()` call in `defineDynamicBlock.createAllInputs` threw mid-flight in `domToMutation`, which made the surrounding XML parser drop the rest of the block's `` chain — every Ruby-converted script that touched an extension block with an icon ended up as detached top-level blocks. Add a v2-compatible `makeVerticalSeparator` helper that falls back to `fieldRegistry.fromJson({type: 'field_vertical_separator'})`. 2. **Eager scalar created by `_onVasgn`.** `assignments.js _onVasgn` calls `_lookupOrCreateVariable(varName)` *before* visiting the RHS, which registers `a` as a SCALAR in `_context.localVariables`. The array-literal converter then calls `_lookupOrCreateList(a)` to get the list, but the eager scalar is left behind — target-applier creates *both* `_a_1_` (list) and `_a_1_` (scalar) on the VM target, and scratch-blocks v2 fails to render any block referencing the list. The array-literal handler now drops the eager scalar before creating the list. 3. **Cross-store re-creation at top level.** At the top level the converter has no scope tracking (`_getCurrentScope() === null`), so a subsequent `_lookupOrCreateVariable('a')` call (e.g. from `visitLocalVariableReadNode` for `a.max`) takes the falls-through creation path and registers a fresh scalar with the same transformed name. Add a check in `_lookupOrCreateVariableOrList` to reuse an existing entry from the *other* store (lists ↔ localVariables) when the requested store is empty for that transformed name. Together these keep exactly one canonical variable per local name across the converter's data flow, which is required for scratch-blocks v2 to parse the workspace XML the VM emits. Note: rendering of `smalrubyRuby_arrayMethod` / `smalrubyRuby_hashMethod` blocks themselves is still pending — `Field.getTextContent()` returns null during render even with a clean variable graph. That is a separate v2 incompat tracked in #634 and will be addressed in follow-up commits. --- .../src/lib/define-dynamic-block.js | 37 ++++++++++++++++++- .../variable-utils.js | 16 ++++++++ .../lib/ruby-to-blocks-converter/variables.js | 19 ++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/packages/scratch-gui/src/lib/define-dynamic-block.js b/packages/scratch-gui/src/lib/define-dynamic-block.js index fb43f7aab8d..98dd36ec113 100644 --- a/packages/scratch-gui/src/lib/define-dynamic-block.js +++ b/packages/scratch-gui/src/lib/define-dynamic-block.js @@ -38,6 +38,37 @@ const buildInterpolationArgs = function (blockInfo) { // === Smalruby: Start of argumentsByMethod support === +/** + * Create a vertical separator field instance compatible with both + * scratch-blocks v1 (constructor on namespace) and v2 (fieldRegistry). + * + * In v2 the class is no longer exported as + * `ScratchBlocks.FieldVerticalSeparator`; it is only registered via + * `Blockly.fieldRegistry`. Calling the v1-style constructor throws + * "ScratchBlocks.FieldVerticalSeparator is not a constructor". + * + * The throw aborts `defineDynamicBlock.domToMutation` mid-flight, which in + * turn causes the surrounding XML parser to drop the `` chain after + * the failing block — every block in a Ruby-converted script that uses an + * extension block with an icon (smalrubyRuby_arrayMethod / + * smalrubyRuby_hashMethod / smalrubyRuby_stringMethod) ends up as a + * detached top-level workspace block. Combined with the recyclable flyout + * dispose path also crashing on the same blocks, this manifests as the + * "toolbox/workspace renders blank" symptom (issue #634). + * @param {object} ScratchBlocks - the ScratchBlocks namespace. + * @returns {object|null} a Field instance, or null if neither path works. + */ +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 +212,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 +258,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); } } } diff --git a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js index bd07d2b2617..07a858d034f 100644 --- a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js +++ b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js @@ -83,6 +83,22 @@ const VariableUtils = { variable = this._context[storeName][transformedName]; if (!variable) { + // === Smalruby (issue #634): Before creating a new variable + // of the requested type, check the *other* store for an + // existing entry with the same transformed name. This + // happens at top-level (no scope tracking) when the + // converter sees `a = [1, 2, 3]` (registers `a` in `lists`) + // followed by `puts(a.max)` — the second `_lookupOrCreateVariable('a')` + // would otherwise create a parallel scalar and target-applier + // would push two same-name variables to the VM, breaking + // scratch-blocks v2's workspace XML render. Reuse the + // existing canonical variable so there is exactly one + // `_a_1_` per local name. === + const otherStore = type === Variable.SCALAR_TYPE ? 'lists' : 'localVariables'; + const otherVar = this._context[otherStore][transformedName]; + if (otherVar) { + return otherVar; + } variable = { id: Blockly.utils.idGenerator.genUid(), name: transformedName, diff --git a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js index 2c407ccb317..a3b1ccde055 100644 --- a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js +++ b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js @@ -266,6 +266,25 @@ const VariablesConverter = { } else { prefixedName = variable.originalName; } + // === Smalruby: Drop the scalar entry created eagerly by + // `_onVasgn` (assignments.js) before this handler ran. + // For `a = [1, 2, 3]`, `_onVasgn` calls + // `_lookupOrCreateVariable(varName)` before visiting the + // RHS, which registers `a` as a SCALAR in + // `_context.localVariables` (and as a sprite-level scalar + // via target-applier). The list created below is the + // intended canonical entry, so the eager scalar must be + // removed to keep a single `_a_1_` on the target — + // otherwise scratch-blocks v2 sees two same-name variables + // and fails to render the workspace XML for any block that + // references the list (issue #634). === + if (variable && !converter._isBlock(variable)) { + if (variable.scope === 'local') { + delete converter._context.localVariables[variable.name]; + } else if (variable.scope === 'global' || variable.scope === 'instance') { + delete converter._context.variables[variable.name]; + } + } const listVar = converter._lookupOrCreateList(prefixedName); // Create clear block From 52441f1433aac9beaca495178a98b8785b814152 Mon Sep 17 00:00:00 2001 From: Kouji Takao Date: Mon, 4 May 2026 22:58:49 +0900 Subject: [PATCH 2/4] fix(define-dynamic-block): resolve "text content is null" render crash on v2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #634 — final fix. Smalruby's `defineDynamicBlock.updateBlockDisplay` followed a v1-style pattern that toggled `block.rendered = false` around `appendField` and re-issued `block.initSvg()` + `block.render()` at the end: const wasRendered = block.rendered; block.rendered = false; ... createAllInputs(block, ...); // → appendField(new FieldDropdown(...)) ... block.rendered = wasRendered; if (wasRendered) { block.initSvg(); block.render(); } In Blockly v12 (the runtime under scratch-blocks v2.1.19) this dance silently drops field initialisation: Input.appendField(field, name) { field.setSourceBlock(this.sourceBlock); this.sourceBlock.initialized && this.initField(field); this.sourceBlock.rendered && this.sourceBlock.queueRender(); } Input.initField(field) { this.sourceBlock.rendered ? field.init() : field.initModel(); } For a block constructed via `domToMutation`: * `block.initialized` is `false` until `initSvg()` runs the one-shot guard. * `block.rendered` is `true` (BlockSvg constructor default). Smalruby was forcing `rendered = false` during `appendField`, so: * `appendField` saw `initialized=false` → skipped `initField` entirely → field's SVG `` element never created. * The follow-up `block.initSvg()` is gated by `initialized`, but because `initialized` was still `false`, that path is fine — *except* my code flipped `rendered` back to `true` before it. With `rendered=true`, `initSvg`'s field-init would run `field.init()` (which creates the SVG text). Either path *should* work. * In practice, neither path triggers consistently across the XML-parser-driven domToMutation flow, leaving the field's `textContent_` null. The next `block.render()` invokes the block renderer, which calls `Field.getSize` → `Field.render_` → `Field.getTextContent`, and the missing text element throws "The text content is null." Stack: at Field.getTextContent at Field.render_ at Field.getSize at new BlockRenderRow at BlockRenderer.createRows_ at BlockRenderer.measure at Block.render The error aborts the parser mid-tree, drops the rest of the block's `` chain, and leaves Smalruby's flyout/workspace unable to render any script that references `smalrubyRuby_arrayMethod`, `smalrubyRuby_hashMethod`, or `smalrubyRuby_stringMethod`. Fix: stop toggling `block.rendered`. Let `appendField` see the genuine state (`initialized=false`, `rendered=true`) so it correctly delegates field initialisation to the subsequent `block.initSvg()` call we make at the end. `block.initSvg()` walks `inputList` and calls `field.init()` for every field, which creates each field's SVG `` element. We also keep `block.render()` to re-measure the block with its new inputs. This mirrors what scratch-blocks v2's own `procedures.ts updateDisplay_` does — that's the working blueprint for the same pattern. After the fix: * `puts(a.max)` round-trips correctly (`a = [1, 2, 3]` block chain followed by `Array (a) . max` connected to `戻り値 と 1 秒言う`). * `Field.getTextContent` no longer throws. * Toolbox refresh no longer fails. Verified manually with Playwright on `?tab=ruby&ruby_version=2`. --- .../src/lib/define-dynamic-block.js | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/packages/scratch-gui/src/lib/define-dynamic-block.js b/packages/scratch-gui/src/lib/define-dynamic-block.js index 98dd36ec113..06d06d43898 100644 --- a/packages/scratch-gui/src/lib/define-dynamic-block.js +++ b/packages/scratch-gui/src/lib/define-dynamic-block.js @@ -267,8 +267,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. @@ -276,9 +299,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); @@ -293,10 +313,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(); + } } }; From 6bd94313541041789e15eb4e9e98d5db13225c76 Mon Sep 17 00:00:00 2001 From: Kouji Takao Date: Mon, 4 May 2026 23:08:24 +0900 Subject: [PATCH 3/4] Revert "fix(ruby-converter): de-duplicate scalar/list variable for `a = [...]` literals" This reverts commit cf7dc65d0a471305041024cd7257ffbaef3f4baf. --- .../src/lib/define-dynamic-block.js | 37 +------------------ .../variable-utils.js | 16 -------- .../lib/ruby-to-blocks-converter/variables.js | 19 ---------- 3 files changed, 2 insertions(+), 70 deletions(-) diff --git a/packages/scratch-gui/src/lib/define-dynamic-block.js b/packages/scratch-gui/src/lib/define-dynamic-block.js index 06d06d43898..b90f60244e8 100644 --- a/packages/scratch-gui/src/lib/define-dynamic-block.js +++ b/packages/scratch-gui/src/lib/define-dynamic-block.js @@ -38,37 +38,6 @@ const buildInterpolationArgs = function (blockInfo) { // === Smalruby: Start of argumentsByMethod support === -/** - * Create a vertical separator field instance compatible with both - * scratch-blocks v1 (constructor on namespace) and v2 (fieldRegistry). - * - * In v2 the class is no longer exported as - * `ScratchBlocks.FieldVerticalSeparator`; it is only registered via - * `Blockly.fieldRegistry`. Calling the v1-style constructor throws - * "ScratchBlocks.FieldVerticalSeparator is not a constructor". - * - * The throw aborts `defineDynamicBlock.domToMutation` mid-flight, which in - * turn causes the surrounding XML parser to drop the `` chain after - * the failing block — every block in a Ruby-converted script that uses an - * extension block with an icon (smalrubyRuby_arrayMethod / - * smalrubyRuby_hashMethod / smalrubyRuby_stringMethod) ends up as a - * detached top-level workspace block. Combined with the recyclable flyout - * dispose path also crashing on the same blocks, this manifests as the - * "toolbox/workspace renders blank" symptom (issue #634). - * @param {object} ScratchBlocks - the ScratchBlocks namespace. - * @returns {object|null} a Field instance, or null if neither path works. - */ -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}. @@ -212,8 +181,7 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks label.src, label.width, label.height )); } else if (label.fieldVerticalSeparator) { - const sep = makeVerticalSeparator(ScratchBlocks); - if (sep) input.appendField(sep); + input.appendField(new ScratchBlocks.FieldVerticalSeparator()); } } pendingLabels = []; @@ -258,8 +226,7 @@ const createAllInputs = function (block, blockInfo, connectionMap, ScratchBlocks label.src, label.width, label.height )); } else if (label.fieldVerticalSeparator) { - const sep = makeVerticalSeparator(ScratchBlocks); - if (sep) dummyInput.appendField(sep); + dummyInput.appendField(new ScratchBlocks.FieldVerticalSeparator()); } } } diff --git a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js index 07a858d034f..bd07d2b2617 100644 --- a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js +++ b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variable-utils.js @@ -83,22 +83,6 @@ const VariableUtils = { variable = this._context[storeName][transformedName]; if (!variable) { - // === Smalruby (issue #634): Before creating a new variable - // of the requested type, check the *other* store for an - // existing entry with the same transformed name. This - // happens at top-level (no scope tracking) when the - // converter sees `a = [1, 2, 3]` (registers `a` in `lists`) - // followed by `puts(a.max)` — the second `_lookupOrCreateVariable('a')` - // would otherwise create a parallel scalar and target-applier - // would push two same-name variables to the VM, breaking - // scratch-blocks v2's workspace XML render. Reuse the - // existing canonical variable so there is exactly one - // `_a_1_` per local name. === - const otherStore = type === Variable.SCALAR_TYPE ? 'lists' : 'localVariables'; - const otherVar = this._context[otherStore][transformedName]; - if (otherVar) { - return otherVar; - } variable = { id: Blockly.utils.idGenerator.genUid(), name: transformedName, diff --git a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js index a3b1ccde055..2c407ccb317 100644 --- a/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js +++ b/packages/scratch-gui/src/lib/ruby-to-blocks-converter/variables.js @@ -266,25 +266,6 @@ const VariablesConverter = { } else { prefixedName = variable.originalName; } - // === Smalruby: Drop the scalar entry created eagerly by - // `_onVasgn` (assignments.js) before this handler ran. - // For `a = [1, 2, 3]`, `_onVasgn` calls - // `_lookupOrCreateVariable(varName)` before visiting the - // RHS, which registers `a` as a SCALAR in - // `_context.localVariables` (and as a sprite-level scalar - // via target-applier). The list created below is the - // intended canonical entry, so the eager scalar must be - // removed to keep a single `_a_1_` on the target — - // otherwise scratch-blocks v2 sees two same-name variables - // and fails to render the workspace XML for any block that - // references the list (issue #634). === - if (variable && !converter._isBlock(variable)) { - if (variable.scope === 'local') { - delete converter._context.localVariables[variable.name]; - } else if (variable.scope === 'global' || variable.scope === 'instance') { - delete converter._context.variables[variable.name]; - } - } const listVar = converter._lookupOrCreateList(prefixedName); // Create clear block From 8eba417ae13f60f0c50db5f9b5748d64342ba724 Mon Sep 17 00:00:00 2001 From: Kouji Takao Date: Tue, 5 May 2026 01:47:03 +0900 Subject: [PATCH 4/4] fix(issue-634): make FieldVerticalSeparator + array-literal vars work on v2 Three coordinated fixes for the array/hash method flyout crash on scratch-blocks v2: 1. define-dynamic-block.js: scratch-blocks v2 only registers FieldVerticalSeparator via fieldRegistry; the v1-style `new ScratchBlocks.FieldVerticalSeparator()` constructor call now throws "is not a constructor" at runtime. Add a `makeVerticalSeparator` helper that prefers the v1 constructor when present and falls back to `fieldRegistry.fromJson({type: 'field_vertical_separator'})` for v2. 2. target-applier.js: the converter eagerly creates a SCALAR entry in `_context.localVariables` for `t = [...]` before visiting the RHS. The array-literal handler then creates a LIST entry in `_context.lists` under the same transformed name. Pushing both to the VM target makes scratch-blocks v2 fail to parse the workspace XML for any block that references the list. Iterate `lists` first and dedupe by `(scope, name)` so the LIST wins and the eager SCALAR is dropped. 3. smalruby-ruby.js (Ruby generator): the bang-method generator (`sort!`, `reverse!`) reads RECEIVER as a name and looks it up via `variableNameByName(name)` (defaults to SCALAR_TYPE). With the SCALAR dropped at target-applier boundary, the lookup returns null and the round-trip emits `nil.sort!`. Fall back to `listNameByName` so array bang methods resolve through the surviving LIST entry. Closes #634 --- .../src/lib/define-dynamic-block.js | 25 +++++++++++++++++-- .../src/lib/ruby-generator/smalruby-ruby.js | 10 +++++++- .../target-applier.js | 23 ++++++++++++++++- 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/packages/scratch-gui/src/lib/define-dynamic-block.js b/packages/scratch-gui/src/lib/define-dynamic-block.js index b90f60244e8..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); } } } 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;