From 4dc2869778c1e24f0112c62e34835d2b15b4875c Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Fri, 19 May 2023 15:36:44 -0700 Subject: [PATCH] fix: work on calling icon hooks (#7100) * fix: work on calling icon hooks * chore: PR comments * chore: format --- core/block_svg.ts | 32 +++++++++++++++++-------- core/icons/icon.ts | 14 +++++++++++ core/interfaces/i_icon.ts | 13 +++++++--- core/render_management.ts | 20 ++++++++++++---- core/renderers/common/drawer.ts | 29 +++++++++++++++++------ core/renderers/measurables/icon.ts | 15 ++++++++++-- tests/mocha/block_test.js | 38 +++++++++++++++++++++++++++--- tests/mocha/icon_test.js | 37 ++++++++++++++++++++++------- 8 files changed, 160 insertions(+), 38 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index f50016f1f..cb24eda72 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -39,7 +39,7 @@ import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js' import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import type {CopyData, ICopyable} from './interfaces/i_copyable.js'; import type {IDraggable} from './interfaces/i_draggable.js'; -import type {IIcon} from './interfaces/i_icon.js'; +import {IIcon, isIcon} from './interfaces/i_icon.js'; import * as internalConstants from './internal_constants.js'; import {ASTNode} from './keyboard_nav/ast_node.js'; import {TabNavigateCursor} from './keyboard_nav/tab_navigate_cursor.js'; @@ -197,9 +197,14 @@ export class BlockSvg for (let i = 0, input; (input = this.inputList[i]); i++) { input.init(); } - const icons = this.getIcons(); - for (let i = 0; i < icons.length; i++) { - icons[i].createIcon(); + for (const icon of this.getIcons()) { + if (isIcon(icon)) { + // icon.initView(); + icon.updateEditable(); + } else { + // TODO (#7042): Remove old icon handling. + icon.createIcon(); + } } this.applyColour(); this.pathObject.updateMovable(this.isMovable()); @@ -527,17 +532,21 @@ export class BlockSvg } } + for (const icon of this.getIcons()) { + if (isIcon(icon)) { + icon.updateCollapsed(); + } else if (collapsed) { + // TODO(#7042): Remove old icon handling code. + icon.setVisible(false); + } + } + if (!collapsed) { this.updateDisabled(); this.removeInput(collapsedInputName); return; } - const icons = this.getIcons(); - for (let i = 0, icon; (icon = icons[i]); i++) { - icon.setVisible(false); - } - const text = this.toString(internalConstants.COLLAPSE_CHARS); const field = this.getField(collapsedFieldName); if (field) { @@ -1042,6 +1051,9 @@ export class BlockSvg override addIcon(icon: T): T { super.addIcon(icon); if (this.rendered) { + // icon.initView(); + icon.applyColour(); + icon.updateEditable(); // TODO: Change this based on #7024. this.render(); this.bumpNeighbours(); @@ -1062,7 +1074,7 @@ export class BlockSvg // TODO: remove this implementation after #7038, #7039, and #7040 are // resolved. override getIcons(): AnyDuringMigration[] { - const icons = []; + const icons: AnyDuringMigration = [...this.icons]; if (this.commentIcon_) icons.push(this.commentIcon_); if (this.warning) icons.push(this.warning); if (this.mutator) icons.push(this.mutator); diff --git a/core/icons/icon.ts b/core/icons/icon.ts index 4973b2e51..b1506b24b 100644 --- a/core/icons/icon.ts +++ b/core/icons/icon.ts @@ -38,6 +38,7 @@ export abstract class Icon implements IIcon { const svgBlock = this.sourceBlock as BlockSvg; this.svgRoot = dom.createSvgElement(Svg.G, {'class': 'blocklyIconGroup'}); svgBlock.getSvgRoot().appendChild(this.svgRoot); + this.updateSvgRootOffset(); browserEvents.conditionalBind( this.svgRoot, 'pointerdown', @@ -74,12 +75,25 @@ export abstract class Icon implements IIcon { } } + hideForInsertionMarker(): void { + if (!this.svgRoot) return; + this.svgRoot.style.display = 'none'; + } + isShownWhenCollapsed(): boolean { return false; } setOffsetInBlock(offset: Coordinate): void { this.offsetInBlock = offset; + this.updateSvgRootOffset(); + } + + private updateSvgRootOffset(): void { + this.svgRoot?.setAttribute( + 'transform', + `translate(${this.offsetInBlock.x}, ${this.offsetInBlock.y})` + ); } onLocationChange(blockOrigin: Coordinate): void { diff --git a/core/interfaces/i_icon.ts b/core/interfaces/i_icon.ts index 3e860dd28..8b67e298d 100644 --- a/core/interfaces/i_icon.ts +++ b/core/interfaces/i_icon.ts @@ -44,13 +44,19 @@ export interface IIcon { /** @return The dimensions of the icon for use in rendering. */ getSize(): Size; - /** Notifies the icon that the block's colour has changed. */ + /** Updates the icon's color when the block's color changes.. */ applyColour(): void; - /** Notifies the icon that the block's editability has changed. */ + /** Hides the icon when it is part of an insertion marker. */ + hideForInsertionMarker(): void; + + /** Updates the icon's editability when the block's editability changes. */ updateEditable(): void; - /** Notifies the icon that the block's collapsed-ness has changed. */ + /** + * Updates the icon's collapsed-ness/view when the block's collapsed-ness + * changes. + */ updateCollapsed(): void; /** @@ -88,6 +94,7 @@ export function isIcon(obj: any): obj is IIcon { obj.getWeight !== undefined && obj.getSize !== undefined && obj.applyColour !== undefined && + obj.hideForInsertionMarker !== undefined && obj.updateEditable !== undefined && obj.updateCollapsed !== undefined && obj.isShownWhenCollapsed !== undefined && diff --git a/core/render_management.ts b/core/render_management.ts index c71d7d0bb..8c042fa7d 100644 --- a/core/render_management.ts +++ b/core/render_management.ts @@ -5,6 +5,7 @@ */ import {BlockSvg} from './block_svg.js'; +import {isIcon} from './interfaces/i_icon.js'; import {Coordinate} from './utils/coordinate.js'; /** The set of all blocks in need of rendering which don't have parents. */ @@ -84,8 +85,9 @@ function doRenders() { if (block.getParent()) continue; renderBlock(block); - updateConnectionLocations(block, block.getRelativeToSurfaceXY()); - updateIconLocations(block); + const blockOrigin = block.getRelativeToSurfaceXY(); + updateConnectionLocations(block, blockOrigin); + updateIconLocations(block, blockOrigin); } for (const workspace of workspaces) { workspace.resizeContents(); @@ -138,12 +140,20 @@ function updateConnectionLocations(block: BlockSvg, blockOrigin: Coordinate) { * * @param block The block to update the icon locations of. */ -function updateIconLocations(block: BlockSvg) { +function updateIconLocations(block: BlockSvg, blockOrigin: Coordinate) { if (!block.getIcons) return; for (const icon of block.getIcons()) { - icon.computeIconLocation(); + if (isIcon(icon)) { + icon.onLocationChange(blockOrigin); + } else { + // TODO (#7042): Remove old icon handling code. + icon.computeIconLocation(); + } } for (const child of block.getChildren(false)) { - updateIconLocations(child); + updateIconLocations( + child, + Coordinate.sum(blockOrigin, child.relativeCoords) + ); } } diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index 0efde7721..8f9afc033 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -22,6 +22,7 @@ import {Types} from '../measurables/types.js'; import {isDynamicShape} from './constants.js'; import type {ConstantProvider, Notch, PuzzleTab} from './constants.js'; import type {RenderInfo} from './info.js'; +import {isIcon} from '../../interfaces/i_icon.js'; /** * An object that draws a block based on the given rendering information. @@ -305,12 +306,18 @@ export class Drawer { } } if (Types.isIcon(fieldInfo)) { - svgGroup.setAttribute('display', 'block'); - svgGroup.setAttribute( - 'transform', - 'translate(' + xPos + ',' + yPos + ')' - ); - (fieldInfo as Icon).icon.computeIconLocation(); + const icon = (fieldInfo as Icon).icon; + if (isIcon(icon)) { + icon.setOffsetInBlock(new Coordinate(xPos, yPos)); + } else { + // TODO (#7042): Remove old icon handling code. + svgGroup.setAttribute('display', 'block'); + svgGroup.setAttribute( + 'transform', + 'translate(' + xPos + ',' + yPos + ')' + ); + (fieldInfo as Icon).icon.computeIconLocation(); + } } else { svgGroup.setAttribute( 'transform', @@ -321,7 +328,15 @@ export class Drawer { if (this.info_.isInsertionMarker) { // Fields and icons are invisible on insertion marker. They still have to // be rendered so that the block can be sized correctly. - svgGroup.setAttribute('display', 'none'); + // TODO (#7042): Figure out a better way to handle the types here, + // possibly by splitting this method into submethods. + if (isIcon((fieldInfo as Icon).icon)) { + ( + (fieldInfo as Icon).icon as AnyDuringMigration + ).hideForInsertionMarker(); + } else { + svgGroup.setAttribute('display', 'none'); + } } } diff --git a/core/renderers/measurables/icon.ts b/core/renderers/measurables/icon.ts index 3d2a1126e..f5055c4e4 100644 --- a/core/renderers/measurables/icon.ts +++ b/core/renderers/measurables/icon.ts @@ -13,12 +13,18 @@ import type {ConstantProvider} from '../common/constants.js'; import {Measurable} from './base.js'; import {Types} from './types.js'; +import {hasBubble} from '../../interfaces/i_has_bubble.js'; +import {isIcon} from '../../interfaces/i_icon.js'; /** * An object containing information about the space an icon takes up during * rendering. */ export class Icon extends Measurable { + /** + * @deprecated Will be removed in v11. Create a subclass of the Icon + * measurable if this data is necessary for you. + */ isVisible: boolean; flipRtl = false; @@ -32,10 +38,15 @@ export class Icon extends Measurable { constructor(constants: ConstantProvider, public icon: BlocklyIcon) { super(constants); - this.isVisible = icon.isVisible(); + // TODO(#7042): Remove references to old icon API. + this.isVisible = + (icon.isVisible && icon.isVisible()) || + (hasBubble(icon) && icon.isBubbleVisible()); this.type |= Types.ICON; - const size = icon.getCorrectedSize(); + const size = + (icon.getCorrectedSize && icon.getCorrectedSize()) || + (isIcon(icon) && icon.getSize()); this.height = size.height; this.width = size.width; } diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index bc4a65b06..815df161b 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -1158,7 +1158,7 @@ suite('Blocks', function () { }); }); suite('Add Connections Programmatically', function () { - test('Output', function () { + test('Output', async function () { const block = createRenderedBlock(this.workspace, 'empty_block'); block.setOutput(true); @@ -1418,7 +1418,39 @@ suite('Blocks', function () { }); suite('Icon management', function () { - class MockIconA { + class MockIcon { + getType() { + return 'mock icon'; + } + + initView() {} + + dispose() {} + + getWeight() {} + + getSize() { + return new Blockly.utils.Size(0, 0); + } + + applyColour() {} + + hideForInsertionMarker() {} + + updateEditable() {} + + updateCollapsed() {} + + isShownWhenCollapsed() {} + + setOffsetInBlock() {} + + onLocationChange() {} + + onClick() {} + } + + class MockIconA extends MockIcon { getType() { return 'A'; } @@ -1428,7 +1460,7 @@ suite('Blocks', function () { } } - class MockIconB { + class MockIconB extends MockIcon { getType() { return 'B'; } diff --git a/tests/mocha/icon_test.js b/tests/mocha/icon_test.js index 12bd039f5..f873de0ae 100644 --- a/tests/mocha/icon_test.js +++ b/tests/mocha/icon_test.js @@ -29,11 +29,29 @@ suite('Icon', function () { initView() {} + dispose() {} + + getWeight() {} + + getSize() { + return new Blockly.utils.Size(0, 0); + } + applyColour() {} + hideForInsertionMarker() {} + updateEditable() {} updateCollapsed() {} + + isShownWhenCollapsed() {} + + setOffsetInBlock() {} + + onLocationChange() {} + + onClick() {} } class MockSerializableIcon extends MockIcon { @@ -90,7 +108,7 @@ suite('Icon', function () { return createUninitializedBlock(workspace); } - suite.skip('Hooks getting properly triggered by the block', function () { + suite('Hooks getting properly triggered by the block', function () { suite('Triggering view initialization', function () { test('initView is not called by headless blocks', function () { const workspace = createHeadlessWorkspace(); @@ -106,7 +124,7 @@ suite('Icon', function () { ); }); - test('initView is called by headful blocks during initSvg', function () { + test.skip('initView is called by headful blocks during initSvg', function () { const workspace = createWorkspaceSvg(); const block = createUninitializedBlock(workspace); const icon = new MockIcon(); @@ -124,7 +142,7 @@ suite('Icon', function () { ); }); - test( + test.skip( 'initView is called by headful blocks that are currently ' + 'rendered when the icon is added', function () { @@ -231,7 +249,7 @@ suite('Icon', function () { block.addIcon(icon); applyColourSpy.resetHistory(); - block.setDisabled(true); + block.setEnabled(false); chai.assert.isTrue( applyColourSpy.calledOnce, 'Expected applyColour to be called' @@ -357,13 +375,15 @@ suite('Icon', function () { const workspace = createWorkspaceSvg(); const block = createInitializedBlock(workspace); const icon = new MockIcon(); - const updateCollapsedSpy = sinon.spy(icon, 'updateCollapsed'); + const updateCollapsedSpy = sinon.stub(icon, 'updateCollapsed'); block.addIcon(icon); + updateCollapsedSpy.resetHistory(); block.setCollapsed(true); + this.clock.runAll(); chai.assert.isTrue( - updateCollapsedSpy.calledOnce, + updateCollapsedSpy.called, 'Expected updateCollapsed to be called' ); }); @@ -377,10 +397,11 @@ suite('Icon', function () { block.setCollapsed(true); block.setCollapsed(false); + this.clock.runAll(); chai.assert.isTrue( - updateCollapsedSpy.calledTwice, - 'Expected updateCollapsed to be called twice' + updateCollapsedSpy.called, + 'Expected updateCollapsed to be called' ); }); });