CodingPirates/blockly
1
0
Fork 0
mirror of https://github.com/google/blockly.git synced 2026-01-10 10:27:08 +01:00
Code Issues Packages Projects Releases Wiki Activity

fix: comment create and delete events (#7945)

Browse Source 
* chore: switch events to use new comment class

* fix: switch create and delete events to use JSON

* work on getting new comments to fire events

* chore: fixup tests

* chore: rename workspace comment test to comment view test

* chore: add tests for firing events

* chore: remove TODO
This commit is contained in:
Beka Westberg
2024-04-01 21:33:50 +00:00
committed by GitHub
parent 9effba5ee1
commit 63eb4ecb2a
16 changed files with 328 additions and 225 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
core/comments/workspace_comment.ts
View File

@@ -8,6 +8,7 @@ import {Workspace} from '../workspace.js';
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';
export class WorkspaceComment {
/** The unique identifier for this comment. */
@@ -56,7 +57,19 @@ export class WorkspaceComment {
// TODO: File an issue to remove this once everything is migrated.
workspace.addTopComment(this as AnyDuringMigration);
// TODO(7909): Fire events.
this.fireCreateEvent();
}
private fireCreateEvent() {
if (eventUtils.isEnabled()) {
eventUtils.fire(new (eventUtils.get(eventUtils.COMMENT_CREATE))(this));
}
}
private fireDeleteEvent() {
if (eventUtils.isEnabled()) {
eventUtils.fire(new (eventUtils.get(eventUtils.COMMENT_DELETE))(this));
}
}
/** Sets the text of the comment. */
@@ -165,6 +178,7 @@ export class WorkspaceComment {
/** Disposes of this comment. */
dispose() {
this.disposing = true;
this.fireDeleteEvent();
this.workspace.removeTopComment(this as AnyDuringMigration);
this.disposed = true;
}

17
core/events/events_comment_base.ts
View File

@@ -11,10 +11,8 @@
*/
// Former goog.module ID: Blockly.Events.CommentBase
import * as utilsXml from '../utils/xml.js';
import type {WorkspaceComment} from '../workspace_comment.js';
import * as Xml from '../xml.js';
import type {WorkspaceComment} from '../comments/workspace_comment.js';
import * as comments from '../serialization/workspace_comments.js';
import {
Abstract as AbstractEvent,
AbstractEventJson,
@@ -102,12 +100,10 @@ export class CommentBase extends AbstractEvent {
) {
const workspace = event.getEventWorkspace_();
if (create) {
const xmlElement = utilsXml.createElement('xml');
if (!event.xml) {
throw new Error('Ecountered a comment event without proper xml');
if (!event.json) {
throw new Error('Encountered a comment event without proper json');
}
xmlElement.appendChild(event.xml);
Xml.domToWorkspace(xmlElement, workspace);
comments.append(event.json, workspace);
} else {
if (!event.commentId) {
throw new Error(
@@ -119,8 +115,7 @@ export class CommentBase extends AbstractEvent {
if (comment) {
comment.dispose();
} else {
// Only complain about root-level block.
console.warn("Can't uncreate non-existent comment: " + event.commentId);
console.warn("Can't delete non-existent comment: " + event.commentId);
}
}
}

2
core/events/events_comment_change.ts
View File

@@ -12,7 +12,7 @@
// Former goog.module ID: Blockly.Events.CommentChange
import * as registry from '../registry.js';
import type {WorkspaceComment} from '../workspace_comment.js';
import type {WorkspaceComment} from '../comments/workspace_comment.js';
import {CommentBase, CommentBaseJson} from './events_comment_base.js';
import * as eventUtils from './utils.js';

23
core/events/events_comment_create.ts
View File

@@ -12,10 +12,10 @@
// Former goog.module ID: Blockly.Events.CommentCreate
import * as registry from '../registry.js';
import type {WorkspaceComment} from '../workspace_comment.js';
import type {WorkspaceComment} from '../comments/workspace_comment.js';
import * as comments from '../serialization/workspace_comments.js';
import * as utilsXml from '../utils/xml.js';
import * as Xml from '../xml.js';
import {CommentBase, CommentBaseJson} from './events_comment_base.js';
import * as eventUtils from './utils.js';
import type {Workspace} from '../workspace.js';
@@ -29,6 +29,9 @@ export class CommentCreate extends CommentBase {
/** The XML representation of the created workspace comment. */
xml?: Element | DocumentFragment;
/** The JSON representation of the created workspace comment. */
json?: comments.State;
/**
* @param opt_comment The created comment.
* Undefined for a blank event.
@@ -37,10 +40,11 @@ export class CommentCreate extends CommentBase {
super(opt_comment);
if (!opt_comment) {
return;
return; // Blank event to be populated by fromJson.
}
// Blank event to be populated by fromJson.
this.xml = opt_comment.toXmlWithXY();
this.xml = Xml.saveWorkspaceComment(opt_comment);
this.json = comments.save(opt_comment, {addCoordinates: true});
}
// TODO (#1266): "Full" and "minimal" serialization.
@@ -57,7 +61,14 @@ export class CommentCreate extends CommentBase {
'the constructor, or call fromJson',
);
}
if (!this.json) {
throw new Error(
'The comment JSON is undefined. Either pass a block to ' +
'the constructor, or call fromJson',
);
}
json['xml'] = Xml.domToText(this.xml);
json['json'] = this.json;
return json;
}
@@ -81,6 +92,7 @@ export class CommentCreate extends CommentBase {
event ?? new CommentCreate(),
) as CommentCreate;
newEvent.xml = utilsXml.textToDom(json['xml']);
newEvent.json = json['json'];
return newEvent;
}
@@ -96,6 +108,7 @@ export class CommentCreate extends CommentBase {
export interface CommentCreateJson extends CommentBaseJson {
xml: string;
json: object;
}
registry.register(

19
core/events/events_comment_delete.ts
View File

@@ -12,8 +12,8 @@
// Former goog.module ID: Blockly.Events.CommentDelete
import * as registry from '../registry.js';
import type {WorkspaceComment} from '../workspace_comment.js';
import type {WorkspaceComment} from '../comments/workspace_comment.js';
import * as comments from '../serialization/workspace_comments.js';
import {CommentBase, CommentBaseJson} from './events_comment_base.js';
import * as eventUtils from './utils.js';
import * as utilsXml from '../utils/xml.js';
@@ -29,6 +29,9 @@ export class CommentDelete extends CommentBase {
/** The XML representation of the deleted workspace comment. */
xml?: Element;
/** The JSON representation of the created workspace comment. */
json?: comments.State;
/**
* @param opt_comment The deleted comment.
* Undefined for a blank event.
@@ -40,7 +43,8 @@ export class CommentDelete extends CommentBase {
return; // Blank event to be populated by fromJson.
}
this.xml = opt_comment.toXmlWithXY();
this.xml = Xml.saveWorkspaceComment(opt_comment);
this.json = comments.save(opt_comment, {addCoordinates: true});
}
/**
@@ -65,7 +69,14 @@ export class CommentDelete extends CommentBase {
'the constructor, or call fromJson',
);
}
if (!this.json) {
throw new Error(
'The comment JSON is undefined. Either pass a block to ' +
'the constructor, or call fromJson',
);
}
json['xml'] = Xml.domToText(this.xml);
json['json'] = this.json;
return json;
}
@@ -89,12 +100,14 @@ export class CommentDelete extends CommentBase {
event ?? new CommentDelete(),
) as CommentDelete;
newEvent.xml = utilsXml.textToDom(json['xml']);
newEvent.json = json['json'];
return newEvent;
}
}
export interface CommentDeleteJson extends CommentBaseJson {
xml: string;
json: object;
}
registry.register(

2
core/events/events_comment_move.ts
View File

@@ -13,7 +13,7 @@
import * as registry from '../registry.js';
import {Coordinate} from '../utils/coordinate.js';
import type {WorkspaceComment} from '../workspace_comment.js';
import type {WorkspaceComment} from '../comments/workspace_comment.js';
import {CommentBase, CommentBaseJson} from './events_comment_base.js';
import * as eventUtils from './utils.js';

4
core/xml.ts
View File

@@ -55,7 +55,7 @@ export function workspaceToDom(workspace: Workspace, skipId = false): Element {
}
/** Serializes the given workspace comment to XML. */
function saveWorkspaceComment(
export function saveWorkspaceComment(
comment: WorkspaceComment,
skipId = false,
): Element {
@@ -495,7 +495,7 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
}
/** Deserializes the given comment state into the given workspace. */
function loadWorkspaceComment(
export function loadWorkspaceComment(
elem: Element,
workspace: Workspace,
): WorkspaceComment {

199
tests/mocha/comment_view_test.js Normal file
View File

@@ -0,0 +1,199 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import {
sharedTestSetup,
sharedTestTeardown,
} from './test_helpers/setup_teardown.js';
suite('Workspace comment', function () {
setup(function () {
sharedTestSetup.call(this);
this.workspace = new Blockly.inject('blocklyDiv', {});
this.commentView = new Blockly.comments.CommentView(this.workspace);
});
teardown(function () {
sharedTestTeardown.call(this);
});
suite('Listeners', function () {
suite('Text change listeners', function () {
test('text change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addTextChangeListener(spy);
this.commentView.setText('test');
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith('', 'test'),
'Expected the spy to be called with the given args',
);
});
test('text change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeTextChangeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addTextChangeListener(fake1);
this.commentView.addTextChangeListener(fake2);
this.commentView.addTextChangeListener(fake3);
this.commentView.setText('test');
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
suite('Size change listeners', function () {
test('size change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addSizeChangeListener(spy);
const originalSize = this.commentView.getSize();
const newSize = new Blockly.utils.Size(1337, 1337);
this.commentView.setSize(newSize);
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith(originalSize, newSize),
'Expected the spy to be called with the given args',
);
});
test('size change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeSizeChangeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addSizeChangeListener(fake1);
this.commentView.addSizeChangeListener(fake2);
this.commentView.addSizeChangeListener(fake3);
const newSize = new Blockly.utils.Size(1337, 1337);
this.commentView.setSize(newSize);
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
suite('Collapse change listeners', function () {
test('collapse change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addOnCollapseListener(spy);
this.commentView.setCollapsed(true);
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith(true),
'Expected the spy to be called with the given args',
);
});
test('collapse change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeOnCollapseListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addOnCollapseListener(fake1);
this.commentView.addOnCollapseListener(fake2);
this.commentView.addOnCollapseListener(fake3);
this.commentView.setCollapsed(true);
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
suite('Dispose change listeners', function () {
test('dispose listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addDisposeListener(spy);
this.commentView.dispose();
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
});
test('dispose listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeDisposeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addDisposeListener(fake1);
this.commentView.addDisposeListener(fake2);
this.commentView.addDisposeListener(fake3);
this.commentView.dispose();
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
});
});

8
tests/mocha/event_comment_change_test.js
View File

@@ -21,12 +21,8 @@ suite('Comment Change Event', function () {
suite('Serialization', function () {
test('events round-trip through JSON', function () {
const comment = new Blockly.WorkspaceComment(
this.workspace,
'old text',
10,
10,
);
const comment = new Blockly.comments.WorkspaceComment(this.workspace);
comment.setText('old text');
const origEvent = new Blockly.Events.CommentChange(
comment,
'old text',

9
tests/mocha/event_comment_create_test.js
View File

@@ -21,12 +21,9 @@ suite('Comment Create Event', function () {
suite('Serialization', function () {
test('events round-trip through JSON', function () {
const comment = new Blockly.WorkspaceComment(
this.workspace,
'test text',
10,
10,
);
const comment = new Blockly.comments.WorkspaceComment(this.workspace);
comment.setText('test text');
comment.moveTo(new Blockly.utils.Coordinate(10, 10));
const origEvent = new Blockly.Events.CommentCreate(comment);
const json = origEvent.toJson();

9
tests/mocha/event_comment_delete_test.js
View File

@@ -21,12 +21,9 @@ suite('Comment Delete Event', function () {
suite('Serialization', function () {
test('events round-trip through JSON', function () {
const comment = new Blockly.WorkspaceComment(
this.workspace,
'test text',
10,
10,
);
const comment = new Blockly.comments.WorkspaceComment(this.workspace);
comment.setText('test text');
comment.moveTo(new Blockly.utils.Coordinate(10, 10));
const origEvent = new Blockly.Events.CommentDelete(comment);
const json = origEvent.toJson();

11
tests/mocha/event_comment_move_test.js
View File

@@ -21,14 +21,11 @@ suite('Comment Move Event', function () {
suite('Serialization', function () {
test('events round-trip through JSON', function () {
const comment = new Blockly.WorkspaceComment(
this.workspace,
'test text',
10,
10,
);
const comment = new Blockly.comments.WorkspaceComment(this.workspace);
comment.setText('test text');
comment.moveTo(new Blockly.utils.Coordinate(10, 10));
const origEvent = new Blockly.Events.CommentMove(comment);
comment.moveBy(10, 10);
comment.moveTo(new Blockly.utils.Coordinate(20, 20));
origEvent.recordNew();
const json = origEvent.toJson();

34
tests/mocha/event_test.js
View File

@@ -824,7 +824,19 @@ suite('Events', function () {
type: 'comment_create',
group: '',
commentId: thisObj.comment.id,
xml: Blockly.Xml.domToText(thisObj.comment.toXmlWithXY()),
// TODO: Before merging, is this a dumb change detector?
xml: Blockly.Xml.domToText(
Blockly.Xml.saveWorkspaceComment(thisObj.comment),
{addCoordinates: true},
),
json: {
height: 100,
width: 120,
id: 'comment id',
x: 0,
y: 0,
text: 'test text',
},
}),
},
{
@@ -835,7 +847,19 @@ suite('Events', function () {
type: 'comment_delete',
group: '',
commentId: thisObj.comment.id,
xml: Blockly.Xml.domToText(thisObj.comment.toXmlWithXY()),
// TODO: Before merging, is this a dumb change detector?
xml: Blockly.Xml.domToText(
Blockly.Xml.saveWorkspaceComment(thisObj.comment),
{addCoordinates: true},
),
json: {
height: 100,
width: 120,
id: 'comment id',
x: 0,
y: 0,
text: 'test text',
},
}),
},
// TODO(#4577) Test serialization of move event coordinate properties.
@@ -873,13 +897,11 @@ suite('Events', function () {
title: 'WorkspaceComment events',
testCases: workspaceCommentEventTestCases,
setup: (thisObj) => {
thisObj.comment = new Blockly.WorkspaceComment(
thisObj.comment = new Blockly.comments.WorkspaceComment(
thisObj.workspace,
'comment text',
0,
0,
'comment id',
);
thisObj.comment.setText('test text');
},
},
];

1
tests/mocha/index.html
View File

@@ -121,6 +121,7 @@
import './variable_model_test.js';
import './blocks/variables_test.js';
import './widget_div_test.js';
import './comment_view_test.js';
import './workspace_comment_test.js';
import './workspace_svg_test.js';
import './workspace_test.js';

2
tests/mocha/old_workspace_comment_test.js
View File

@@ -9,7 +9,7 @@ import {
sharedTestTeardown,
} from './test_helpers/setup_teardown.js';
suite('Workspace comment', function () {
suite.skip('Workspace comment', function () {
setup(function () {
sharedTestSetup.call(this);
this.workspace = new Blockly.Workspace();

197
tests/mocha/workspace_comment_test.js
View File

@@ -8,192 +8,51 @@ import {
sharedTestSetup,
sharedTestTeardown,
} from './test_helpers/setup_teardown.js';
import {
createChangeListenerSpy,
assertEventFired,
} from './test_helpers/events.js';
suite('Workspace comment', function () {
setup(function () {
sharedTestSetup.call(this);
this.workspace = new Blockly.inject('blocklyDiv', {});
this.commentView = new Blockly.comments.CommentView(this.workspace);
});
teardown(function () {
sharedTestTeardown.call(this);
});
suite('Listeners', function () {
suite('Text change listeners', function () {
test('text change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addTextChangeListener(spy);
suite('Events', function () {
test('create events are fired when a comment is constructed', function () {
const spy = createChangeListenerSpy(this.workspace);
this.commentView.setText('test');
this.renderedComment = new Blockly.comments.RenderedWorkspaceComment(
this.workspace,
);
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith('', 'test'),
'Expected the spy to be called with the given args',
);
});
test('text change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeTextChangeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addTextChangeListener(fake1);
this.commentView.addTextChangeListener(fake2);
this.commentView.addTextChangeListener(fake3);
this.commentView.setText('test');
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
assertEventFired(
spy,
Blockly.Events.CommentCreate,
{commentId: this.renderedComment.id},
this.workspace.id,
);
});
suite('Size change listeners', function () {
test('size change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addSizeChangeListener(spy);
const originalSize = this.commentView.getSize();
const newSize = new Blockly.utils.Size(1337, 1337);
test('delete events are fired when a comment is disposed', function () {
this.renderedComment = new Blockly.comments.RenderedWorkspaceComment(
this.workspace,
);
const spy = createChangeListenerSpy(this.workspace);
this.commentView.setSize(newSize);
this.renderedComment.dispose();
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith(originalSize, newSize),
'Expected the spy to be called with the given args',
);
});
test('size change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeSizeChangeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addSizeChangeListener(fake1);
this.commentView.addSizeChangeListener(fake2);
this.commentView.addSizeChangeListener(fake3);
const newSize = new Blockly.utils.Size(1337, 1337);
this.commentView.setSize(newSize);
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
suite('Collapse change listeners', function () {
test('collapse change listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addOnCollapseListener(spy);
this.commentView.setCollapsed(true);
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
chai.assert.isTrue(
spy.calledWith(true),
'Expected the spy to be called with the given args',
);
});
test('collapse change listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeOnCollapseListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addOnCollapseListener(fake1);
this.commentView.addOnCollapseListener(fake2);
this.commentView.addOnCollapseListener(fake3);
this.commentView.setCollapsed(true);
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
});
suite('Dispose change listeners', function () {
test('dispose listeners are called when text is changed', function () {
const spy = sinon.spy();
this.commentView.addDisposeListener(spy);
this.commentView.dispose();
chai.assert.isTrue(
spy.calledOnce,
'Expected the spy to be called once',
);
});
test('dispose listeners can remove themselves without skipping others', function () {
const fake1 = sinon.fake();
const fake2 = sinon.fake(() =>
this.commentView.removeDisposeListener(fake2),
);
const fake3 = sinon.fake();
this.commentView.addDisposeListener(fake1);
this.commentView.addDisposeListener(fake2);
this.commentView.addDisposeListener(fake3);
this.commentView.dispose();
chai.assert.isTrue(
fake1.calledOnce,
'Expected the first listener to be called',
);
chai.assert.isTrue(
fake2.calledOnce,
'Expected the second listener to be called',
);
chai.assert.isTrue(
fake3.calledOnce,
'Expected the third listener to be called',
);
});
assertEventFired(
spy,
Blockly.Events.CommentDelete,
{commentId: this.renderedComment.id},
this.workspace.id,
);
});
});
});