Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: comment have XML save and load new workspace comments classes #7931

Merged
merged 5 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/comments/comment_view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import * as dom from '../utils/dom.js';
import {Svg} from '../utils/svg.js';
import * as layers from '../layers.js';
import * as css from '../css.js';
import {Coordinate, Size, browserEvents} from '../utils.js';
import {Coordinate} from '../utils/coordinate.js';
import {Size} from '../utils/size.js';
import * as browserEvents from '../browser_events.js';
import * as touch from '../touch.js';

export class CommentView implements IRenderedElement {
Expand Down
85 changes: 63 additions & 22 deletions core/xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,42 +19,63 @@ import * as utilsXml from './utils/xml.js';
import type {VariableModel} from './variable_model.js';
import * as Variables from './variables.js';
import type {Workspace} from './workspace.js';
import {WorkspaceComment} from './workspace_comment.js';
import {WorkspaceCommentSvg} from './workspace_comment_svg.js';
import type {WorkspaceSvg} from './workspace_svg.js';
import {WorkspaceSvg} from './workspace_svg.js';
import * as renderManagement from './render_management.js';
import {WorkspaceComment} from './comments/workspace_comment.js';
import {RenderedWorkspaceComment} from './comments/rendered_workspace_comment.js';
import {Coordinate} from './utils/coordinate.js';

/**
* Encode a block tree as XML.
*
* @param workspace The workspace containing blocks.
* @param opt_noId True if the encoder should skip the block IDs.
* @param skipId True if the encoder should skip the block IDs. False by
* default.
* @returns XML DOM element.
*/
export function workspaceToDom(
workspace: Workspace,
opt_noId?: boolean,
): Element {
export function workspaceToDom(workspace: Workspace, skipId = false): Element {
const treeXml = utilsXml.createElement('xml');
const variablesElement = variablesToDom(
Variables.allUsedVarModels(workspace),
);
if (variablesElement.hasChildNodes()) {
treeXml.appendChild(variablesElement);
}
const comments = workspace.getTopComments(true);
for (let i = 0; i < comments.length; i++) {
const comment = comments[i];
treeXml.appendChild(comment.toXmlWithXY(opt_noId));
for (const comment of workspace.getTopComments()) {
treeXml.appendChild(
saveWorkspaceComment(comment as AnyDuringMigration, skipId),
);
}
const blocks = workspace.getTopBlocks(true);
for (let i = 0; i < blocks.length; i++) {
const block = blocks[i];
treeXml.appendChild(blockToDomWithXY(block, opt_noId));
treeXml.appendChild(blockToDomWithXY(block, skipId));
}
return treeXml;
}

/** Serializes the given workspace comment to XML. */
function saveWorkspaceComment(
comment: WorkspaceComment,
skipId = false,
): Element {
const elem = utilsXml.createElement('comment');
if (!skipId) elem.setAttribute('id', comment.id);

elem.setAttribute('x', `${comment.getRelativeToSurfaceXY().x}`);
maribethb marked this conversation as resolved.
Show resolved Hide resolved
elem.setAttribute('y', `${comment.getRelativeToSurfaceXY().y}`);
elem.setAttribute('w', `${comment.getSize().width}`);
elem.setAttribute('h', `${comment.getSize().height}`);

if (comment.getText()) elem.textContent = comment.getText();
if (comment.isCollapsed()) elem.setAttribute('collapsed', 'true');
if (!comment.isOwnEditable()) elem.setAttribute('editable', 'false');
if (!comment.isOwnMovable()) elem.setAttribute('movable', 'false');
if (!comment.isOwnDeletable()) elem.setAttribute('deletable', 'false');

return elem;
}

/**
* Encode a list of variables as XML.
*
Expand Down Expand Up @@ -443,15 +464,7 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
} else if (name === 'shadow') {
throw TypeError('Shadow block cannot be a top-level block.');
} else if (name === 'comment') {
if (workspace.rendered) {
WorkspaceCommentSvg.fromXmlRendered(
xmlChildElement,
workspace as WorkspaceSvg,
width,
);
} else {
WorkspaceComment.fromXml(xmlChildElement, workspace);
}
loadWorkspaceComment(xmlChildElement, workspace);
} else if (name === 'variables') {
if (variablesFirst) {
domToVariables(xmlChildElement, workspace);
Expand All @@ -478,6 +491,34 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
return newBlockIds;
}

/** Deserializes the given comment state into the given workspace. */
function loadWorkspaceComment(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why not have this implemented as a function on the class itself, like it used to be? It seems like that would make it easier on people who are implementing custom subclasses, and it would make more sense organizationally IMO. And we do have a pattern of a fromXml method on fields, events, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is allowed to implement custom subclasses atm. We discussed that and decided YAGNI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough :)

elem: Element,
workspace: Workspace,
): WorkspaceComment {
const id = elem.getAttribute('id') ?? undefined;
const comment = workspace.rendered
? new RenderedWorkspaceComment(workspace as WorkspaceSvg, id)
: new WorkspaceComment(workspace, id);

comment.setText(elem.textContent ?? '');

const x = parseInt(elem.getAttribute('x') ?? '', 10);
const y = parseInt(elem.getAttribute('y') ?? '', 10);
if (!isNaN(x) && !isNaN(y)) comment.moveTo(new Coordinate(x, y));

const w = parseInt(elem.getAttribute('w') ?? '', 10);
const h = parseInt(elem.getAttribute('h') ?? '', 10);
if (!isNaN(w) && !isNaN(h)) comment.setSize(new Size(w, h));

if (elem.getAttribute('collapsed') === 'true') comment.setCollapsed(true);
if (elem.getAttribute('editable') === 'false') comment.setEditable(false);
if (elem.getAttribute('movable') === 'false') comment.setMovable(false);
if (elem.getAttribute('deletable') === 'false') comment.setDeletable(false);

return comment;
}

/**
* Decode an XML DOM and create blocks on the workspace. Position the new
* blocks immediately below prior blocks, aligned by their starting edge.
Expand Down
3 changes: 2 additions & 1 deletion tests/mocha/clipboard_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ suite('Clipboard', function () {
});

suite('pasting comments', function () {
test('pasted comments are bumped to not overlap', function () {
// TODO: Reenable test when we readd copy-paste.
test.skip('pasted comments are bumped to not overlap', function () {
Blockly.Xml.domToWorkspace(
Blockly.utils.xml.textToDom(
'<xml><comment id="test" x=10 y=10/></xml>',
Expand Down
169 changes: 169 additions & 0 deletions tests/mocha/serializer_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1868,12 +1868,181 @@ Serializer.Mutations.testSuites = [
Serializer.Mutations.Procedure,
];

Serializer.Comments = new SerializerTestSuite('Comments');

Serializer.Comments.Coordinates = new SerializerTestSuite('Coordinates');
Serializer.Comments.Coordinates.Basic = new SerializerTestCase(
'Basic',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Coordinates.Negative = new SerializerTestCase(
'Negative',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="-42" y="-42" w="42" h="42">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Coordinates.Zero = new SerializerTestCase(
'Zero',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="0" y="0" w="42" h="42">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Coordinates.testCases = [
Serializer.Comments.Coordinates.Basic,
Serializer.Comments.Coordinates.Negative,
Serializer.Comments.Coordinates.Zero,
];

Serializer.Comments.Size = new SerializerTestSuite('Size');
Serializer.Comments.Size.Basic = new SerializerTestCase(
'Basic',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Size.testCases = [Serializer.Comments.Size.Basic];

Serializer.Comments.Text = new SerializerTestSuite('Text');
Serializer.Comments.Text.Symbols = new SerializerTestCase(
'Symbols',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'~`!@#$%^*()_+-={[}]|\\:;,.?/' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.EscapedSymbols = new SerializerTestCase(
'EscapedSymbols',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'&amp;&lt;&gt;' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.SingleQuotes = new SerializerTestCase(
'SingleQuotes',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
"'test'" +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.DoubleQuotes = new SerializerTestCase(
'DoubleQuotes',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'"test"' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.Numbers = new SerializerTestCase(
'Numbers',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'1234567890a123a123a' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.Emoji = new SerializerTestCase(
'Emoji',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'😀👋🏿👋🏾👋🏽👋🏼👋🏻😀❤❤❤' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.Russian = new SerializerTestCase(
'Russian',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'ты любопытный кот' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.Japanese = new SerializerTestCase(
'Japanese',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'あなたは好奇心旺盛な猫です' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.Zalgo = new SerializerTestCase(
'Zalgo',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42">' +
'z̴̪͈̲̜͕̽̈̀͒͂̓̋̉̍a̸̧̧̜̻̘̤̫̱̲͎̞̻͆̋ļ̸̛̖̜̳͚̖͔̟̈́͂̉̀͑̑͑̎ǵ̸̫̳̽̐̃̑̚̕o̶͇̫͔̮̼̭͕̹̘̬͋̀͆̂̇̋͊̒̽' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Text.testCases = [
Serializer.Comments.Text.Symbols,
Serializer.Comments.Text.EscapedSymbols,
Serializer.Comments.Text.SingleQuotes,
Serializer.Comments.Text.DoubleQuotes,
Serializer.Comments.Text.Numbers,
Serializer.Comments.Text.Emoji,
Serializer.Comments.Text.Russian,
Serializer.Comments.Text.Japanese,
Serializer.Comments.Text.Zalgo,
];

Serializer.Comments.Attributes = new SerializerTestSuite('Attributes');
Serializer.Comments.Attributes.Collapsed = new SerializerTestCase(
'Collapsed',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42" collapsed="true">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Attributes.NotEditable = new SerializerTestCase(
'NotEditable',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42" editable="false">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Attributes.NotMovable = new SerializerTestCase(
'NotMovable',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42" movable="false">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Attributes.NotDeletable = new SerializerTestCase(
'NotDeletable',
'<xml xmlns="https://developers.google.com/blockly/xml">' +
'<comment id="id******************" x="42" y="42" w="42" h="42" deletable="false">' +
'</comment>' +
'</xml>',
);
Serializer.Comments.Attributes.testCases = [
Serializer.Comments.Attributes.Collapsed,
Serializer.Comments.Attributes.NotEditable,
Serializer.Comments.Attributes.NotMovable,
Serializer.Comments.Attributes.NotDeletable,
];

Serializer.Comments.testSuites = [
Serializer.Comments.Coordinates,
Serializer.Comments.Size,
Serializer.Comments.Text,
Serializer.Comments.Attributes,
];

Serializer.testSuites = [
Serializer.Attributes,
Serializer.Fields,
Serializer.Icons,
Serializer.Connections,
Serializer.Mutations,
Serializer.Comments,
];

const runSerializerTestSuite = (serializer, deserializer, testSuite) => {
Expand Down