From ed403d0b77eaeba8939c8fd8ffeef48721257ffc Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Thu, 4 Apr 2024 15:52:43 +0000 Subject: [PATCH] feat!: change gestures to look at selected when dragging (#7991) * feat: change gestures to look at selected when dragging * chore: fix tests * chore: format * chore: PR comments --- blocks/procedures.ts | 3 +- core/block_svg.ts | 42 +---- core/bubbles/bubble.ts | 19 ++- core/bubbles/mini_workspace_bubble.ts | 2 +- core/bubbles/text_bubble.ts | 2 +- core/bubbles/textinput_bubble.ts | 2 +- core/clipboard/block_paster.ts | 3 +- core/comments/rendered_workspace_comment.ts | 9 +- core/common.ts | 12 ++ core/contextmenu.ts | 5 +- core/dragging/block_drag_strategy.ts | 5 +- core/gesture.ts | 173 ++++---------------- core/interfaces/i_selectable.ts | 5 + tests/mocha/comment_deserialization_test.js | 6 +- 14 files changed, 88 insertions(+), 200 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 1457971d8..546f93606 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -38,6 +38,7 @@ import {config} from '../core/config.js'; import {defineBlocks} from '../core/common.js'; import '../core/icons/comment_icon.js'; import '../core/icons/warning_icon.js'; +import * as common from '../core/common.js'; /** A dictionary of the block definitions provided by this module. */ export const blocks: {[key: string]: BlockDefinition} = {}; @@ -1157,7 +1158,7 @@ const PROCEDURE_CALL_COMMON = { const def = Procedures.getDefinition(name, workspace); if (def) { (workspace as WorkspaceSvg).centerOnBlock(def.id); - (def as BlockSvg).select(); + common.setSelected(def as BlockSvg); } }, }); diff --git a/core/block_svg.ts b/core/block_svg.ts index c7ab9289e..6441cc434 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -241,56 +241,18 @@ export class BlockSvg return this.style.colourTertiary; } - /** - * Selects this block. Highlights the block visually and fires a select event - * if the block is not already selected. - */ + /** Selects this block. Highlights the block visually. */ select() { if (this.isShadow() && this.getParent()) { // Shadow blocks should not be selected. this.getParent()!.select(); return; } - if (common.getSelected() === this) { - return; - } - let oldId = null; - if (common.getSelected()) { - oldId = common.getSelected()!.id; - // Unselect any previously selected block. - eventUtils.disable(); - try { - common.getSelected()!.unselect(); - } finally { - eventUtils.enable(); - } - } - const event = new (eventUtils.get(eventUtils.SELECTED))( - oldId, - this.id, - this.workspace.id, - ); - eventUtils.fire(event); - common.setSelected(this); this.addSelect(); } - /** - * Unselects this block. Unhighlights the block and fires a select (false) - * event if the block is currently selected. - */ + /** Unselects this block. Unhighlights the blockv visually. */ unselect() { - if (common.getSelected() !== this) { - return; - } - const event = new (eventUtils.get(eventUtils.SELECTED))( - this.id, - null, - this.workspace.id, - ); - event.workspaceId = this.workspace.id; - eventUtils.fire(event); - common.setSelected(null); this.removeSelect(); } diff --git a/core/bubbles/bubble.ts b/core/bubbles/bubble.ts index 28f12c4fa..35b9e7dde 100644 --- a/core/bubbles/bubble.ts +++ b/core/bubbles/bubble.ts @@ -16,13 +16,16 @@ import {Rect} from '../utils/rect.js'; import {Size} from '../utils/size.js'; import {Svg} from '../utils/svg.js'; import {WorkspaceSvg} from '../workspace_svg.js'; +import * as common from '../common.js'; +import {ISelectable} from '../blockly.js'; +import * as idGenerator from '../utils/idgenerator.js'; /** * The abstract pop-up bubble class. This creates a UI that looks like a speech * bubble, where it has a "tail" that points to the block, and a "head" that * displays arbitrary svg elements. */ -export abstract class Bubble implements IBubble { +export abstract class Bubble implements IBubble, ISelectable { /** The width of the border around the bubble. */ static readonly BORDER_WIDTH = 6; @@ -50,6 +53,8 @@ export abstract class Bubble implements IBubble { /** Distance between arrow point and anchor point. */ static readonly ANCHOR_RADIUS = 8; + public id: string; + /** The SVG group containing all parts of the bubble. */ protected svgRoot: SVGGElement; @@ -89,10 +94,11 @@ export abstract class Bubble implements IBubble { * when automatically positioning. */ constructor( - protected readonly workspace: WorkspaceSvg, + public readonly workspace: WorkspaceSvg, protected anchor: Coordinate, protected ownerRect?: Rect, ) { + this.id = idGenerator.getNextUniqueId(); this.svgRoot = dom.createSvgElement( Svg.G, {'class': 'blocklyBubble'}, @@ -209,6 +215,7 @@ export abstract class Bubble implements IBubble { /** Passes the pointer event off to the gesture system. */ private onMouseDown(e: PointerEvent) { this.workspace.getGesture(e)?.handleBubbleStart(e, this); + common.setSelected(this); } /** Positions the bubble relative to its anchor. Does not render its tail. */ @@ -640,4 +647,12 @@ export abstract class Bubble implements IBubble { revertDrag(): void { this.dragStrategy.revertDrag(); } + + select(): void { + // Bubbles don't have any visual for being selected. + } + + unselect(): void { + // Bubbles don't have any visual for being selected. + } } diff --git a/core/bubbles/mini_workspace_bubble.ts b/core/bubbles/mini_workspace_bubble.ts index f459654fa..74317d57b 100644 --- a/core/bubbles/mini_workspace_bubble.ts +++ b/core/bubbles/mini_workspace_bubble.ts @@ -47,7 +47,7 @@ export class MiniWorkspaceBubble extends Bubble { /** @internal */ constructor( workspaceOptions: BlocklyOptions, - protected readonly workspace: WorkspaceSvg, + public readonly workspace: WorkspaceSvg, protected anchor: Coordinate, protected ownerRect?: Rect, ) { diff --git a/core/bubbles/text_bubble.ts b/core/bubbles/text_bubble.ts index 6f50d303b..020ab4f2e 100644 --- a/core/bubbles/text_bubble.ts +++ b/core/bubbles/text_bubble.ts @@ -20,7 +20,7 @@ export class TextBubble extends Bubble { constructor( private text: string, - protected readonly workspace: WorkspaceSvg, + public readonly workspace: WorkspaceSvg, protected anchor: Coordinate, protected ownerRect?: Rect, ) { diff --git a/core/bubbles/textinput_bubble.ts b/core/bubbles/textinput_bubble.ts index 7a8a44bdf..a3c572faf 100644 --- a/core/bubbles/textinput_bubble.ts +++ b/core/bubbles/textinput_bubble.ts @@ -70,7 +70,7 @@ export class TextInputBubble extends Bubble { * when automatically positioning. */ constructor( - protected readonly workspace: WorkspaceSvg, + public readonly workspace: WorkspaceSvg, protected anchor: Coordinate, protected ownerRect?: Rect, ) { diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index 0bb707c36..fefc9947d 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -13,6 +13,7 @@ import {Coordinate} from '../utils/coordinate.js'; import {WorkspaceSvg} from '../workspace_svg.js'; import * as eventUtils from '../events/utils.js'; import {config} from '../config.js'; +import * as common from '../common.js'; export class BlockPaster implements IPaster { static TYPE = 'block'; @@ -43,7 +44,7 @@ export class BlockPaster implements IPaster { if (eventUtils.isEnabled() && !block.isShadow()) { eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(block)); } - block.select(); + common.setSelected(block); return block; } } diff --git a/core/comments/rendered_workspace_comment.ts b/core/comments/rendered_workspace_comment.ts index c386cef0c..f6d86f203 100644 --- a/core/comments/rendered_workspace_comment.ts +++ b/core/comments/rendered_workspace_comment.ts @@ -16,10 +16,12 @@ import * as dom from '../utils/dom.js'; import {IDraggable} from '../interfaces/i_draggable.js'; import {CommentDragStrategy} from '../dragging/comment_drag_strategy.js'; import * as browserEvents from '../browser_events.js'; +import * as common from '../common.js'; +import {ISelectable} from '../interfaces/i_selectable.js'; export class RenderedWorkspaceComment extends WorkspaceComment - implements IBoundedElement, IRenderedElement, IDraggable + implements IBoundedElement, IRenderedElement, IDraggable, ISelectable { /** The class encompassing the svg elements making up the workspace comment. */ private view: CommentView; @@ -159,6 +161,7 @@ export class RenderedWorkspaceComment const gesture = this.workspace.getGesture(e); if (gesture) { gesture.handleCommentStart(e, this); + common.setSelected(this); } } @@ -186,4 +189,8 @@ export class RenderedWorkspaceComment revertDrag(): void { this.dragStrategy.revertDrag(); } + + select(): void {} + + unselect(): void {} } diff --git a/core/common.ts b/core/common.ts index 625921350..fba960a5b 100644 --- a/core/common.ts +++ b/core/common.ts @@ -13,6 +13,7 @@ import {BlockDefinition, Blocks} from './blocks.js'; import type {Connection} from './connection.js'; import type {Workspace} from './workspace.js'; import type {WorkspaceSvg} from './workspace_svg.js'; +import * as eventUtils from './events/utils.js'; /** Database of all workspaces. */ const WorkspaceDB_ = Object.create(null); @@ -105,7 +106,18 @@ export function getSelected(): ISelectable | null { * @internal */ export function setSelected(newSelection: ISelectable | null) { + if (selected === newSelection) return; + + const event = new (eventUtils.get(eventUtils.SELECTED))( + selected?.id ?? null, + newSelection?.id ?? null, + newSelection?.workspace.id ?? selected?.workspace.id ?? '', + ); + eventUtils.fire(event); + + selected?.unselect(); selected = newSelection; + selected?.select(); } /** diff --git a/core/contextmenu.ts b/core/contextmenu.ts index 78431c93f..d1d5f5e4a 100644 --- a/core/contextmenu.ts +++ b/core/contextmenu.ts @@ -29,6 +29,7 @@ import * as WidgetDiv from './widgetdiv.js'; import {WorkspaceCommentSvg} from './workspace_comment_svg.js'; import type {WorkspaceSvg} from './workspace_svg.js'; import * as Xml from './xml.js'; +import * as common from './common.js'; /** * Which block is the context menu attached to? @@ -261,7 +262,7 @@ export function callbackFactory( if (eventUtils.isEnabled() && !newBlock.isShadow()) { eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(newBlock)); } - newBlock.select(); + common.setSelected(newBlock); return newBlock; }; } @@ -374,7 +375,7 @@ export function workspaceCommentOption( if (ws.rendered) { comment.initSvg(); comment.render(); - comment.select(); + common.setSelected(comment); } } diff --git a/core/dragging/block_drag_strategy.ts b/core/dragging/block_drag_strategy.ts index 559a6080a..84afe2a13 100644 --- a/core/dragging/block_drag_strategy.ts +++ b/core/dragging/block_drag_strategy.ts @@ -65,7 +65,10 @@ export class BlockDragStrategy implements IDragStrategy { this.block.isOwnMovable() && !this.block.isShadow() && !this.block.isDeadOrDying() && - !this.workspace.options.readOnly + !this.workspace.options.readOnly && + // We never drag blocks in the flyout, only create new blocks that are + // dragged. + !this.block.isInFlyout ); } diff --git a/core/gesture.ts b/core/gesture.ts index a6ca15528..52281e6b9 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -30,13 +30,12 @@ import * as internalConstants from './internal_constants.js'; import * as Tooltip from './tooltip.js'; import * as Touch from './touch.js'; import {Coordinate} from './utils/coordinate.js'; -import {WorkspaceCommentSvg} from './workspace_comment_svg.js'; import {WorkspaceDragger} from './workspace_dragger.js'; import type {WorkspaceSvg} from './workspace_svg.js'; import type {IIcon} from './interfaces/i_icon.js'; import {IDragger} from './interfaces/i_dragger.js'; import * as registry from './registry.js'; -import {IDraggable} from './interfaces/i_draggable.js'; +import {IDraggable, isDraggable} from './interfaces/i_draggable.js'; import {RenderedWorkspaceComment} from './comments.js'; /** @@ -298,71 +297,7 @@ export class Gesture { // The start block is no longer relevant, because this is a drag. this.startBlock = null; this.targetBlock = this.flyout.createBlock(this.targetBlock); - this.targetBlock.select(); - return true; - } - return false; - } - - /** - * Update this gesture to record whether a bubble is being dragged. - * This function should be called on a pointermove event the first time - * the drag radius is exceeded. It should be called no more than once per - * gesture. If a bubble should be dragged this function creates the necessary - * BubbleDragger and starts the drag. - * - * @returns True if a bubble is being dragged. - */ - private updateIsDraggingBubble(e: PointerEvent): boolean { - if (!this.startBubble) { - return false; - } - - this.startDraggingBubble(e); - return true; - } - - /** - * Update this gesture to record whether a comment is being dragged. - * This function should be called on a pointermove event the first time - * the drag radius is exceeded. It should be called no more than once per - * gesture. - * - * @returns True if a comment is being dragged. - */ - private updateIsDraggingComment(e: PointerEvent): boolean { - if (!this.startComment) { - return false; - } - - this.startDraggingComment(e); - return true; - } - - /** - * Check whether to start a block drag. If a block should be dragged, either - * from the flyout or in the workspace, create the necessary BlockDragger and - * start the drag. - * - * This function should be called on a pointermove event the first time - * the drag radius is exceeded. It should be called no more than once per - * gesture. If a block should be dragged, either from the flyout or in the - * workspace, this function creates the necessary BlockDragger and starts the - * drag. - * - * @returns True if a block is being dragged. - */ - private updateIsDraggingBlock(e: PointerEvent): boolean { - if (!this.targetBlock) { - return false; - } - if (this.flyout) { - if (this.updateIsDraggingFromFlyout()) { - this.startDraggingBlock(e); - return true; - } - } else if (this.targetBlock.isMovable()) { - this.startDraggingBlock(e); + common.setSelected(this.targetBlock); return true; } return false; @@ -403,76 +338,30 @@ export class Gesture { * gesture. */ private updateIsDragging(e: PointerEvent) { - // Sanity check. + if (!this.startWorkspace_) { + throw new Error( + 'Cannot update dragging because the start workspace is undefined', + ); + } + if (this.calledUpdateIsDragging) { throw Error('updateIsDragging_ should only be called once per gesture.'); } this.calledUpdateIsDragging = true; - // First check if it was a bubble drag. Bubbles always sit on top of - // blocks. - if (this.updateIsDraggingBubble(e)) { - return; - } - // Then check if it was a block drag. - if (this.updateIsDraggingBlock(e)) { - return; - } - if (this.updateIsDraggingComment(e)) { - return; - } - // Then check if it's a workspace drag. - this.updateIsDraggingWorkspace(); - } + // If we drag a block out of the flyout, it updates `common.getSelected` + // to return the new block. + if (this.flyout) this.updateIsDraggingFromFlyout(); - /** Start dragging the selected block. */ - private startDraggingBlock(e: PointerEvent) { - this.dragging = true; - this.dragger = this.createDragger(this.targetBlock!, this.startWorkspace_!); - this.dragger.onDragStart(e); - this.dragger.onDrag(e, this.currentDragDeltaXY); - } - - /** Start dragging the selected bubble. */ - private startDraggingBubble(e: PointerEvent) { - if (!this.startBubble) { - throw new Error( - 'Cannot update dragging the bubble because the start ' + - 'bubble is undefined', - ); + const selected = common.getSelected(); + if (selected && isDraggable(selected) && selected.isMovable()) { + this.dragging = true; + this.dragger = this.createDragger(selected, this.startWorkspace_); + this.dragger.onDragStart(e); + this.dragger.onDrag(e, this.currentDragDeltaXY); + } else { + this.updateIsDraggingWorkspace(); } - if (!this.startWorkspace_) { - throw new Error( - 'Cannot update dragging the bubble because the start ' + - 'workspace is undefined', - ); - } - - this.dragging = true; - this.dragger = this.createDragger(this.startBubble, this.startWorkspace_); - this.dragger.onDragStart(e); - this.dragger.onDrag(e, this.currentDragDeltaXY); - } - - /** Start dragging the selected comment. */ - private startDraggingComment(e: PointerEvent) { - if (!this.startComment) { - throw new Error( - 'Cannot update dragging the comment because the start ' + - 'comment is undefined', - ); - } - if (!this.startWorkspace_) { - throw new Error( - 'Cannot update dragging the comment because the start ' + - 'workspace is undefined', - ); - } - - this.dragging = true; - this.dragger = this.createDragger(this.startComment, this.startWorkspace_); - this.dragger.onDragStart(e); - this.dragger.onDrag(e, this.currentDragDeltaXY); } private createDragger( @@ -532,10 +421,6 @@ export class Gesture { Tooltip.block(); - if (this.targetBlock) { - this.targetBlock.select(); - } - if (browserEvents.isRightButton(e)) { this.handleRightClick(e); return; @@ -668,8 +553,7 @@ export class Gesture { } else if (this.workspaceDragger) { this.workspaceDragger.endDrag(this.currentDragDeltaXY); } else if (this.isBubbleClick()) { - // Bubbles are in front of all fields and blocks. - this.doBubbleClick(); + // Do nothing, bubbles don't currently respond to clicks. } else if (this.isFieldClick()) { this.doFieldClick(); } else if (this.isIconClick()) { @@ -873,6 +757,13 @@ export class Gesture { } this.setStartWorkspace(ws); this.mostRecentEvent = e; + + if (!this.startBlock && !this.startBubble && !this.startComment) { + // Selection determines what things start drags. So to drag the workspace, + // we need to deselect anything that was previously selected. + common.setSelected(null); + } + this.doStart(e); } @@ -963,15 +854,6 @@ export class Gesture { * type of target. Any developer wanting to add behaviour on clicks should * modify only this code. */ - /** Execute a bubble click. */ - private doBubbleClick() { - // TODO (#1673): Consistent handling of single clicks. - if (this.startBubble instanceof WorkspaceCommentSvg) { - this.startBubble.setFocus(); - this.startBubble.select(); - } - } - /** Execute a field click. */ private doFieldClick() { if (!this.startField) { @@ -1138,6 +1020,7 @@ export class Gesture { // If the gesture already went through a bubble, don't set the start block. if (!this.startBlock && !this.startBubble) { this.startBlock = block; + common.setSelected(this.startBlock); if (block.isInFlyout && block !== block.getRootBlock()) { this.setTargetBlock(block.getRootBlock()); } else { diff --git a/core/interfaces/i_selectable.ts b/core/interfaces/i_selectable.ts index 8090ad94a..7cf9ad98c 100644 --- a/core/interfaces/i_selectable.ts +++ b/core/interfaces/i_selectable.ts @@ -6,12 +6,16 @@ // Former goog.module ID: Blockly.ISelectable +import type {Workspace} from '../workspace.js'; + /** * The interface for an object that is selectable. */ export interface ISelectable { id: string; + workspace: Workspace; + /** Select this. Highlight it visually. */ select(): void; @@ -23,6 +27,7 @@ export interface ISelectable { export function isSelectable(obj: Object): obj is ISelectable { return ( typeof (obj as any).id === 'string' && + (obj as any).workspace !== undefined && (obj as any).select !== undefined && (obj as any).unselect !== undefined ); diff --git a/tests/mocha/comment_deserialization_test.js b/tests/mocha/comment_deserialization_test.js index 494c584e7..843453278 100644 --- a/tests/mocha/comment_deserialization_test.js +++ b/tests/mocha/comment_deserialization_test.js @@ -74,9 +74,7 @@ suite('Comment Deserialization', function () { simulateClick(this.workspace.trashcan.svgGroup); // Place from trashcan. simulateClick( - this.workspace.trashcan.flyout.svgGroup_.querySelector( - '.blocklyDraggable', - ), + this.workspace.trashcan.flyout.svgGroup_.querySelector('.blocklyPath'), ); chai.assert.equal(this.workspace.getAllBlocks().length, 1); // Check comment. @@ -113,7 +111,7 @@ suite('Comment Deserialization', function () { const toolbox = this.workspace.getToolbox(); simulateClick(toolbox.HtmlDiv.querySelector('.blocklyTreeRow')); simulateClick( - toolbox.getFlyout().svgGroup_.querySelector('.blocklyDraggable'), + toolbox.getFlyout().svgGroup_.querySelector('.blocklyPath'), ); chai.assert.equal(this.workspace.getAllBlocks().length, 1); // Check comment.