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

@@ -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)) {

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);
     }
   }
 

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('');

core/icon_old.ts

@@ -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:
-        <g class="blocklyIconGroup">
-          ...
-        </g>
-        */
-    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

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 {

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(

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);
         }
       }
     }

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');
       }
     }

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);
       }
     }

core/renderers/geras/drawer.ts

@@ -39,7 +39,6 @@ export class Drawer extends BaseDrawer {
   }
 
   override draw() {
-    this.hideHiddenIcons_();
     this.drawOutline_();
     this.drawInternals_();
 

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;
   }

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_();
 

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();