From a2d1faabc238883f84f00d616946e98c22f39547 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 19:54:31 +0000 Subject: [PATCH 01/11] fix: early return from updateDisabled if it is noop --- core/block_svg.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index ee21986f848..a4d22b3f128 100644 --- a/core/block_svg.ts +++ b/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(); } } From 76cf448372e3c8e4a89c987077e1faf4939cb449 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 21:29:29 +0000 Subject: [PATCH 02/11] chore: trigger connect and disconnect before hiding --- core/insertion_marker_manager.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/core/insertion_marker_manager.ts b/core/insertion_marker_manager.ts index 20e27d42650..ad29d27c121 100644 --- a/core/insertion_marker_manager.ts +++ b/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(); From 441a5acba20c23ba3206bc4b333ccc9c1a771667 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 21:30:29 +0000 Subject: [PATCH 03/11] chore: remove disconnectInternal --- core/connection.ts | 71 +++++++++++++++---------------------- core/rendered_connection.ts | 24 +++++++------ 2 files changed, 42 insertions(+), 53 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index 76536aa8bb5..bd5576e5cc4 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -245,65 +245,52 @@ export class Connection implements IASTNodeLocationWithBlock { /** Disconnect this connection. */ disconnect() { - const otherConnection = this.targetConnection; - if (!otherConnection) { + const {parentConnection, childConnection} = + this.getParentAndChildConnections(); + if (!parentConnection || !childConnection) { 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); - } - } + if (!eventGroup) eventUtils.setGroup(true); - /** - * Disconnect two blocks that are connected by this connection. - * - * @param parentBlock The superior block. - * @param childBlock The inferior block. - */ - protected disconnectInternal_(parentBlock: Block, childBlock: Block) { 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); + 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); + } + + 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, + }; } /** diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index dbe729dd321..70d269b6418 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -482,21 +482,23 @@ export class RenderedConnection extends Connection { * @param parentBlock The superior block. * @param childBlock The inferior block. */ - protected override disconnectInternal_( - parentBlock: Block, childBlock: Block) { - super.disconnectInternal_(parentBlock, childBlock); - const renderedParent = parentBlock as BlockSvg; - const renderedChild = childBlock as BlockSvg; + override disconnect() { + const {parentConnection, childConnection} = + this.getParentAndChildConnections(); + if (!parentConnection || !childConnection) return; + const parent = parentConnection.getSourceBlock() as BlockSvg; + const child = childConnection.getSourceBlock() as BlockSvg; // 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'; } + super.disconnect(); } /** From 8d60bb233b1e974482f5399fe684fe36c97f6034 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 21:38:29 +0000 Subject: [PATCH 04/11] fix: skip setting parent when disconnecting before connecting --- core/connection.ts | 6 +++--- core/rendered_connection.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index bd5576e5cc4..5d1ea58c84b 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -93,7 +93,7 @@ export class Connection implements IASTNodeLocationWithBlock { // Make sure the childConnection is available. if (childConnection.isConnected()) { - childConnection.disconnect(); + childConnection.disconnect(false); } // Make sure the parentConnection is available. @@ -244,7 +244,7 @@ export class Connection implements IASTNodeLocationWithBlock { } /** Disconnect this connection. */ - disconnect() { + disconnect(setParent = true) { const {parentConnection, childConnection} = this.getParentAndChildConnections(); if (!parentConnection || !childConnection) { @@ -264,7 +264,7 @@ export class Connection implements IASTNodeLocationWithBlock { otherConnection.targetConnection = null; } this.targetConnection = null; - childConnection.getSourceBlock().setParent(null); + if (setParent) childConnection.getSourceBlock().setParent(null); if (event) { event.recordNew(); eventUtils.fire(event); diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 70d269b6418..eacc4884c23 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -482,7 +482,7 @@ export class RenderedConnection extends Connection { * @param parentBlock The superior block. * @param childBlock The inferior block. */ - override disconnect() { + override disconnect(setParent = true) { const {parentConnection, childConnection} = this.getParentAndChildConnections(); if (!parentConnection || !childConnection) return; @@ -498,7 +498,7 @@ export class RenderedConnection extends Connection { // Reset visibility, since the child is now a top block. child.getSvgRoot().style.display = 'block'; } - super.disconnect(); + super.disconnect(setParent); } /** From 9f7eec7f70e6f44b8aa343ebe3ee81b778cc9404 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 22:16:54 +0000 Subject: [PATCH 05/11] chore: fixup docs --- core/connection.ts | 11 ++++++++++- core/rendered_connection.ts | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index 5d1ea58c84b..f6ec8019312 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -243,7 +243,12 @@ export class Connection implements IASTNodeLocationWithBlock { return this.isConnected(); } - /** Disconnect this connection. */ + /** + * Disconnect this connection. + * + * @param setParent Whether to set the parent of the disconnected block or + * not, defaults to true. + */ disconnect(setParent = true) { const {parentConnection, childConnection} = this.getParentAndChildConnections(); @@ -278,6 +283,10 @@ export class Connection implements IASTNodeLocationWithBlock { if (!eventGroup) eventUtils.setGroup(false); } + /** + * @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 {}; diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index eacc4884c23..a8ccd1d2c7e 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -479,8 +479,8 @@ 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. */ override disconnect(setParent = true) { const {parentConnection, childConnection} = From 05e602978ded0acdce82699014936ff16a79807c Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 2 Mar 2023 23:01:56 +0000 Subject: [PATCH 06/11] chore: remove erroneous test --- tests/mocha/block_test.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index d04868b560d..3c1ecc95c1c 100644 --- a/tests/mocha/block_test.js +++ b/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'); From 99b95a4c322002b069d968cd564bc85947e669be Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 6 Mar 2023 14:37:38 +0000 Subject: [PATCH 07/11] fix: add delay to context menu callback. Improve INP by allowing the browser to do a paint (closing the context menu) before we trigger callbacks. This improves the user experience for expensive callbacks (e.g. collapsing, or updating disabled). --- core/contextmenu.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/core/contextmenu.ts b/core/contextmenu.ts index ec56d90382b..8122b1b03c4 100644 --- a/core/contextmenu.ts +++ b/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, {}); } From 995fa1de8985e4347d3cd51ef418b974a20fbae5 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 8 Mar 2023 15:54:40 +0000 Subject: [PATCH 08/11] fix: rendering bug When disconnecting the last block in the stack, the block would not be rerendered correctly (the top-start corner would not be reshaped) --- core/rendered_connection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index a8ccd1d2c7e..4696db23960 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -488,6 +488,7 @@ export class RenderedConnection extends Connection { if (!parentConnection || !childConnection) return; const parent = parentConnection.getSourceBlock() as BlockSvg; const child = childConnection.getSourceBlock() as BlockSvg; + super.disconnect(setParent); // Rerender the parent so that it may reflow. if (parent.rendered) { parent.queueRender(); @@ -498,7 +499,6 @@ export class RenderedConnection extends Connection { // Reset visibility, since the child is now a top block. child.getSvgRoot().style.display = 'block'; } - super.disconnect(setParent); } /** From 7e2129b5a7c48f9ca9a9d4758b1ea7212b98eeb3 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 8 Mar 2023 15:55:34 +0000 Subject: [PATCH 09/11] fix: connecting bug The order for applying connections was changed so that connections were applied and then the insertion marker was hidden. This caused an error because hiding the insertion marker expected there to be a child block when there was not. --- core/insertion_marker_manager.ts | 34 ++++++-------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/core/insertion_marker_manager.ts b/core/insertion_marker_manager.ts index ad29d27c121..532fb11bed8 100644 --- a/core/insertion_marker_manager.ts +++ b/core/insertion_marker_manager.ts @@ -599,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) { From aa6751ea34d77cbadfd7f6a097b9a34a2f754cff Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 15 Mar 2023 18:47:58 +0000 Subject: [PATCH 10/11] chore: remove setParent param from public API --- core/connection.ts | 15 ++++++++++++--- core/rendered_connection.ts | 6 ++++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index f6ec8019312..6247e8ce6c0 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -93,7 +93,7 @@ export class Connection implements IASTNodeLocationWithBlock { // Make sure the childConnection is available. if (childConnection.isConnected()) { - childConnection.disconnect(false); + 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); @@ -245,11 +245,20 @@ export class Connection implements IASTNodeLocationWithBlock { /** * Disconnect this connection. + */ + disconnect() { + this.disconnectInternal(); + } + + /** + * Disconnect two blocks that are connected by this connection. * * @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. */ - disconnect(setParent = true) { + protected disconnectInternal(setParent = true) { const {parentConnection, childConnection} = this.getParentAndChildConnections(); if (!parentConnection || !childConnection) { diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 4696db23960..583a9aff8e3 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -481,14 +481,16 @@ export class RenderedConnection extends Connection { * * @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. */ - override disconnect(setParent = true) { + 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.disconnect(setParent); + super.disconnectInternal(setParent); // Rerender the parent so that it may reflow. if (parent.rendered) { parent.queueRender(); From e94cca185f1bd4ae911a7fdb9821be8ad52ddc08 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 15 Mar 2023 18:58:43 +0000 Subject: [PATCH 11/11] chore: tsdoc --- core/connection.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/connection.ts b/core/connection.ts index 6247e8ce6c0..485f3895ec4 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -293,6 +293,9 @@ export class Connection implements IASTNodeLocationWithBlock { } /** + * 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. */