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

* fix: have XML save and load new comment classes

* chore: fix imports to resolve circular dependencies

* chore: add round-trip tests

* chore: skip failing test

* fixup: PR comments

core/comments/comment_view.ts

@@ -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 {

core/xml.ts

@@ -19,22 +19,21 @@ 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 {WorkspaceSvg} from './workspace_svg.js';
-import {WorkspaceCommentSvg} from './workspace_comment_svg.js';
-import type {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(
+export function workspaceToDom(workspace: Workspace, skipId = false): Element {
-  workspace: Workspace,
-  opt_noId?: boolean,
-): Element {
   const treeXml = utilsXml.createElement('xml');
   const variablesElement = variablesToDom(
     Variables.allUsedVarModels(workspace),
@@ -42,19 +41,41 @@ export function workspaceToDom(
   if (variablesElement.hasChildNodes()) {
     treeXml.appendChild(variablesElement);
   }
-  const comments = workspace.getTopComments(true);
+  for (const comment of workspace.getTopComments()) {
-  for (let i = 0; i < comments.length; i++) {
+    treeXml.appendChild(
-    const comment = comments[i];
+      saveWorkspaceComment(comment as AnyDuringMigration, skipId),
-    treeXml.appendChild(comment.toXmlWithXY(opt_noId));
+    );
   }
   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}`);
+  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.
  *
@@ -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) {
+        loadWorkspaceComment(xmlChildElement, workspace);
-          WorkspaceCommentSvg.fromXmlRendered(
-            xmlChildElement,
-            workspace as WorkspaceSvg,
-            width,
-          );
-        } else {
-          WorkspaceComment.fromXml(xmlChildElement, workspace);
-        }
       } else if (name === 'variables') {
         if (variablesFirst) {
           domToVariables(xmlChildElement, workspace);
@@ -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(
+  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.

tests/mocha/clipboard_test.js

@@ -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>',

tests/mocha/serializer_test.js

@@ -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) => {