revert "fix: Use setShadowState instead of setShadow in shadow-block-converter" (#2149)

Author: johnnesky

plugins/shadow-block-converter/README.md

@@ -1,6 +1,6 @@
 # @blockly/shadow-block-converter [![Built on Blockly](https://tinyurl.com/built-on-blockly)](https://github.com/google/blockly)
 
-A [Blockly](https://www.npmjs.com/package/blockly) plugin for automatically converting shadow blocks to regular blocks when the user edits them.
+A [Blockly](https://www.npmjs.com/package/blockly) plugin for automatically converting shadow blocks to real blocks when the user edits them.
 
 ## Installation
 
@@ -19,24 +19,9 @@ npm install @blockly/shadow-block-converter --save
 ## Usage
 
 This plugin exports a function called `shadowBlockConversionChangeListener`. If
-you add it as a change listener to your Blockly workspace then any shadow block
-the user edits will be converted to a regular block. This allows the user to
-move or delete the block, in which case the original shadow block will
-automatically return. With this plugin, shadow blocks behave like a persistent
-default value associated with the parent block (unlike standard Blockly
-behavior, where shadow blocks retain any edits made to them even after a regular
-block is dragged on top of them).
-
-The regular block will be a new block instance, separate from the shadow block
-that was replaced, and will have a different id. It will otherwise have the same
-properties and shape as the original shadow block.
-
-If the shadow block was attached to any ancestor blocks that were also shadows,
-they will be recreated as regular blocks. If the shadow block was attached to
-any descendent blocks, they will be recreated with different ids but will still
-be shadow blocks.
-
-See below for an example using it with a workspace.
+you add it as a change listener to your blockly workspace then any shadow block
+the user edits will be converted to a real block. See below for an example using
+it with a workspace.
 
 ### JavaScript
 

plugins/shadow-block-converter/src/shadow_block_converter.ts

@@ -11,56 +11,48 @@
 import * as Blockly from 'blockly/core';
 import {Block} from 'blockly/core/block';
 import {Abstract} from 'blockly/core/events/events_abstract';
-
-export interface BlockShadowStateChangeJson
-  extends Blockly.Events.BlockBaseJson {
-  inputIndexInParent: number | null;
-  shadowState: Blockly.serialization.blocks.State;
-}
+import {BlockChangeJson} from 'blockly/core/events/events_block_change';
 
 /**
- * A Blockly event class to revert a block connection's shadow state to the
- * provided state, to be used after attaching a child block that would
- * ordinarily overwrite the connection's shadow state.
+ * A new blockly event class specifically for recording changes to the shadow
+ * state of a block. This implementation is similar to and could be merged with
+ * the implementation of Blockly.Events.BlockChange in Blockly core code.
  */
-export class BlockShadowStateChange extends Blockly.Events.BlockBase {
+export class BlockShadowChange extends Blockly.Events.BlockBase {
   /**
    * The name of the event type for broadcast and listening purposes.
    */
   /* eslint-disable @typescript-eslint/naming-convention */
-  static readonly EVENT_TYPE = 'block_shadow_state_change';
+  static readonly EVENT_TYPE = 'block_shadow_change';
   /* eslint-enable @typescript-eslint/naming-convention */
 
   /**
-   * The index of the connection in the parent block's list of connections. If
-   * null, then the nextConnection will be used instead.
+   * The previous value of the field.
    */
-  inputIndexInParent: number | null;
+  oldValue: unknown;
 
   /**
-   * The intended shadow state of the connection.
+   * The new value of the field.
    */
-  shadowState: Blockly.serialization.blocks.State;
+  newValue: unknown;
 
   /**
-   * The constructor for a new BlockShadowStateChange event.
+   * The constructor for a new BlockShadowChange event.
    *
-   * @param block The parent of the connection to modify.
-   * @param inputIndexInParent The index of the input associated with the
-   *     connection to modify, if it is associated with one. Otherwise the
-   *     nextConnection will be used.
-   * @param shadowState The intended shadow state of the connection.
+   * @param block The changed block. Undefined for a blank event.
+   * @param oldValue Previous value of shadow state.
+   * @param newValue New value of shadow state.
    */
-  constructor(
-    block: Block,
-    inputIndexInParent: number | null,
-    shadowState: Blockly.serialization.blocks.State,
-  ) {
+  constructor(block?: Block, oldValue?: boolean, newValue?: boolean) {
     super(block);
 
-    this.type = BlockShadowStateChange.EVENT_TYPE;
-    this.inputIndexInParent = inputIndexInParent;
-    this.shadowState = shadowState;
+    this.type = BlockShadowChange.EVENT_TYPE;
+
+    if (!block) {
+      return; // Blank event to be populated by fromJson.
+    }
+    this.oldValue = typeof oldValue === 'undefined' ? '' : oldValue;
+    this.newValue = typeof newValue === 'undefined' ? '' : newValue;
   }
 
   /**
@@ -69,10 +61,10 @@ export class BlockShadowStateChange extends Blockly.Events.BlockBase {
    * @returns JSON representation.
    * @override
    */
-  toJson(): BlockShadowStateChangeJson {
-    const json = super.toJson() as BlockShadowStateChangeJson;
-    json['inputIndexInParent'] = this.inputIndexInParent;
-    json['shadowState'] = this.shadowState;
+  toJson(): BlockChangeJson {
+    const json = super.toJson() as BlockChangeJson;
+    json['oldValue'] = this.oldValue;
+    json['newValue'] = this.newValue;
     return json;
   }
 
@@ -83,18 +75,18 @@ export class BlockShadowStateChange extends Blockly.Events.BlockBase {
    * @override
    */
   static fromJson(
-    json: BlockShadowStateChangeJson,
+    json: BlockChangeJson,
     workspace: Blockly.Workspace,
     /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
     event?: any,
-  ): BlockShadowStateChange {
+  ): BlockShadowChange {
     const newEvent = super.fromJson(
       json,
       workspace,
       event,
-    ) as BlockShadowStateChange;
-    newEvent.inputIndexInParent = json['inputIndexInParent'];
-    newEvent.shadowState = json['shadowState'];
+    ) as BlockShadowChange;
+    newEvent.oldValue = json['oldValue'];
+    newEvent.newValue = json['newValue'];
     return event;
   }
 
@@ -105,7 +97,7 @@ export class BlockShadowStateChange extends Blockly.Events.BlockBase {
    * @override
    */
   isNull(): boolean {
-    return false;
+    return this.oldValue === this.newValue;
   }
 
   /**
@@ -130,169 +122,17 @@ export class BlockShadowStateChange extends Blockly.Events.BlockBase {
       );
     }
 
-    const connections = block.getConnections_(true);
-
-    let connection: Blockly.Connection | null;
-    if (this.inputIndexInParent === null) {
-      connection = block.nextConnection;
-    } else if (
-      typeof this.inputIndexInParent !== 'number' ||
-      this.inputIndexInParent < 0 ||
-      this.inputIndexInParent >= connections.length
-    ) {
-      throw new Error('inputIndexInParent was invalid.');
-    } else {
-      connection = block.inputList[this.inputIndexInParent].connection;
-    }
-    if (connection === null) {
-      throw new Error('No matching connection was found.');
-    }
-
-    if (forward) {
-      connection.setShadowState(this.shadowState || null);
-    }
-
-    // Nothing to be done when run backward, because removing a child block
-    // doesn't overwrite the connection's shadowState and thus doesn't need to
-    // be reverted.
+    const value = forward ? this.newValue : this.oldValue;
+    block.setShadow(!!value);
   }
 }
 
 Blockly.registry.register(
   Blockly.registry.Type.EVENT,
-  BlockShadowStateChange.EVENT_TYPE,
-  BlockShadowStateChange,
+  BlockShadowChange.EVENT_TYPE,
+  BlockShadowChange,
 );
 
-/**
- * Convert the provided shadow block into a regular block, along with any parent
- * shadow blocks.
- *
- * The provided block will be deleted, and a new regular block will be created
- * in its place that has new id but is otherwise identical to the shadow block.
- * The parent connection's shadow state will be forcibly preserved, despite the
- * fact that attaching a regular block to the connection ordinarily overwrites
- * the connection's shadow state.
- *
- * @param shadowBlock
- * @returns The newly created regular block with a different id, if one could be
- *     created.
- */
-function reifyEditedShadowBlock(shadowBlock: Block): Blockly.Block {
-  // Determine how the shadow block is connected to the parent.
-  let parentConnection: Blockly.Connection | null = null;
-  let connectionIsThroughOutputConnection = false;
-  if (shadowBlock.previousConnection?.isConnected()) {
-    parentConnection = shadowBlock.previousConnection.targetConnection;
-  } else if (shadowBlock.outputConnection?.isConnected()) {
-    parentConnection = shadowBlock.outputConnection.targetConnection;
-    connectionIsThroughOutputConnection = true;
-  }
-  if (parentConnection === null) {
-    // We can't change the shadow status of a block with no parent, so just
-    // return the block as-is.
-    return shadowBlock;
-  }
-
-  // Get the parent block, and the index of the connection's input if it is
-  // associated with one.
-  let parentBlock = parentConnection.getSourceBlock();
-  const inputInParent = parentConnection.getParentInput();
-  const inputIndexInParent: number | null = inputInParent
-    ? parentBlock.inputList.indexOf(inputInParent)
-    : null;
-
-  // Recover the state of the shadow block before it was edited. The connection
-  // should still have the original state until a new block is attached to it.
-  const originalShadowState = parentConnection.getShadowState(
-    /* returnCurrent = */ false,
-  );
-
-  // Serialize the current state of the shadow block (after it was edited).
-  const editedBlockState = Blockly.serialization.blocks.save(shadowBlock, {
-    addCoordinates: false,
-    addInputBlocks: true,
-    addNextBlocks: true,
-    doFullSerialization: false,
-  });
-  if (originalShadowState === null || editedBlockState === null) {
-    // The serialized block states are necessary to convert the block. Without
-    // them, just return the block as-is.
-    return shadowBlock;
-  }
-
-  // If the parent block is a shadow, it must be converted first.
-  if (parentBlock.isShadow()) {
-    const newParentBlock = reifyEditedShadowBlock(parentBlock);
-    if (newParentBlock === null) {
-      throw new Error(
-        "No parent block was created, so we can't recreate the " +
-          'current block either.',
-      );
-    }
-    parentBlock = newParentBlock;
-
-    // The reference to the connection is obsolete. Find it from the new parent.
-    if (inputIndexInParent === null) {
-      parentConnection = parentBlock.nextConnection;
-    } else if (
-      inputIndexInParent < 0 ||
-      inputIndexInParent >= parentBlock.inputList.length
-    ) {
-      throw new Error('inputIndexInParent is invalid.');
-    } else {
-      parentConnection = parentBlock.inputList[inputIndexInParent].connection;
-    }
-    if (parentConnection === null) {
-      throw new Error(
-        "Couldn't find the corresponding connection on the new " +
-          'version of the parent block.',
-      );
-    }
-  }
-
-  // Let Blockly generate a new id for the new regular block. Ideally, we would
-  // let the shadow block and the regular block have the same id, and in
-  // principle that ought to be possible since they don't need to coexist at the
-  // same time. However, we'll need to call setShadowState on the connection
-  // after attaching the regular block to revert any changes made by attaching
-  // the block, and the setShadowState implementation temporarily instantiates
-  // the provided shadow state, which can't have the same id as a block in the
-  // workspace. The new shadow state id won't be compatible with any existing
-  // undo history on the shadow block, such as the block change event that
-  // triggered this whole shadow conversion!
-  editedBlockState.id = undefined;
-
-  // Create a regular version of the shadow block by deserializing its state
-  // independently from the connection.
-  const regularBlock = Blockly.serialization.blocks.append(
-    editedBlockState,
-    parentBlock.workspace,
-    {recordUndo: true},
-  );
-
-  // Attach the regular block to the connection in place of the shadow block.
-  const childConnection = connectionIsThroughOutputConnection
-    ? regularBlock.outputConnection
-    : regularBlock.previousConnection;
-  if (childConnection) {
-    parentConnection.connect(childConnection);
-  }
-
-  // The process of connecting a block overwrites the connection's shadow state,
-  // so revert it.
-  parentConnection.setShadowState(originalShadowState);
-  Blockly.Events.fire(
-    new BlockShadowStateChange(
-      parentBlock,
-      inputIndexInParent,
-      originalShadowState,
-    ),
-  );
-
-  return regularBlock;
-}
-
 /**
  * Add this function to your workspace as a change listener to automatically
  * convert shadow blocks to real blocks whenever the user edits a field on the
@@ -351,7 +191,40 @@ export function shadowBlockConversionChangeListener(event: Abstract) {
     Blockly.Events.setGroup(true);
   }
 
-  reifyEditedShadowBlock(block);
+  // If the changed shadow block is a child of another shadow block, then both
+  // blocks should be converted to real blocks. To find all the shadow block
+  // ancestors that need to be converted to real blocks, seed the list of blocks
+  // starting with the changed block, and append all shadow block ancestors.
+  const shadowBlocks = [block];
+  for (let i = 0; i < shadowBlocks.length; i++) {
+    const shadowBlock = shadowBlocks[i];
+
+    // If connected blocks need to be converted too, add them to the list.
+    const outputBlock: Block | null | undefined =
+      shadowBlock.outputConnection?.targetBlock();
+    const previousBlock: Block | null | undefined =
+      shadowBlock.previousConnection?.targetBlock();
+    if (outputBlock?.isShadow()) {
+      shadowBlocks.push(outputBlock);
+    }
+    if (previousBlock?.isShadow()) {
+      shadowBlocks.push(previousBlock);
+    }
+  }
+
+  // The list of shadow blocks starts with the deepest child and ends with the
+  // highest parent, but the parent of a real block should never be a shadow
+  // block, so the parents need to be converted to real blocks first. Start
+  // at the end of the list and iterate backward to convert the blocks.
+  for (let i = shadowBlocks.length - 1; i >= 0; i--) {
+    const shadowBlock = shadowBlocks[i];
+    // Convert the shadow block to a real block and fire an event recording the
+    // change so that it can be undone. Ideally the
+    // Blockly.Block.prototype.setShadow method should fire this event directly,
+    // but for this plugin it needs to be explicitly fired here.
+    shadowBlock.setShadow(false);
+    Blockly.Events.fire(new BlockShadowChange(shadowBlock, true, false));
+  }
 
   // Revert to the current event group, if any.
   Blockly.Events.setGroup(currentGroup);

plugins/shadow-block-converter/test/index.ts

@@ -15,16 +15,6 @@ import {shadowBlockConversionChangeListener} from '../src/index';
 const toolbox: Blockly.utils.toolbox.ToolboxDefinition = {
   kind: 'flyoutToolbox',
   contents: [
-    {
-      kind: 'block',
-      type: 'text_reverse',
-      inputs: {
-        TEXT: {
-          shadow: {type: 'text', fields: {TEXT: 'abc'}},
-          block: undefined,
-        },
-      },
-    },
     {
       kind: 'block',
       type: 'colour_blend',