fix: improve performance of connecting blocks

core/block_svg.ts

@@ -123,6 +123,7 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
   /** @internal */
   pathObject: IPathObject;
   override rendered = false;
+  private visuallyDisabled = false;
 
   /**
    * Is this block currently rendering? Used to stop recursive render calls
@@ -889,15 +890,12 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
    * @internal
    */
   updateDisabled() {
-    const children = (this.getChildren(false));
+    const disabled = !this.isEnabled() || this.getInheritedDisabled();
+    if (this.visuallyDisabled === disabled) return;
     this.applyColour();
-    if (this.isCollapsed()) {
-      return;
-    }
-    for (let i = 0, child; child = children[i]; i++) {
-      if (child.rendered) {
-        child.updateDisabled();
-      }
+    this.visuallyDisabled = disabled;
+    for (const child of this.getChildren(false)) {
+      child.updateDisabled();
     }
   }
 

core/connection.ts

@@ -93,7 +93,7 @@ export class Connection implements IASTNodeLocationWithBlock {
 
     // Make sure the childConnection is available.
     if (childConnection.isConnected()) {
-      childConnection.disconnect();
+      childConnection.disconnectInternal(false);
     }
 
     // Make sure the parentConnection is available.
@@ -104,7 +104,7 @@ export class Connection implements IASTNodeLocationWithBlock {
       if (target!.isShadow()) {
         target!.dispose(false);
       } else {
-        this.disconnect();
+        this.disconnectInternal();
         orphan = target;
       }
       this.applyShadowState_(shadowState);
@@ -243,67 +243,75 @@ export class Connection implements IASTNodeLocationWithBlock {
     return this.isConnected();
   }
 
-  /** Disconnect this connection. */
+  /**
+   * Disconnect this connection.
+   */
   disconnect() {
-    const otherConnection = this.targetConnection;
-    if (!otherConnection) {
-      throw Error('Source connection not connected.');
-    }
-    if (otherConnection.targetConnection !== this) {
-      throw Error('Target connection not connected to source connection.');
-    }
-    let parentBlock;
-    let childBlock;
-    let parentConnection;
-    if (this.isSuperior()) {
-      // Superior block.
-      parentBlock = this.sourceBlock_;
-      childBlock = otherConnection.getSourceBlock();
-      /* eslint-disable-next-line @typescript-eslint/no-this-alias */
-      parentConnection = this;
-    } else {
-      // Inferior block.
-      parentBlock = otherConnection.getSourceBlock();
-      childBlock = this.sourceBlock_;
-      parentConnection = otherConnection;
-    }
-
-    const eventGroup = eventUtils.getGroup();
-    if (!eventGroup) {
-      eventUtils.setGroup(true);
-    }
-    this.disconnectInternal_(parentBlock, childBlock);
-    if (!childBlock.isShadow()) {
-      // If we were disconnecting a shadow, no need to spawn a new one.
-      parentConnection.respawnShadow_();
-    }
-    if (!eventGroup) {
-      eventUtils.setGroup(false);
-    }
+    this.disconnectInternal();
   }
 
   /**
    * Disconnect two blocks that are connected by this connection.
    *
-   * @param parentBlock The superior block.
-   * @param childBlock The inferior block.
+   * @param setParent Whether to set the parent of the disconnected block or
+   *     not, defaults to true.
+   *     If you do not set the parent, ensure that a subsequent action does,
+   *     otherwise the view and model will be out of sync.
    */
-  protected disconnectInternal_(parentBlock: Block, childBlock: Block) {
+  protected disconnectInternal(setParent = true) {
+    const {parentConnection, childConnection} =
+        this.getParentAndChildConnections();
+    if (!parentConnection || !childConnection) {
+      throw Error('Source connection not connected.');
+    }
+
+    const eventGroup = eventUtils.getGroup();
+    if (!eventGroup) eventUtils.setGroup(true);
+
     let event;
     if (eventUtils.isEnabled()) {
-      event =
-          new (eventUtils.get(eventUtils.BLOCK_MOVE))(childBlock) as BlockMove;
+      event = new (eventUtils.get(eventUtils.BLOCK_MOVE))(
+                  childConnection.getSourceBlock()) as BlockMove;
     }
     const otherConnection = this.targetConnection;
     if (otherConnection) {
       otherConnection.targetConnection = null;
     }
     this.targetConnection = null;
-    childBlock.setParent(null);
+    if (setParent) childConnection.getSourceBlock().setParent(null);
     if (event) {
       event.recordNew();
       eventUtils.fire(event);
     }
+
+    if (!childConnection.getSourceBlock().isShadow()) {
+      // If we were disconnecting a shadow, no need to spawn a new one.
+      parentConnection.respawnShadow_();
+    }
+
+    if (!eventGroup) eventUtils.setGroup(false);
+  }
+
+  /**
+   * Returns the parent connection (superior) and child connection (inferior)
+   * given this connection and the connection it is connected to.
+   *
+   * @returns The parent connection and child connection, given this connection
+   *     and the connection it is connected to.
+   */
+  protected getParentAndChildConnections():
+      {parentConnection?: Connection, childConnection?: Connection} {
+    if (!this.targetConnection) return {};
+    if (this.isSuperior()) {
+      return {
+        parentConnection: this,
+        childConnection: this.targetConnection,
+      };
+    }
+    return {
+      parentConnection: this.targetConnection,
+      childConnection: this,
+    };
   }
 
   /**

core/contextmenu.ts

@@ -117,11 +117,15 @@ function populate_(
     if (option.enabled) {
       const actionHandler = function() {
         hide();
-        // If .scope does not exist on the option, then the callback will not
-        // be expecting a scope parameter, so there should be no problems. Just
-        // assume it is a ContextMenuOption and we'll pass undefined if it's
-        // not.
-        option.callback((option as ContextMenuOption).scope);
+        requestAnimationFrame(() => {
+          setTimeout(() => {
+            // If .scope does not exist on the option, then the callback
+            // will not be expecting a scope parameter, so there should be
+            // no problems. Just assume it is a ContextMenuOption and we'll
+            // pass undefined if it's not.
+            option.callback((option as ContextMenuOption).scope);
+          }, 0);
+        });
       };
       menuItem.onAction(actionHandler, {});
     }

core/insertion_marker_manager.ts

@@ -180,19 +180,14 @@ export class InsertionMarkerManager {
    */
   applyConnections() {
     if (!this.activeCandidate) return;
-    // Don't fire events for insertion markers.
+    const {local, closest} = this.activeCandidate;
+    local.connect(closest);
     eventUtils.disable();
     this.hidePreview();
     eventUtils.enable();
-    const {local, closest} = this.activeCandidate;
-    // Connect two blocks together.
-    local.connect(closest);
     if (this.topBlock.rendered) {
-      // Trigger a connection animation.
-      // Determine which connection is inferior (lower in the source stack).
       const inferiorConnection = local.isSuperior() ? closest : local;
       blockAnimations.connectionUiEffect(inferiorConnection.getSourceBlock());
-      // Bring the just-edited stack to the front.
       const rootBlock = this.topBlock.getRootBlock();
       setTimeout(() => {
         rootBlock.bringToFront();
@@ -604,38 +599,16 @@ export class InsertionMarkerManager {
 
     const markerConn = this.markerConnection;
     const imBlock = markerConn.getSourceBlock();
-    const markerNext = imBlock.nextConnection;
     const markerPrev = imBlock.previousConnection;
     const markerOutput = imBlock.outputConnection;
 
-    const isNext = markerConn === markerNext;
-
-    const isFirstInStatementStack =
-        isNext && !(markerPrev && markerPrev.targetConnection);
-
-    const isFirstInOutputStack =
-        markerConn.type === ConnectionType.INPUT_VALUE &&
-        !(markerOutput && markerOutput.targetConnection);
-    // The insertion marker is the first block in a stack.  Unplug won't do
-    // anything in that case.  Instead, unplug the following block.
-    if (isFirstInStatementStack || isFirstInOutputStack) {
-      markerConn.targetBlock()!.unplug(false);
-    } else if (markerConn.type === ConnectionType.NEXT_STATEMENT && !isNext) {
-      // Inside of a C-block, first statement connection.
-      const innerConnection = markerConn.targetConnection;
-      if (innerConnection) {
-        innerConnection.getSourceBlock().unplug(false);
-      }
-
-      const previousBlockNextConnection =
-          markerPrev ? markerPrev.targetConnection : null;
-
-      imBlock.unplug(true);
-      if (previousBlockNextConnection && innerConnection) {
-        previousBlockNextConnection.connect(innerConnection);
-      }
+    if (!markerPrev?.targetConnection && !markerOutput?.targetConnection) {
+      // If we are the top block, unplugging doesn't do anything.
+      // The marker connection may not have a target block if we are hiding
+      // as part of applying connections.
+      markerConn.targetBlock()?.unplug(false);
     } else {
-      imBlock.unplug(/* healStack */ true);
+      imBlock.unplug(true);
     }
 
     if (markerConn.targetConnection) {

core/rendered_connection.ts

@@ -479,23 +479,27 @@ export class RenderedConnection extends Connection {
   /**
    * Disconnect two blocks that are connected by this connection.
    *
-   * @param parentBlock The superior block.
-   * @param childBlock The inferior block.
+   * @param setParent Whether to set the parent of the disconnected block or
+   *     not, defaults to true.
+   *     If you do not set the parent, ensure that a subsequent action does,
+   *     otherwise the view and model will be out of sync.
    */
-  protected override disconnectInternal_(
-      parentBlock: Block, childBlock: Block) {
-    super.disconnectInternal_(parentBlock, childBlock);
-    const renderedParent = parentBlock as BlockSvg;
-    const renderedChild = childBlock as BlockSvg;
+  override disconnectInternal(setParent = true) {
+    const {parentConnection, childConnection} =
+        this.getParentAndChildConnections();
+    if (!parentConnection || !childConnection) return;
+    const parent = parentConnection.getSourceBlock() as BlockSvg;
+    const child = childConnection.getSourceBlock() as BlockSvg;
+    super.disconnectInternal(setParent);
     // Rerender the parent so that it may reflow.
-    if (renderedParent.rendered) {
-      renderedParent.queueRender();
+    if (parent.rendered) {
+      parent.queueRender();
     }
-    if (renderedChild.rendered) {
-      renderedChild.updateDisabled();
-      renderedChild.queueRender();
+    if (child.rendered) {
+      child.updateDisabled();
+      child.queueRender();
       // Reset visibility, since the child is now a top block.
-      renderedChild.getSvgRoot().style.display = 'block';
+      child.getSvgRoot().style.display = 'block';
     }
   }
 

tests/mocha/block_test.js

@@ -1825,25 +1825,6 @@ suite('Blocks', function() {
         chai.assert.isFalse(blockB.disabled);
         chai.assert.isFalse(blockB.getSvgRoot().classList.contains('blocklyDisabled'));
       });
-      test('Children of Collapsed Block Should Not Update', function() {
-        const blockA = createRenderedBlock(this.workspace, 'statement_block');
-        const blockB = createRenderedBlock(this.workspace, 'stack_block');
-        blockA.getInput('STATEMENT').connection
-            .connect(blockB.previousConnection);
-
-        // Disable the block and collapse it.
-        blockA.setEnabled(false);
-        blockA.setCollapsed(true);
-
-        const blockUpdateDisabled = sinon.stub(blockB, 'updateDisabled');
-
-        // Enable the block before expanding it.
-        blockA.setEnabled(true);
-
-        // For performance reasons updateDisabled should not be called
-        // on children of collapsed blocks.
-        sinon.assert.notCalled(blockUpdateDisabled);
-      });
       test('Disabled Children of Collapsed Blocks Should Stay Disabled', function() {
         const blockA = createRenderedBlock(this.workspace, 'statement_block');
         const blockB = createRenderedBlock(this.workspace, 'stack_block');