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: make deserialization use the new render management system #7306

Merged
merged 4 commits into from
Jul 26, 2023
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
2 changes: 1 addition & 1 deletion core/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ export class Connection implements IASTNodeLocationWithBlock {
}

if (shadowDom) {
blockShadow = Xml.domToBlock(shadowDom, parentBlock.workspace);
blockShadow = Xml.domToBlockInternal(shadowDom, parentBlock.workspace);
if (attemptToConnect) {
if (this.type === ConnectionType.INPUT_VALUE) {
if (!blockShadow.outputConnection) {
Expand Down
2 changes: 1 addition & 1 deletion core/contextmenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export function callbackFactory(block: Block, xml: Element): () => void {
eventUtils.disable();
let newBlock;
try {
newBlock = Xml.domToBlock(xml, block.workspace!) as BlockSvg;
newBlock = Xml.domToBlockInternal(xml, block.workspace!) as BlockSvg;
// Move the new block next to the old block.
const xy = block.getRelativeToSurfaceXY();
if (block.RTL) {
Expand Down
10 changes: 8 additions & 2 deletions core/flyout_base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import * as Variables from './variables.js';
import {WorkspaceSvg} from './workspace_svg.js';
import * as utilsXml from './utils/xml.js';
import * as Xml from './xml.js';
import * as renderManagement from './render_management.js';

enum FlyoutItemType {
BLOCK = 'block',
Expand Down Expand Up @@ -626,6 +627,8 @@ export abstract class Flyout extends DeleteArea implements IFlyout {
const parsedContent = toolbox.convertFlyoutDefToJsonArray(flyoutDef);
const flyoutInfo = this.createFlyoutInfo(parsedContent);

renderManagement.triggerQueuedRenders();
BeksOmega marked this conversation as resolved.
Show resolved Hide resolved

this.layout_(flyoutInfo.contents, flyoutInfo.gaps);

if (this.horizontalLayout) {
Expand Down Expand Up @@ -770,7 +773,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout {
) as Element;
block = this.getRecycledBlock(xml.getAttribute('type')!);
if (!block) {
block = Xml.domToBlock(xml, this.workspace_);
block = Xml.domToBlockInternal(xml, this.workspace_);
}
} else {
block = this.getRecycledBlock(blockInfo['type']!);
Expand All @@ -779,7 +782,10 @@ export abstract class Flyout extends DeleteArea implements IFlyout {
blockInfo['enabled'] =
blockInfo['disabled'] !== 'true' && blockInfo['disabled'] !== true;
}
block = blocks.append(blockInfo as blocks.State, this.workspace_);
block = blocks.appendInternal(
blockInfo as blocks.State,
this.workspace_
);
}
}

Expand Down
9 changes: 7 additions & 2 deletions core/serialization/blocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import * as registry from '../registry.js';
import * as utilsXml from '../utils/xml.js';
import type {Workspace} from '../workspace.js';
import * as Xml from '../xml.js';
import * as renderManagement from '../render_management.js';

import {
BadConnectionCheck,
Expand Down Expand Up @@ -349,7 +350,9 @@ export function append(
workspace: Workspace,
{recordUndo = false}: {recordUndo?: boolean} = {}
): Block {
return appendInternal(state, workspace, {recordUndo});
const block = appendInternal(state, workspace, {recordUndo});
if (workspace.rendered) renderManagement.triggerQueuedRenders();
return block;
}

/**
Expand Down Expand Up @@ -701,7 +704,9 @@ function initBlock(block: Block, rendered: boolean) {
blockSvg.setConnectionTracking(false);

blockSvg.initSvg();
blockSvg.render(false);
blockSvg.queueRender();
blockSvg.updateDisabled();

// fixes #6076 JSO deserialization doesn't
// set .iconXY_ property so here it will be set
for (const icon of blockSvg.getIcons()) {
Expand Down
2 changes: 1 addition & 1 deletion core/workspace_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,7 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
let blockX = 0;
let blockY = 0;
if (xmlBlock) {
block = Xml.domToBlock(xmlBlock, this) as BlockSvg;
block = Xml.domToBlockInternal(xmlBlock, this) as BlockSvg;
blockX = parseInt(xmlBlock.getAttribute('x') ?? '0');
if (this.RTL) {
blockX = -blockX;
Expand Down
33 changes: 28 additions & 5 deletions core/xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ 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 * as renderManagement from './render_management.js';

/**
* Encode a block tree as XML.
Expand Down Expand Up @@ -430,7 +431,7 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
// Allow top-level shadow blocks if recordUndo is disabled since
// that means an undo is in progress. Such a block is expected
// to be moved to a nested destination in the next operation.
const block = domToBlock(xmlChildElement, workspace);
const block = domToBlockInternal(xmlChildElement, workspace);
newBlockIds.push(block.id);
const blockX = parseInt(xmlChildElement.getAttribute('x') ?? '10', 10);
const blockY = parseInt(xmlChildElement.getAttribute('y') ?? '10', 10);
Expand Down Expand Up @@ -467,12 +468,13 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
}
} finally {
eventUtils.setGroup(existingGroup);
if ((workspace as WorkspaceSvg).setResizesEnabled) {
(workspace as WorkspaceSvg).setResizesEnabled(true);
}
if (workspace.rendered) renderManagement.triggerQueuedRenders();
dom.stopTextWidthCache();
}
// Re-enable workspace resizing.
if ((workspace as WorkspaceSvg).setResizesEnabled) {
(workspace as WorkspaceSvg).setResizesEnabled(true);
}
eventUtils.fire(new (eventUtils.get(eventUtils.FINISHED_LOADING))(workspace));
return newBlockIds;
}
Expand Down Expand Up @@ -545,6 +547,27 @@ export function appendDomToWorkspace(
* @returns The root block created.
*/
export function domToBlock(xmlBlock: Element, workspace: Workspace): Block {
const block = domToBlockInternal(xmlBlock, workspace);
if (workspace.rendered) renderManagement.triggerQueuedRenders();
return block;
}

/**
* Decode an XML block tag and create a block (and possibly sub blocks) on the
* workspace.
*
* This is defined internally so that it doesn't trigger an immediate render,
* which we do want to happen for external calls.
*
* @param xmlBlock XML block element.
* @param workspace The workspace.
* @returns The root block created.
* @internal
*/
export function domToBlockInternal(
Copy link
Contributor

Choose a reason for hiding this comment

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

I get why you made domToBlock render immediately, but I don't love this name. It's not very descriptive, and it implies external developers would never want to call it. But in reality they might be perfectly fine not triggering an immediate render. What about domToBlockAsync? or something better than that in the same vein? Then we don't have to mark it as internal.

Or is your plan to eventually make the breaking change to domToBlock? in that case i don't care quite as much if this method is intended to be temporary

Copy link
Collaborator Author

@BeksOmega BeksOmega Jul 24, 2023

Choose a reason for hiding this comment

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

My plan is indeed to eventually make the breaking change to domToBlock (unless there's an important reason we don't want to do that)! I chose the domToBlockInternal name to match the naming of the JSON serialization system.

xmlBlock: Element,
workspace: Workspace
): Block {
// Create top-level block.
eventUtils.disable();
const variablesBeforeCreation = workspace.getAllVariables();
Expand All @@ -561,7 +584,7 @@ export function domToBlock(xmlBlock: Element, workspace: Workspace): Block {
(blocks[i] as BlockSvg).initSvg();
}
for (let i = blocks.length - 1; i >= 0; i--) {
(blocks[i] as BlockSvg).render(false);
(blocks[i] as BlockSvg).queueRender();
}
// Populating the connection database may be deferred until after the
// blocks have rendered.
Expand Down
3 changes: 2 additions & 1 deletion tests/mocha/flyout_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {

suite('Flyout', function () {
setup(function () {
sharedTestSetup.call(this);
this.clock = sharedTestSetup.call(this, {fireEventsNow: false}).clock;
Blockly.defineBlocksWithJsonArray([
{
'type': 'basic_block',
Expand All @@ -48,6 +48,7 @@ suite('Flyout', function () {
});

teardown(function () {
this.clock.runAll();
sharedTestTeardown.call(this);
});

Expand Down