From aeee27876789d0404aaa7c4eaed9976872a18683 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Tue, 13 Jun 2023 15:41:07 -0700 Subject: [PATCH] fix: remove old icon handling code (#7141) * fix: remove old icon handling code * fix: remove calls to getCommentIcon * chore: delete the old icon class --- core/block_dragger.ts | 26 +--- core/block_svg.ts | 38 +----- core/contextmenu_items.ts | 5 +- core/icon_old.ts | 194 ----------------------------- core/icons/icon.ts | 7 +- core/render_management.ts | 8 +- core/rendered_connection.ts | 7 +- core/renderers/common/drawer.ts | 52 +++----- core/renderers/common/info.ts | 6 +- core/renderers/geras/drawer.ts | 1 - core/renderers/measurables/icon.ts | 12 +- core/renderers/zelos/drawer.ts | 1 - core/serialization/blocks.ts | 7 +- 13 files changed, 38 insertions(+), 326 deletions(-) delete mode 100644 core/icon_old.ts diff --git a/core/block_dragger.ts b/core/block_dragger.ts index 399763a93..1e6d34a7d 100644 --- a/core/block_dragger.ts +++ b/core/block_dragger.ts @@ -21,8 +21,7 @@ import * as bumpObjects from './bump_objects.js'; import * as common from './common.js'; import type {BlockMove} from './events/events_block_move.js'; import * as eventUtils from './events/utils.js'; -import type {Icon} from './icon_old.js'; -import {isIcon} from './interfaces/i_icon.js'; +import type {Icon} from './icons/icon.js'; import {InsertionMarkerManager} from './insertion_marker_manager.js'; import type {IBlockDragger} from './interfaces/i_block_dragger.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; @@ -409,12 +408,7 @@ export class BlockDragger implements IBlockDragger { protected dragIcons_(dxy: Coordinate) { // Moving icons moves their associated bubbles. for (const data of this.dragIconData_) { - if (isIcon(data.icon)) { - data.icon.onLocationChange(Coordinate.sum(data.location, dxy)); - } else { - // TODO: Remove old icon handling logic. - data.icon.setIconLocation(Coordinate.sum(data.location, dxy)); - } + data.icon.onLocationChange(Coordinate.sum(data.location, dxy)); } } @@ -461,21 +455,9 @@ function initIconData( for (const icon of block.getIcons()) { // Only bother to track icons whose bubble is visible. if (hasBubble(icon) && !icon.bubbleIsVisible()) continue; - // TODO (#7042): Remove old icon handling code. - if (icon.isVisible && !icon.isVisible()) continue; - if (isIcon(icon)) { - dragIconData.push({location: blockOrigin, icon: icon}); - icon.onLocationChange(blockOrigin); - } else { - // TODO (#7042): Remove old icon handling code. - dragIconData.push({ - // Coordinate with x and y properties (workspace - // coordinates). - location: icon.getIconLocation(), // Blockly.Icon - icon: icon, - }); - } + dragIconData.push({location: blockOrigin, icon: icon}); + icon.onLocationChange(blockOrigin); } for (const child of block.getChildren(false)) { diff --git a/core/block_svg.ts b/core/block_svg.ts index 82204f76d..f24e0e085 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 {IIcon, isIcon} from './interfaces/i_icon.js'; +import {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'; @@ -201,13 +201,8 @@ export class BlockSvg input.init(); } for (const icon of this.getIcons()) { - if (isIcon(icon)) { - icon.initView(this.createIconPointerDownListener(icon)); - icon.updateEditable(); - } else { - // TODO (#7042): Remove old icon handling. - icon.createIcon(); - } + icon.initView(this.createIconPointerDownListener(icon)); + icon.updateEditable(); } this.applyColour(); this.pathObject.updateMovable(this.isMovable()); @@ -536,12 +531,7 @@ 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); - } + icon.updateCollapsed(); } if (!collapsed) { @@ -676,12 +666,7 @@ export class BlockSvg const icons = this.getIcons(); const pos = this.getRelativeToSurfaceXY(); for (const icon of icons) { - if (isIcon(icon)) { - icon.onLocationChange(pos); - } else { - // TODO (#7042): Remove old icon handling code. - icon.computeIconLocation(); - } + icon.onLocationChange(pos); } // Recurse through all blocks attached under this one. @@ -1042,12 +1027,6 @@ export class BlockSvg return removed; } - // TODO: remove this implementation after #7038, #7039, and #7040 are - // resolved. - override getIcons(): AnyDuringMigration[] { - return [...this.icons]; - } - /** * Set whether the block is enabled or not. * @@ -1711,12 +1690,7 @@ export class BlockSvg } for (const icon of this.getIcons()) { - if (isIcon(icon)) { - icon.onLocationChange(blockTL); - } - // TODO (#7042): Remove the below comment. - // Updating the positions of old style icons is handled directly in the - // drawer. + icon.onLocationChange(blockTL); } } diff --git a/core/contextmenu_items.ts b/core/contextmenu_items.ts index 77ab48433..ab0b104fc 100644 --- a/core/contextmenu_items.ts +++ b/core/contextmenu_items.ts @@ -17,6 +17,7 @@ import { import * as dialog from './dialog.js'; import * as Events from './events/events.js'; import * as eventUtils from './events/utils.js'; +import {CommentIcon} from './icons/comment_icon.js'; import {Msg} from './msg.js'; import {StatementInput} from './renderers/zelos/zelos.js'; import type {WorkspaceSvg} from './workspace_svg.js'; @@ -347,7 +348,7 @@ export function registerDuplicate() { export function registerComment() { const commentOption: RegistryItem = { displayText(scope: Scope) { - if (scope.block!.getCommentIcon()) { + if (scope.block!.hasIcon(CommentIcon.TYPE)) { // If there's already a comment, option is to remove. return Msg['REMOVE_COMMENT']; } @@ -368,7 +369,7 @@ export function registerComment() { }, callback(scope: Scope) { const block = scope.block; - if (block!.getCommentIcon()) { + if (block!.hasIcon(CommentIcon.TYPE)) { block!.setCommentText(null); } else { block!.setCommentText(''); diff --git a/core/icon_old.ts b/core/icon_old.ts deleted file mode 100644 index f0f6ac97d..000000000 --- a/core/icon_old.ts +++ /dev/null @@ -1,194 +0,0 @@ -/** - * @license - * Copyright 2013 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * Object representing an icon on a block. - * - * @class - */ -import * as goog from '../closure/goog/goog.js'; -goog.declareModuleId('Blockly.Icon'); - -import type {BlockSvg} from './block_svg.js'; -import * as browserEvents from './browser_events.js'; -import type {Bubble} from './bubble_old.js'; -import {Coordinate} from './utils/coordinate.js'; -import * as dom from './utils/dom.js'; -import {Size} from './utils/size.js'; -import {Svg} from './utils/svg.js'; -import * as svgMath from './utils/svg_math.js'; - -/** - * Class for an icon. - */ -export abstract class Icon { - protected block_: BlockSvg; - /** The icon SVG group. */ - iconGroup_: SVGGElement | null = null; - - /** Whether this icon gets hidden when the block is collapsed. */ - collapseHidden = true; - - /** Height and width of icons. */ - readonly SIZE = 17; - - /** Bubble UI (if visible). */ - protected bubble_: Bubble | null = null; - - /** Absolute coordinate of icon's center. */ - protected iconXY_: Coordinate | null = null; - - /** @param block The block associated with this icon. */ - constructor(block: BlockSvg) { - this.block_ = block; - } - - /** Create the icon on the block. */ - createIcon() { - if (this.iconGroup_) { - // Icon already exists. - return; - } - /* Here's the markup that will be generated: - - ... - - */ - this.iconGroup_ = dom.createSvgElement(Svg.G, { - 'class': 'blocklyIconGroup', - }); - if (this.getBlock().isInFlyout) { - dom.addClass(this.iconGroup_, 'blocklyIconGroupReadonly'); - } - this.drawIcon_(this.iconGroup_); - - this.getBlock().getSvgRoot().appendChild(this.iconGroup_); - browserEvents.conditionalBind( - this.iconGroup_, - 'pointerup', - this, - this.iconClick_ - ); - this.updateEditable(); - } - - /** Dispose of this icon. */ - dispose() { - if (!this.getBlock().isDeadOrDying()) { - dom.removeNode(this.iconGroup_); - } - this.setVisible(false); // Dispose of and unlink the bubble. - } - - /** Add or remove the UI indicating if this icon may be clicked or not. */ - updateEditable() { - // No-op on the base class. - } - - /** - * Is the associated bubble visible? - * - * @returns True if the bubble is visible. - */ - isVisible(): boolean { - return !!this.bubble_; - } - - /** - * Clicking on the icon toggles if the bubble is visible. - * - * @param e Mouse click event. - */ - protected iconClick_(e: PointerEvent) { - if (this.getBlock().workspace.isDragging()) { - // Drag operation is concluding. Don't open the editor. - return; - } - if (!this.getBlock().isInFlyout && !browserEvents.isRightButton(e)) { - this.setVisible(!this.isVisible()); - } - } - - /** Change the colour of the associated bubble to match its block. */ - applyColour() { - if (this.bubble_ && this.isVisible()) { - this.bubble_.setColour(this.getBlock().style.colourPrimary); - } - } - - /** - * Notification that the icon has moved. Update the arrow accordingly. - * - * @param xy Absolute location in workspace coordinates. - */ - setIconLocation(xy: Coordinate) { - this.iconXY_ = xy; - if (this.bubble_ && this.isVisible()) { - this.bubble_.setAnchorLocation(xy); - } - } - - /** - * Notification that the icon has moved, but we don't really know where. - * Recompute the icon's location from scratch. - */ - computeIconLocation() { - // Find coordinates for the centre of the icon and update the arrow. - const blockXY = this.getBlock().getRelativeToSurfaceXY(); - const iconXY = svgMath.getRelativeXY(this.iconGroup_ as SVGElement); - const newXY = new Coordinate( - blockXY.x + iconXY.x + this.SIZE / 2, - blockXY.y + iconXY.y + this.SIZE / 2 - ); - if (!Coordinate.equals(this.getIconLocation(), newXY)) { - this.setIconLocation(newXY); - } - } - - /** - * Returns the center of the block's icon relative to the surface. - * - * @returns Object with x and y properties in workspace coordinates. - */ - getIconLocation(): Coordinate | null { - return this.iconXY_; - } - - /** - * Get the size of the icon as used for rendering. - * This differs from the actual size of the icon, because it bulges slightly - * out of its row rather than increasing the height of its row. - * - * @returns Height and width. - */ - getCorrectedSize(): Size { - // TODO (#2562): Remove getCorrectedSize. - return new Size(this.SIZE, this.SIZE - 2); - } - - /** - * Draw the icon. - * - * @param _group The icon group. - */ - protected drawIcon_(_group: Element) {} - // No-op on base class. - - /** - * Show or hide the bubble. - * - * @param _visible True if the bubble should be visible. - */ - setVisible(_visible: boolean) {} - - /** - * @returns The block this icon is attached to. - */ - protected getBlock(): BlockSvg { - return this.block_; - } -} -// No-op on base class diff --git a/core/icons/icon.ts b/core/icons/icon.ts index fa704f33c..839894ca6 100644 --- a/core/icons/icon.ts +++ b/core/icons/icon.ts @@ -66,12 +66,7 @@ export abstract class Icon implements IIcon { updateEditable(): void {} updateCollapsed(): void { - if (!this.svgRoot) { - throw new Error( - 'Attempt to update the collapsed-ness of an icon before its ' + - 'view has been initialized.' - ); - } + if (!this.svgRoot) return; if (this.sourceBlock.isCollapsed()) { this.svgRoot.style.display = 'none'; } else { diff --git a/core/render_management.ts b/core/render_management.ts index fc301623b..8c469f856 100644 --- a/core/render_management.ts +++ b/core/render_management.ts @@ -5,7 +5,6 @@ */ 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. */ @@ -143,12 +142,7 @@ function updateConnectionLocations(block: BlockSvg, blockOrigin: Coordinate) { function updateIconLocations(block: BlockSvg, blockOrigin: Coordinate) { if (!block.getIcons) return; for (const icon of block.getIcons()) { - if (isIcon(icon)) { - icon.onLocationChange(blockOrigin); - } else { - // TODO (#7042): Remove old icon handling code. - icon.computeIconLocation(); - } + icon.onLocationChange(blockOrigin); } for (const child of block.getChildren(false)) { updateIconLocations( diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index b4dc647f1..c1e01f86c 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -430,12 +430,7 @@ export class RenderedConnection extends Connection { } // Close all bubbles of all children. for (const icon of block.getIcons()) { - if (hasBubble(icon)) { - icon.setBubbleVisible(false); - } else if (icon.setVisible) { - // TODO (#7042): Remove old icon handling code. - icon.setVisible(false); - } + if (hasBubble(icon)) icon.setBubbleVisible(false); } } } diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index d6b5cf122..8e334cca4 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -22,7 +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'; +import * as deprecation from '../../utils/deprecation.js'; /** * An object that draws a block based on the given rendering information. @@ -59,7 +59,6 @@ export class Drawer { * required. */ draw() { - this.hideHiddenIcons_(); this.drawOutline_(); this.drawInternals_(); @@ -70,6 +69,16 @@ export class Drawer { this.recordSizeOnBlock_(); } + /** + * Hide icons that were marked as hidden. + * + * @deprecated Manually hiding icons is no longer necessary. To be removed + * in v11. + */ + protected hideHiddenIcons_() { + deprecation.warn('hideHiddenIcons_', 'v10', 'v11'); + } + /** * Save sizing information back to the block * Most of the rendering information can be thrown away at the end of the @@ -83,13 +92,6 @@ export class Drawer { this.block_.width = this.info_.widthWithChildren; } - /** Hide icons that were marked as hidden. */ - protected hideHiddenIcons_() { - for (let i = 0, iconInfo; (iconInfo = this.info_.hiddenIcons[i]); i++) { - iconInfo.icon.iconGroup_?.setAttribute('display', 'none'); - } - } - /** Create the outline of the block. This is a single continuous path. */ protected drawOutline_() { this.drawTop_(); @@ -291,10 +293,6 @@ export class Drawer { * @param fieldInfo The rendering information for the field or icon. */ protected layoutField_(fieldInfo: Icon | Field) { - const svgGroup = Types.isField(fieldInfo) - ? (fieldInfo as Field).field.getSvgRoot()! - : (fieldInfo as Icon).icon.iconGroup_!; // Never null in rendered case. - const yPos = fieldInfo.centerline - fieldInfo.height / 2; let xPos = fieldInfo.xPos; let scale = ''; @@ -305,36 +303,20 @@ export class Drawer { scale = 'scale(-1 1)'; } } + if (Types.isIcon(fieldInfo)) { 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(); + icon.setOffsetInBlock(new Coordinate(xPos, yPos)); + if (this.info_.isInsertionMarker) { + icon.hideForInsertionMarker(); } } else { + const svgGroup = (fieldInfo as Field).field.getSvgRoot()!; svgGroup.setAttribute( 'transform', 'translate(' + xPos + ',' + yPos + ')' + scale ); - } - - 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. - // TODO (#7042): Figure out a better way to handle the types here, - // possibly by splitting this method into submethods. - if (Types.isIcon(fieldInfo) && isIcon((fieldInfo as Icon).icon)) { - ( - (fieldInfo as Icon).icon as AnyDuringMigration - ).hideForInsertionMarker(); - } else { + if (this.info_.isInsertionMarker) { svgGroup.setAttribute('display', 'none'); } } diff --git a/core/renderers/common/info.ts b/core/renderers/common/info.ts index 0f53f3f46..7f53e178d 100644 --- a/core/renderers/common/info.ts +++ b/core/renderers/common/info.ts @@ -75,8 +75,6 @@ export class RenderInfo { /** An array of input rows on the block. */ inputRows: InputRow[] = []; - /** An array of measurable objects containing hidden icons. */ - hiddenIcons: Icon[] = []; topRow: TopRow; bottomRow: BottomRow; @@ -174,9 +172,7 @@ export class RenderInfo { const icons = this.block_.getIcons(); for (let i = 0, icon; (icon = icons[i]); i++) { const iconInfo = new Icon(this.constants_, icon); - if (this.isCollapsed && icon.collapseHidden) { - this.hiddenIcons.push(iconInfo); - } else { + if (!this.isCollapsed || icon.isShownWhenCollapsed()) { activeRow.elements.push(iconInfo); } } diff --git a/core/renderers/geras/drawer.ts b/core/renderers/geras/drawer.ts index c2a70af1a..d96933de7 100644 --- a/core/renderers/geras/drawer.ts +++ b/core/renderers/geras/drawer.ts @@ -39,7 +39,6 @@ export class Drawer extends BaseDrawer { } override draw() { - this.hideHiddenIcons_(); this.drawOutline_(); this.drawInternals_(); diff --git a/core/renderers/measurables/icon.ts b/core/renderers/measurables/icon.ts index f1592f8fe..6efb16b51 100644 --- a/core/renderers/measurables/icon.ts +++ b/core/renderers/measurables/icon.ts @@ -8,13 +8,12 @@ import * as goog from '../../../closure/goog/goog.js'; goog.declareModuleId('Blockly.blockRendering.Icon'); /* eslint-disable-next-line no-unused-vars */ -import type {Icon as BlocklyIcon} from '../../icon_old.js'; +import type {IIcon as BlocklyIcon} from '../../interfaces/i_icon.js'; 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 @@ -38,15 +37,10 @@ export class Icon extends Measurable { constructor(constants: ConstantProvider, public icon: BlocklyIcon) { super(constants); - // TODO(#7042): Remove references to old icon API. - this.isVisible = - (icon.isVisible && icon.isVisible()) || - (hasBubble(icon) && icon.bubbleIsVisible()); + this.isVisible = hasBubble(icon) && icon.bubbleIsVisible(); this.type |= Types.ICON; - const size = - (icon.getCorrectedSize && icon.getCorrectedSize()) || - (isIcon(icon) && icon.getSize()); + const size = icon.getSize(); this.height = size.height; this.width = size.width; } diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index 538347fab..a2d615e06 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -40,7 +40,6 @@ export class Drawer extends BaseDrawer { override draw() { const pathObject = this.block_.pathObject as PathObject; pathObject.beginDrawing(); - this.hideHiddenIcons_(); this.drawOutline_(); this.drawInternals_(); diff --git a/core/serialization/blocks.ts b/core/serialization/blocks.ts index d0e823e82..23de10dba 100644 --- a/core/serialization/blocks.ts +++ b/core/serialization/blocks.ts @@ -12,7 +12,6 @@ import type {BlockSvg} from '../block_svg.js'; import type {Connection} from '../connection.js'; import * as eventUtils from '../events/utils.js'; import {inputTypes} from '../inputs/input_types.js'; -import {isIcon} from '../interfaces/i_icon.js'; import {isSerializable} from '../interfaces/i_serializable.js'; import type {ISerializer} from '../interfaces/i_serializer.js'; import * as registry from '../registry.js'; @@ -706,11 +705,7 @@ function initBlock(block: Block, rendered: boolean) { // fixes #6076 JSO deserialization doesn't // set .iconXY_ property so here it will be set for (const icon of blockSvg.getIcons()) { - if (isIcon(icon)) { - icon.onLocationChange(blockSvg.getRelativeToSurfaceXY()); - } else { - icon.computeIconLocation(); - } + icon.onLocationChange(blockSvg.getRelativeToSurfaceXY()); } } else { block.initModel();