From e75a4fb1d38e9048f062743c471749b030408a81 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 3 Apr 2024 19:58:04 +0000 Subject: [PATCH] fix: comment move and change events (#7947) * fix: comment move event * feat: add support for a drag reason * fix: comment change events * chore: add tests for move and change events --- core/comments/rendered_workspace_comment.ts | 9 +++-- core/comments/workspace_comment.ts | 22 +++++++++++- core/events/events_comment_change.ts | 9 +++-- core/events/events_comment_move.ts | 29 ++++++++++++--- tests/mocha/workspace_comment_test.js | 40 +++++++++++++++++++++ 5 files changed, 96 insertions(+), 13 deletions(-) diff --git a/core/comments/rendered_workspace_comment.ts b/core/comments/rendered_workspace_comment.ts index ee713d9ff..c386cef0c 100644 --- a/core/comments/rendered_workspace_comment.ts +++ b/core/comments/rendered_workspace_comment.ts @@ -109,16 +109,15 @@ export class RenderedWorkspaceComment } /** Move the comment by the given amounts in workspace coordinates. */ - moveBy(dx: number, dy: number, _reason?: string[] | undefined): void { - // TODO(#7909): Deal with reason when we add events. + moveBy(dx: number, dy: number, reason?: string[] | undefined): void { const loc = this.getRelativeToSurfaceXY(); const newLoc = new Coordinate(loc.x + dx, loc.y + dy); - this.moveTo(newLoc); + this.moveTo(newLoc, reason); } /** Moves the comment to the given location in workspace coordinates. */ - override moveTo(location: Coordinate): void { - super.moveTo(location); + override moveTo(location: Coordinate, reason?: string[] | undefined): void { + super.moveTo(location, reason); this.view.moveTo(location); } diff --git a/core/comments/workspace_comment.ts b/core/comments/workspace_comment.ts index 2090882b8..6226e0176 100644 --- a/core/comments/workspace_comment.ts +++ b/core/comments/workspace_comment.ts @@ -9,6 +9,7 @@ import {Size} from '../utils/size.js'; import {Coordinate} from '../utils/coordinate.js'; import * as idGenerator from '../utils/idgenerator.js'; import * as eventUtils from '../events/utils.js'; +import {CommentMove} from '../events/events_comment_move.js'; export class WorkspaceComment { /** The unique identifier for this comment. */ @@ -72,9 +73,20 @@ export class WorkspaceComment { } } + /** Fires a comment change event. */ + private fireChangeEvent(oldText: string, newText: string) { + if (eventUtils.isEnabled()) { + eventUtils.fire( + new (eventUtils.get(eventUtils.COMMENT_CHANGE))(this, oldText, newText), + ); + } + } + /** Sets the text of the comment. */ setText(text: string) { + const oldText = this.text; this.text = text; + this.fireChangeEvent(oldText, text); } /** Returns the text of the comment. */ @@ -166,8 +178,16 @@ export class WorkspaceComment { } /** Moves the comment to the given location in workspace coordinates. */ - moveTo(location: Coordinate) { + moveTo(location: Coordinate, reason?: string[] | undefined) { + const event = new (eventUtils.get(eventUtils.COMMENT_MOVE))( + this, + ) as CommentMove; + if (reason) event.setReason(reason); + this.location = location; + + event.recordNew(); + if (eventUtils.isEnabled()) eventUtils.fire(event); } /** Returns the position of the comment in workspace coordinates. */ diff --git a/core/events/events_comment_change.ts b/core/events/events_comment_change.ts index 45444c25d..eb39d929d 100644 --- a/core/events/events_comment_change.ts +++ b/core/events/events_comment_change.ts @@ -124,13 +124,16 @@ export class CommentChange extends CommentBase { 'the constructor, or call fromJson', ); } - const comment = workspace.getCommentById(this.commentId); + // TODO: Remove the cast when we fix the type of getCommentById. + const comment = workspace.getCommentById( + this.commentId, + ) as unknown as WorkspaceComment; if (!comment) { console.warn("Can't change non-existent comment: " + this.commentId); return; } const contents = forward ? this.newContents_ : this.oldContents_; - if (!contents) { + if (contents === undefined) { if (forward) { throw new Error( 'The new contents is undefined. Either pass a value to ' + @@ -142,7 +145,7 @@ export class CommentChange extends CommentBase { 'the constructor, or call fromJson', ); } - comment.setContent(contents); + comment.setText(contents); } } diff --git a/core/events/events_comment_move.ts b/core/events/events_comment_move.ts index ece914310..502ca032f 100644 --- a/core/events/events_comment_move.ts +++ b/core/events/events_comment_move.ts @@ -35,6 +35,17 @@ export class CommentMove extends CommentBase { /** The location of the comment after the move, in workspace coordinates. */ newCoordinate_?: Coordinate; + /** + * An explanation of what this move is for. Known values include: + * 'drag' -- A drag operation completed. + * 'snap' -- Comment got shifted to line up with the grid. + * 'inbounds' -- Block got pushed back into a non-scrolling workspace. + * 'create' -- Block created via deserialization. + * 'cleanup' -- Workspace aligned top-level blocks. + * Event merging may create multiple reasons: ['drag', 'inbounds', 'snap']. + */ + reason?: string[]; + /** * @param opt_comment The comment that is being moved. Undefined for a blank * event. @@ -70,6 +81,15 @@ export class CommentMove extends CommentBase { this.newCoordinate_ = this.comment_.getRelativeToSurfaceXY(); } + /** + * Sets the reason for a move event. + * + * @param reason Why is this move happening? 'drag', 'bump', 'snap', ... + */ + setReason(reason: string[]) { + this.reason = reason; + } + /** * Override the location before the move. Use this if you don't create the * event until the end of the move, but you know the original location. @@ -158,7 +178,10 @@ export class CommentMove extends CommentBase { 'the constructor, or call fromJson', ); } - const comment = workspace.getCommentById(this.commentId); + // TODO: Remove cast when we update getCommentById. + const comment = workspace.getCommentById( + this.commentId, + ) as unknown as WorkspaceComment; if (!comment) { console.warn("Can't move non-existent comment: " + this.commentId); return; @@ -172,9 +195,7 @@ export class CommentMove extends CommentBase { 'or call fromJson', ); } - // TODO: Check if the comment is being dragged, and give up if so. - const current = comment.getRelativeToSurfaceXY(); - comment.moveBy(target.x - current.x, target.y - current.y); + comment.moveTo(target); } } diff --git a/tests/mocha/workspace_comment_test.js b/tests/mocha/workspace_comment_test.js index 6cf8b3d4b..b5658b265 100644 --- a/tests/mocha/workspace_comment_test.js +++ b/tests/mocha/workspace_comment_test.js @@ -54,5 +54,45 @@ suite('Workspace comment', function () { this.workspace.id, ); }); + + test('move events are fired when a comment is moved', function () { + this.renderedComment = new Blockly.comments.RenderedWorkspaceComment( + this.workspace, + ); + const spy = createChangeListenerSpy(this.workspace); + + this.renderedComment.moveTo(new Blockly.utils.Coordinate(42, 42)); + + assertEventFired( + spy, + Blockly.Events.CommentMove, + { + commentId: this.renderedComment.id, + oldCoordinate_: {x: 0, y: 0}, + newCoordinate_: {x: 42, y: 42}, + }, + this.workspace.id, + ); + }); + + test('change events are fired when a comments text is edited', function () { + this.renderedComment = new Blockly.comments.RenderedWorkspaceComment( + this.workspace, + ); + const spy = createChangeListenerSpy(this.workspace); + + this.renderedComment.setText('test text'); + + assertEventFired( + spy, + Blockly.Events.CommentChange, + { + commentId: this.renderedComment.id, + oldContents_: '', + newContents_: 'test text', + }, + this.workspace.id, + ); + }); }); });