From 8b0c40bb1baa0daa4118b9e7e9f08d9d728f85f7 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 11 May 2023 15:48:01 -0700 Subject: [PATCH] feat: add implementations for adding, removing, and getting icons (#7059) * feat: add implementations for adding, removing, and getting icons * chore: fix tests * chore: switch order of adding icons * chore: create duplicate icon error * chore: un-only test --- core/block.ts | 55 +++++++++++++++++++++++++++++++--- core/block_svg.ts | 51 ++++++++++++++++++------------- core/blockly.ts | 2 ++ core/icons.ts | 9 ++++++ core/icons/exceptions.ts | 20 +++++++++++++ tests/mocha/block_test.js | 63 +++++++++++++++++++++++++-------------- 6 files changed, 154 insertions(+), 46 deletions(-) create mode 100644 core/icons.ts create mode 100644 core/icons/exceptions.ts diff --git a/core/block.ts b/core/block.ts index 541966ae1..2903e94f7 100644 --- a/core/block.ts +++ b/core/block.ts @@ -25,6 +25,7 @@ import * as common from './common.js'; import {Connection} from './connection.js'; import {ConnectionType} from './connection_type.js'; import * as constants from './constants.js'; +import {DuplicateIconType} from './icons/exceptions.js'; import type {Abstract} from './events/events_abstract.js'; import type {BlockMove} from './events/events_block_move.js'; import * as eventUtils from './events/utils.js'; @@ -34,6 +35,7 @@ import * as fieldRegistry from './field_registry.js'; import {Align, Input} from './inputs/input.js'; import type {IASTNodeLocation} from './interfaces/i_ast_node_location.js'; import type {IDeletable} from './interfaces/i_deletable.js'; +import type {IIcon} from './interfaces/i_icon.js'; import type {Mutator} from './mutator.js'; import * as Tooltip from './tooltip.js'; import * as arrayUtils from './utils/array.js'; @@ -156,6 +158,7 @@ export class Block implements IASTNodeLocation, IDeletable { previousConnection: Connection | null = null; inputList: Input[] = []; inputsInline?: boolean; + icons: IIcon[] = []; private disabled = false; tooltip: Tooltip.TipInfo = ''; contextMenu = true; @@ -2217,16 +2220,60 @@ export class Block implements IASTNodeLocation, IDeletable { * @param _opt_id An optional ID for the warning text to be able to maintain * multiple warnings. */ - setWarningText(_text: string | null, _opt_id?: string) {} - // NOP. + setWarningText(_text: string | null, _opt_id?: string) { + // NOOP. + } /** * Give this block a mutator dialog. * * @param _mutator A mutator dialog instance or null to remove. */ - setMutator(_mutator: Mutator) {} - // NOP. + setMutator(_mutator: Mutator) { + // NOOP. + } + + /** Adds the given icon to the block. */ + addIcon(icon: T): T { + if (this.hasIcon(icon.getType())) throw new DuplicateIconType(icon); + this.icons.push(icon); + this.icons.sort((a, b) => a.getWeight() - b.getWeight()); + return icon; + } + + /** + * Removes the icon whose getType matches the given type iconType from the + * block. + * + * @param type The type of the icon to remove from the block. + * @return True if an icon with the given type was found, false otherwise. + */ + removeIcon(type: string): boolean { + if (!this.hasIcon(type)) return false; + this.icons = this.icons.filter((icon) => icon.getType() !== type); + return true; + } + + /** + * @return True if an icon with the given type exists on the block, + * false otherwise. + */ + hasIcon(type: string): boolean { + return this.icons.some((icon) => icon.getType() === type); + } + + /** + * @return The icon with the given type if it exists on the block, undefined + * otherwise. + */ + getIcon(type: string): IIcon | undefined { + return this.icons.find((icon) => icon.getType() === type); + } + + /** @return An array of the icons attached to this block. */ + getIcons(): IIcon[] { + return [...this.icons]; + } /** * Return the coordinates of the top-left corner of this block relative to the diff --git a/core/block_svg.ts b/core/block_svg.ts index cd8fce5ef..dbb549a09 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -34,12 +34,12 @@ import type {BlockMove} from './events/events_block_move.js'; import * as eventUtils from './events/utils.js'; import type {Field} from './field.js'; import {FieldLabel} from './field_label.js'; -import type {Icon} from './icon_old.js'; import type {Input} from './inputs/input.js'; 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 * as internalConstants from './internal_constants.js'; import {ASTNode} from './keyboard_nav/ast_node.js'; import {TabNavigateCursor} from './keyboard_nav/tab_navigate_cursor.js'; @@ -305,25 +305,6 @@ export class BlockSvg this.removeSelect(); } - /** - * Returns a list of mutator, comment, and warning icons. - * - * @returns List of icons. - */ - getIcons(): Icon[] { - const icons = []; - if (this.mutator) { - icons.push(this.mutator); - } - if (this.commentIcon_) { - icons.push(this.commentIcon_); - } - if (this.warning) { - icons.push(this.warning); - } - return icons; - } - /** * Sets the parent of this block to be a new block or null. * @@ -1145,6 +1126,36 @@ export class BlockSvg } } + override addIcon(icon: T): T { + super.addIcon(icon); + if (this.rendered) { + // TODO: Change this based on #7024. + this.render(); + this.bumpNeighbours(); + } + return icon; + } + + override removeIcon(type: string): boolean { + const removed = super.removeIcon(type); + if (this.rendered) { + // TODO: Change this based on #7024. + this.render(); + this.bumpNeighbours(); + } + return removed; + } + + // TODO: remove this implementation after #7038, #7039, and #7040 are + // resolved. + override getIcons(): AnyDuringMigration[] { + const icons = []; + if (this.commentIcon_) icons.push(this.commentIcon_); + if (this.warning) icons.push(this.warning); + if (this.mutator) icons.push(this.mutator); + return icons; + } + /** * Set whether the block is enabled or not. * diff --git a/core/blockly.ts b/core/blockly.ts index 8d91463ae..a6a5dbd4f 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -124,6 +124,7 @@ import {CodeGenerator} from './generator.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; import {Icon} from './icon_old.js'; +import * as icons from './icons.js'; import {inject} from './inject.js'; import {Align, Input} from './inputs/input.js'; import {inputTypes} from './inputs/input_types.js'; @@ -586,6 +587,7 @@ export {IComponent}; export {IConnectionChecker}; export {IContextMenu}; export {Icon}; +export {icons}; export {ICopyable}; export {IDeletable}; export {IDeleteArea}; diff --git a/core/icons.ts b/core/icons.ts new file mode 100644 index 000000000..49b435634 --- /dev/null +++ b/core/icons.ts @@ -0,0 +1,9 @@ +/** + * @license + * Copyright 2023 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as exceptions from './icons/exceptions.js'; + +export {exceptions}; diff --git a/core/icons/exceptions.ts b/core/icons/exceptions.ts new file mode 100644 index 000000000..58dabaacf --- /dev/null +++ b/core/icons/exceptions.ts @@ -0,0 +1,20 @@ +/** + * @license + * Copyright 2023 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type {IIcon} from '../interfaces/i_icon.js'; + +export class DuplicateIconType extends Error { + /** + * @internal + */ + constructor(public icon: IIcon) { + super( + `Tried to append an icon of type ${icon.getType()} when an icon of ` + + `that type already exists on the block. ` + + `Use getIcon to access the existing icon.` + ); + } +} diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 284390d73..bc4a65b06 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -1422,22 +1422,30 @@ suite('Blocks', function () { getType() { return 'A'; } + + getWeight() { + return 1; + } } class MockIconB { getType() { return 'B'; } + + getWeight() { + return 2; + } } - suite.skip('Adding icons', function () { + suite('Adding icons', function () { setup(function () { - // Tear down the old headless workspace and create a new rendered one. - workspaceTeardown.call(this, this.workspace); - this.workspace = Blockly.inject('blocklyDiv'); + this.workspace = Blockly.inject('blocklyDiv', {}); this.block = this.workspace.newBlock('stack_block'); - this.renderSpy = sinon.spy(this.block, 'queueRender'); + this.block.initSvg(); + this.block.render(); + this.renderSpy = sinon.spy(this.block, 'render'); }); teardown(function () { @@ -1455,9 +1463,14 @@ suite('Blocks', function () { test('adding two icons of the same type throws', function () { this.block.addIcon(new MockIconA()); - chai.assert.throws(() => { - this.block.addIcon(new MockIconA()); - }, 'Expected adding an icon of the same type to throw'); + chai.assert.throws( + () => { + this.block.addIcon(new MockIconA()); + }, + Blockly.icons.DuplicateIconType, + '', + 'Expected adding an icon of the same type to throw' + ); }); test('adding an icon triggers a render', function () { @@ -1470,14 +1483,14 @@ suite('Blocks', function () { }); }); - suite.skip('Removing icons', function () { + suite('Removing icons', function () { setup(function () { - // Tear down the old headless workspace and create a new rendered one. - workspaceTeardown.call(this, this.workspace); this.workspace = Blockly.inject('blocklyDiv'); this.block = this.workspace.newBlock('stack_block'); - this.renderSpy = sinon.spy(this.block, 'queueRender'); + this.block.initSvg(); + this.block.render(); + this.renderSpy = sinon.spy(this.block, 'render'); }); teardown(function () { @@ -1505,8 +1518,8 @@ suite('Blocks', function () { }); test('removing an icon triggers a render', function () { - this.renderSpy.resetHistory(); this.block.addIcon(new MockIconA()); + this.renderSpy.resetHistory(); this.block.removeIcon('A'); chai.assert.isTrue( this.renderSpy.calledOnce, @@ -1515,20 +1528,28 @@ suite('Blocks', function () { }); }); - suite.skip('Getting icons', function () { + suite('Getting icons', function () { setup(function () { this.block = this.workspace.newBlock('stack_block'); }); - test('all icons are returned from getIcons', function () { + test('all icons are returned from getIcons, in order of weight', function () { const iconA = new MockIconA(); const iconB = new MockIconB(); - this.block.addIcon(iconA); this.block.addIcon(iconB); - chai.assert.sameMembers( + this.block.addIcon(iconA); + chai.assert.sameOrderedMembers( this.block.getIcons(), [iconA, iconB], - 'Expected getIcon to return both icons' + 'Expected getIcon to return both icons in order of weight' + ); + }); + + test('if there are no icons, getIcons returns an empty array', function () { + chai.assert.isEmpty( + this.block.getIcons(), + 'Expected getIcons to return an empty array ' + + 'for a block with no icons' ); }); @@ -1552,9 +1573,9 @@ suite('Blocks', function () { ); }); - test('if there is no matching icon, getIcon returns null', function () { + test('if there is no matching icon, getIcon returns undefined', function () { this.block.addIcon(new MockIconA()); - chai.assert.isNull( + chai.assert.isUndefined( this.block.getIcon('B'), 'Expected getIcon to return null if there is no ' + 'icon with a matching type' @@ -1564,8 +1585,6 @@ suite('Blocks', function () { suite('Bubbles and collapsing', function () { setup(function () { - // Tear down the old headless workspace and create a new rendered one. - workspaceTeardown.call(this, this.workspace); this.workspace = Blockly.inject('blocklyDiv'); });