From f4e378d09646abe6a1eb13a873ca1cd62353e216 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Fri, 2 Jun 2023 09:34:34 -0700 Subject: [PATCH] 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 | 2 +- core/block.ts | 1 + core/block_svg.ts | 57 ++++----- core/blockly.ts | 2 - core/bubbles/bubble.ts | 2 +- core/icons/icon.ts | 4 +- core/icons/warning_icon.ts | 205 ++++++++++++++++++++++++++++++ core/warning.ts | 168 ------------------------ scripts/migration/renamings.json5 | 10 ++ 9 files changed, 248 insertions(+), 203 deletions(-) create mode 100644 core/icons/warning_icon.ts delete mode 100644 core/warning.ts diff --git a/blocks/loops.ts b/blocks/loops.ts index 104a5ef6e..fa9d77656 100644 --- a/blocks/loops.ts +++ b/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'; diff --git a/core/block.ts b/core/block.ts index 314ba1383..b14080c49 100644 --- a/core/block.ts +++ b/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; } diff --git a/core/block_svg.ts b/core/block_svg.ts index e8172c51a..40fe25eb6 100644 --- a/core/block_svg.ts +++ b/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(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; } diff --git a/core/blockly.ts b/core/blockly.ts index 2caa2a272..b09d9dfaa 100644 --- a/core/blockly.ts +++ b/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}; diff --git a/core/bubbles/bubble.ts b/core/bubbles/bubble.ts index 0ba299ad0..490bc6a1d 100644 --- a/core/bubbles/bubble.ts +++ b/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); diff --git a/core/icons/icon.ts b/core/icons/icon.ts index b1506b24b..b35ba286e 100644 --- a/core/icons/icon.ts +++ b/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; diff --git a/core/icons/warning_icon.ts b/core/icons/warning_icon.ts new file mode 100644 index 000000000..7d52a5fda --- /dev/null +++ b/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 = 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); + } +} diff --git a/core/warning.ts b/core/warning.ts deleted file mode 100644 index 439e3f81e..000000000 --- a/core/warning.ts +++ /dev/null @@ -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(); - } -} diff --git a/scripts/migration/renamings.json5 b/scripts/migration/renamings.json5 index e46308482..2ea7f3e9f 100644 --- a/scripts/migration/renamings.json5 +++ b/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', + }, + }, + }, ], }