fix!: refactor warning icon (#7112)

* feat: add basic warning icon

* feat: work on actually using the warning icon

* chore: add docs

* chore: delete old warning icon

* chore: fix build

* chore: my own comments

* chore: move Warning to icons.WarningIcon

* chore: move properties to the module level

* chore: properly override and call super

* chore: properly use optional chaining

* chore: fixup comment typo

* chore: change imports to import type

* chore: reduces changes in block js files

* chore: add renaming

blocks/loops.ts

@@ -27,7 +27,7 @@ import '../core/field_dropdown.js';
 import '../core/field_label.js';
 import '../core/field_number.js';
 import '../core/field_variable.js';
-import '../core/warning.js';
+import '../core/icons/warning_icon.js';
 import {FieldVariable} from '../core/field_variable.js';
 import {WorkspaceSvg} from '../core/workspace_svg.js';
 

core/block.ts

@@ -2231,6 +2231,7 @@ export class Block implements IASTNodeLocation, IDeletable {
    */
   removeIcon(type: string): boolean {
     if (!this.hasIcon(type)) return false;
+    this.getIcon(type)?.dispose();
     this.icons = this.icons.filter((icon) => icon.getType() !== type);
     return true;
   }

core/block_svg.ts

@@ -56,7 +56,7 @@ import * as dom from './utils/dom.js';
 import {Rect} from './utils/rect.js';
 import {Svg} from './utils/svg.js';
 import * as svgMath from './utils/svg_math.js';
-import {Warning} from './warning.js';
+import {WarningIcon} from './icons/warning_icon.js';
 import type {Workspace} from './workspace.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 import {queueRender} from './render_management.js';
@@ -112,8 +112,12 @@ export class BlockSvg
   /** Block's comment icon (if any). */
   private commentIcon_: Comment | null = null;
 
-  /** Block's warning icon (if any). */
-  warning: Warning | null = null;
+  /**
+   * Block's warning icon (if any).
+   *
+   * @deprecated Use `setWarningText` to modify warnings on this block.
+   */
+  warning: WarningIcon | null = null;
 
   private svgGroup_: SVGGElement;
   style: BlockStyle;
@@ -983,7 +987,8 @@ export class BlockSvg
       text = null;
     }
 
-    let changedState = false;
+    // TODO: Make getIcon take in a type parameter?
+    const icon = this.getIcon(WarningIcon.TYPE) as WarningIcon | undefined;
     if (typeof text === 'string') {
       // Bubble up to add a warning on top-most collapsed block.
       let parent = this.getSurroundParent();
@@ -1001,33 +1006,19 @@ export class BlockSvg
         );
       }
 
-      if (!this.warning) {
-        this.warning = new Warning(this);
-        changedState = true;
+      if (icon) {
+        (icon as WarningIcon).addMessage(text, id);
+      } else {
+        this.addIcon(new WarningIcon(this).addMessage(text, id));
       }
-      this.warning!.setText(text, id);
-    } else {
+    } else if (icon) {
       // Dispose all warnings if no ID is given.
-      if (this.warning && !id) {
-        this.warning.dispose();
-        changedState = true;
-      } else if (this.warning) {
-        const oldText = this.warning.getText();
-        this.warning.setText('', id);
-        const newText = this.warning.getText();
-        if (!newText) {
-          this.warning.dispose();
-        }
-        changedState = oldText !== newText;
+      if (!id) {
+        this.removeIcon(WarningIcon.TYPE);
+      } else {
+        if (!icon.getText()) this.removeIcon(WarningIcon.TYPE);
       }
     }
-    if (changedState && this.rendered) {
-      // Icons must force an immediate render so that bubbles can be opened
-      // immedately at the correct position.
-      this.render();
-      // Adding or removing a warning icon will cause the block to change shape.
-      this.bumpNeighbours();
-    }
   }
 
   /**
@@ -1055,14 +1046,18 @@ export class BlockSvg
 
   override addIcon<T extends IIcon>(icon: T): T {
     super.addIcon(icon);
+
+    if (icon instanceof WarningIcon) this.warning = icon;
+
     if (this.rendered) {
       icon.initView(this.createIconPointerDownListener(icon));
       icon.applyColour();
       icon.updateEditable();
-      // TODO: Change this based on #7024.
+      // TODO: Change this based on #7068.
       this.render();
       this.bumpNeighbours();
     }
+
     return icon;
   }
 
@@ -1082,8 +1077,11 @@ export class BlockSvg
 
   override removeIcon(type: string): boolean {
     const removed = super.removeIcon(type);
+
+    if (type === WarningIcon.TYPE) this.warning = null;
+
     if (this.rendered) {
-      // TODO: Change this based on #7024.
+      // TODO: Change this based on #7068.
       this.render();
       this.bumpNeighbours();
     }
@@ -1095,7 +1093,6 @@ export class BlockSvg
   override getIcons(): AnyDuringMigration[] {
     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);
     return icons;
   }

core/blockly.ts

@@ -211,7 +211,6 @@ import {VariableMap} from './variable_map.js';
 import {VariableModel} from './variable_model.js';
 import * as Variables from './variables.js';
 import * as VariablesDynamic from './variables_dynamic.js';
-import {Warning} from './warning.js';
 import * as WidgetDiv from './widgetdiv.js';
 import {Workspace} from './workspace.js';
 import {WorkspaceAudio} from './workspace_audio.js';
@@ -644,7 +643,6 @@ export {Trashcan};
 export {VariableMap};
 export {VariableModel};
 export {VerticalFlyout};
-export {Warning};
 export {Workspace};
 export {WorkspaceAudio};
 export {WorkspaceComment};

core/bubbles/bubble.ts

@@ -184,7 +184,7 @@ export abstract class Bubble implements IBubble {
   }
 
   /** Sets the colour of the background and tail of this bubble. */
-  protected setColour(colour: string) {
+  public setColour(colour: string) {
     this.colour = colour;
     this.tail.setAttribute('fill', colour);
     this.background.setAttribute('fill', colour);

core/icons/icon.ts

@@ -47,7 +47,9 @@ export abstract class Icon implements IIcon {
     );
   }
 
-  dispose(): void {}
+  dispose(): void {
+    dom.removeNode(this.svgRoot);
+  }
 
   getWeight(): number {
     return -1;

core/icons/warning_icon.ts

@@ -0,0 +1,205 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as goog from '../../closure/goog/goog.js';
+goog.declareModuleId('Blockly.Warning');
+
+import type {BlockSvg} from '../block_svg.js';
+import {Coordinate} from '../utils/coordinate.js';
+import * as dom from '../utils/dom.js';
+import * as eventUtils from '../events/utils.js';
+import {Icon} from './icon.js';
+import type {IHasBubble} from '../interfaces/i_has_bubble.js';
+import {Rect} from '../utils/rect.js';
+import {Size} from '../utils.js';
+import {Svg} from '../utils/svg.js';
+import {TextBubble} from '../bubbles/text_bubble.js';
+
+/** The size of the warning icon in workspace-scale units. */
+const SIZE = 17;
+
+export class WarningIcon extends Icon implements IHasBubble {
+  /** The type string used to identify this icon. */
+  static readonly TYPE = 'warning';
+
+  /**
+   * The weight this icon has relative to other icons. Icons with more positive
+   * weight values are rendered farther toward the end of the block.
+   */
+  static readonly WEIGHT = 2;
+
+  /** A map of warning IDs to warning text. */
+  private textMap: Map<string, string> = new Map();
+
+  /** The bubble used to display the warnings to the user. */
+  private textBubble: TextBubble | null = null;
+
+  /** @internal */
+  constructor(protected readonly sourceBlock: BlockSvg) {
+    super(sourceBlock);
+  }
+
+  override getType(): string {
+    return WarningIcon.TYPE;
+  }
+
+  override initView(pointerdownListener: (e: PointerEvent) => void): void {
+    if (this.svgRoot) return; // Already initialized.
+
+    super.initView(pointerdownListener);
+
+    // Triangle with rounded corners.
+    dom.createSvgElement(
+      Svg.PATH,
+      {
+        'class': 'blocklyIconShape',
+        'd': 'M2,15Q-1,15 0.5,12L6.5,1.7Q8,-1 9.5,1.7L15.5,12Q17,15 14,15z',
+      },
+      this.svgRoot
+    );
+    // Can't use a real '!' text character since different browsers and
+    // operating systems render it differently. Body of exclamation point.
+    dom.createSvgElement(
+      Svg.PATH,
+      {
+        'class': 'blocklyIconSymbol',
+        'd': 'm7,4.8v3.16l0.27,2.27h1.46l0.27,-2.27v-3.16z',
+      },
+      this.svgRoot
+    );
+    // Dot of exclamation point.
+    dom.createSvgElement(
+      Svg.RECT,
+      {
+        'class': 'blocklyIconSymbol',
+        'x': '7',
+        'y': '11',
+        'height': '2',
+        'width': '2',
+      },
+      this.svgRoot
+    );
+  }
+
+  override dispose() {
+    super.dispose();
+    this.textBubble?.dispose();
+  }
+
+  override getWeight(): number {
+    return WarningIcon.WEIGHT;
+  }
+
+  override getSize(): Size {
+    return new Size(SIZE, SIZE);
+  }
+
+  override applyColour(): void {
+    super.applyColour();
+    this.textBubble?.setColour(this.sourceBlock.style.colourPrimary);
+  }
+
+  override updateCollapsed(): void {
+    // We are shown when collapsed, so do nothing! I.e. skip the default
+    // behavior of hiding.
+  }
+
+  /** Tells blockly that this icon is shown when the block is collapsed. */
+  override isShownWhenCollapsed(): boolean {
+    return true;
+  }
+
+  /** Updates the location of the icon's bubble if it is open. */
+  override onLocationChange(blockOrigin: Coordinate): void {
+    super.onLocationChange(blockOrigin);
+    this.textBubble?.setAnchorLocation(this.getAnchorLocation());
+  }
+
+  /**
+   * Adds a warning message to this warning icon.
+   *
+   * @param text The text of the message to add.
+   * @param id The id of the message to add.
+   * @internal
+   */
+  addMessage(text: string, id: string): this {
+    if (this.textMap.get(id) === text) return this;
+
+    if (text) {
+      this.textMap.set(id, text);
+    } else {
+      this.textMap.delete(id);
+    }
+
+    this.textBubble?.setText(this.getText());
+    return this;
+  }
+
+  /**
+   * @returns the display text for this icon. Includes all warning messages
+   *     concatenated together with newlines.
+   * @internal
+   */
+  getText(): string {
+    return [...this.textMap.values()].join('\n');
+  }
+
+  /** Toggles the visibility of the bubble. */
+  override onClick(): void {
+    super.onClick();
+    this.setBubbleVisible(!this.bubbleIsVisible());
+  }
+
+  bubbleIsVisible(): boolean {
+    return !!this.textBubble;
+  }
+
+  setBubbleVisible(visible: boolean): void {
+    if (this.bubbleIsVisible() === visible) return;
+
+    if (visible) {
+      this.textBubble = new TextBubble(
+        this.getText(),
+        this.sourceBlock.workspace,
+        this.getAnchorLocation(),
+        this.getBubbleOwnerRect()
+      );
+      this.applyColour();
+    } else {
+      this.textBubble?.dispose();
+      this.textBubble = null;
+    }
+
+    eventUtils.fire(
+      new (eventUtils.get(eventUtils.BUBBLE_OPEN))(
+        this.sourceBlock,
+        visible,
+        'warning'
+      )
+    );
+  }
+
+  /**
+   * @returns the location the bubble should be anchored to.
+   *     I.E. the middle of this icon.
+   */
+  private getAnchorLocation(): Coordinate {
+    const midIcon = SIZE / 2;
+    return Coordinate.sum(
+      this.workspaceLocation,
+      new Coordinate(midIcon, midIcon)
+    );
+  }
+
+  /**
+   * @returns the rect the bubble should avoid overlapping.
+   *     I.E. the block that owns this icon.
+   */
+  private getBubbleOwnerRect(): Rect {
+    const bbox = this.sourceBlock.getSvgRoot().getBBox();
+    return new Rect(bbox.y, bbox.y + bbox.height, bbox.x, bbox.x + bbox.width);
+  }
+}

core/warning.ts

@@ -1,168 +0,0 @@
-/**
- * @license
- * Copyright 2012 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-
-/**
- * Object representing a warning.
- *
- * @class
- */
-import * as goog from '../closure/goog/goog.js';
-goog.declareModuleId('Blockly.Warning');
-
-// Unused import preserved for side-effects. Remove if unneeded.
-import './events/events_bubble_open.js';
-
-import type {BlockSvg} from './block_svg.js';
-import {Bubble} from './bubble_old.js';
-import * as eventUtils from './events/utils.js';
-import {Icon} from './icon_old.js';
-import type {Coordinate} from './utils/coordinate.js';
-import * as dom from './utils/dom.js';
-import {Svg} from './utils/svg.js';
-
-/**
- * Class for a warning.
- */
-export class Warning extends Icon {
-  private text: {[key: string]: string};
-
-  /** The top-level node of the warning text, or null if not created. */
-  private paragraphElement: SVGTextElement | null = null;
-
-  /** Does this icon get hidden when the block is collapsed? */
-  override collapseHidden = false;
-
-  /** @param block The block associated with this warning. */
-  constructor(block: BlockSvg) {
-    super(block);
-    this.createIcon();
-    // The text object can contain multiple warnings.
-    this.text = Object.create(null);
-  }
-
-  /**
-   * Draw the warning icon.
-   *
-   * @param group The icon group.
-   */
-  protected override drawIcon_(group: Element) {
-    // Triangle with rounded corners.
-    dom.createSvgElement(
-      Svg.PATH,
-      {
-        'class': 'blocklyIconShape',
-        'd': 'M2,15Q-1,15 0.5,12L6.5,1.7Q8,-1 9.5,1.7L15.5,12Q17,15 14,15z',
-      },
-      group
-    );
-    // Can't use a real '!' text character since different browsers and
-    // operating systems render it differently. Body of exclamation point.
-    dom.createSvgElement(
-      Svg.PATH,
-      {
-        'class': 'blocklyIconSymbol',
-        'd': 'm7,4.8v3.16l0.27,2.27h1.46l0.27,-2.27v-3.16z',
-      },
-      group
-    );
-    // Dot of exclamation point.
-    dom.createSvgElement(
-      Svg.RECT,
-      {
-        'class': 'blocklyIconSymbol',
-        'x': '7',
-        'y': '11',
-        'height': '2',
-        'width': '2',
-      },
-      group
-    );
-  }
-
-  /**
-   * Show or hide the warning bubble.
-   *
-   * @param visible True if the bubble should be visible.
-   */
-  override setVisible(visible: boolean) {
-    if (visible === this.isVisible()) {
-      return;
-    }
-    eventUtils.fire(
-      new (eventUtils.get(eventUtils.BUBBLE_OPEN))(
-        this.getBlock(),
-        visible,
-        'warning'
-      )
-    );
-    if (visible) {
-      this.createBubble();
-    } else {
-      this.disposeBubble();
-    }
-  }
-
-  /** Show the bubble. */
-  private createBubble() {
-    this.paragraphElement = Bubble.textToDom(this.getText());
-    this.bubble_ = Bubble.createNonEditableBubble(
-      this.paragraphElement,
-      this.getBlock(),
-      this.iconXY_ as Coordinate
-    );
-    this.applyColour();
-  }
-
-  /** Dispose of the bubble and references to it. */
-  private disposeBubble() {
-    if (this.bubble_) {
-      this.bubble_.dispose();
-      this.bubble_ = null;
-    }
-    this.paragraphElement = null;
-  }
-
-  /**
-   * Set this warning's text.
-   *
-   * @param text Warning text (or '' to delete). This supports linebreaks.
-   * @param id An ID for this text entry to be able to maintain multiple
-   *     warnings.
-   */
-  setText(text: string, id: string) {
-    if (this.text[id] === text) {
-      return;
-    }
-    if (text) {
-      this.text[id] = text;
-    } else {
-      delete this.text[id];
-    }
-    if (this.isVisible()) {
-      this.setVisible(false);
-      this.setVisible(true);
-    }
-  }
-
-  /**
-   * Get this warning's texts.
-   *
-   * @returns All texts concatenated into one string.
-   */
-  getText(): string {
-    const allWarnings = [];
-    for (const id in this.text) {
-      allWarnings.push(this.text[id]);
-    }
-    return allWarnings.join('\n');
-  }
-
-  /** Dispose of this warning. */
-  override dispose() {
-    this.getBlock().warning = null;
-    super.dispose();
-  }
-}

scripts/migration/renamings.json5

@@ -1447,5 +1447,15 @@
 
   'develop': [
     // New renamings go here!
+    {
+      oldName: 'Blockly.Warning',
+      exports: {
+        Warning: {
+          newExport: 'WarningIcon',
+          oldPath: 'Blockly.Warning',
+          newPath: 'Blocky.icons.WarningIcon',
+        },
+      },
+    },
   ],
 }