fix: updating icon locations (#7110)

* fix: updating icon locations

* fix: updating icon locations after appending

* fix: updating icon locations in deserialization

core/block_dragger.ts

@@ -22,6 +22,7 @@ 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 {InsertionMarkerManager} from './insertion_marker_manager.js';
 import type {IBlockDragger} from './interfaces/i_block_dragger.js';
 import type {IDragTarget} from './interfaces/i_drag_target.js';
@@ -29,6 +30,7 @@ import * as registry from './registry.js';
 import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
+import {hasBubble} from './interfaces/i_has_bubble.js';
 
 /**
  * Class for a block dragger.  It moves blocks around the workspace when they
@@ -75,7 +77,7 @@ export class BlockDragger implements IBlockDragger {
      * on this block and its descendants.  Moving an icon moves the bubble that
      * extends from it if that bubble is open.
      */
-    this.dragIconData_ = initIconData(block);
+    this.dragIconData_ = initIconData(block, this.startXY_);
   }
 
   /**
@@ -406,9 +408,13 @@ export class BlockDragger implements IBlockDragger {
    */
   protected dragIcons_(dxy: Coordinate) {
     // Moving icons moves their associated bubbles.
-    for (let i = 0; i < this.dragIconData_.length; i++) {
-      const data = this.dragIconData_[i];
-      data.icon.setIconLocation(Coordinate.sum(data.location, dxy));
+    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));
+      }
     }
   }
 
@@ -442,27 +448,41 @@ export interface IconPositionData {
  * extends from it if that bubble is open.
  *
  * @param block The root block that is being dragged.
+ * @param blockOrigin The top left of the given block in workspace coordinates.
  * @returns The list of all icons and their locations.
  */
-function initIconData(block: BlockSvg): IconPositionData[] {
+function initIconData(
+  block: BlockSvg,
+  blockOrigin: Coordinate
+): IconPositionData[] {
   // Build a list of icons that need to be moved and where they started.
   const dragIconData = [];
-  const descendants = block.getDescendants(false);
 
-  for (let i = 0, descendant; (descendant = descendants[i]); i++) {
-    const icons = descendant.getIcons();
-    for (let j = 0; j < icons.length; j++) {
-      // Only bother to track icons whose bubble is visible.
-      if (!icons[j].isVisible()) continue;
-      const data = {
+  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: icons[j].getIconLocation(), // Blockly.Icon
-        icon: icons[j],
-      };
-      dragIconData.push(data);
+        location: icon.getIconLocation(), // Blockly.Icon
+        icon: icon,
+      });
     }
   }
+
+  for (const child of block.getChildren(false)) {
+    dragIconData.push(
+      ...initIconData(child, Coordinate.sum(blockOrigin, child.relativeCoords))
+    );
+  }
   // AnyDuringMigration because:  Type '{ location: Coordinate | null; icon:
   // Icon; }[]' is not assignable to type 'IconPositionData[]'.
   return dragIconData as AnyDuringMigration;

core/block_svg.ts

@@ -671,8 +671,14 @@ export class BlockSvg
       myConnections[i].moveBy(dx, dy);
     }
     const icons = this.getIcons();
-    for (let i = 0; i < icons.length; i++) {
-      icons[i].computeIconLocation();
+    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();
+      }
     }
 
     // Recurse through all blocks attached under this one.
@@ -1649,7 +1655,7 @@ export class BlockSvg
         this.updateCollapsed_();
       }
       this.workspace.getRenderer().render(this);
-      this.updateConnectionLocations();
+      this.updateConnectionAndIconLocations();
 
       if (opt_bubble !== false) {
         const parentBlock = this.getParent();
@@ -1728,7 +1734,7 @@ export class BlockSvg
    *
    * @internal
    */
-  updateConnectionLocations() {
+  private updateConnectionAndIconLocations() {
     const blockTL = this.getRelativeToSurfaceXY();
     // Don't tighten previous or output connections because they are inferior
     // connections.
@@ -1755,6 +1761,15 @@ export class BlockSvg
         this.nextConnection.tighten();
       }
     }
+
+    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.
+    }
   }
 
   /**

core/renderers/common/drawer.ts

@@ -330,7 +330,7 @@ export class Drawer {
       // 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 (isIcon((fieldInfo as Icon).icon)) {
+      if (Types.isIcon(fieldInfo) && isIcon((fieldInfo as Icon).icon)) {
         (
           (fieldInfo as Icon).icon as AnyDuringMigration
         ).hideForInsertionMarker();

core/serialization/blocks.ts

@@ -12,6 +12,7 @@ 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';
@@ -728,9 +729,12 @@ function initBlock(block: Block, rendered: boolean) {
     blockSvg.render(false);
     // fixes #6076 JSO deserialization doesn't
     // set .iconXY_ property so here it will be set
-    const icons = blockSvg.getIcons();
-    for (let i = 0; i < icons.length; i++) {
-      icons[i].computeIconLocation();
+    for (const icon of blockSvg.getIcons()) {
+      if (isIcon(icon)) {
+        icon.onLocationChange(blockSvg.getRelativeToSurfaceXY());
+      } else {
+        icon.computeIconLocation();
+      }
     }
   } else {
     block.initModel();