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: remove old render method #7308

Merged
merged 1 commit into from
Jul 27, 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
46 changes: 8 additions & 38 deletions core/block_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1549,44 +1549,14 @@ export class BlockSvg
* Immediately lays out and reflows a block based on its contents and
* settings.
*
* @param opt_bubble If false, just render this block.
* If true, also render block's parent, grandparent, etc. Defaults to true.
*/
render(opt_bubble?: boolean) {
if (this.renderIsInProgress_) {
return; // Don't allow recursive renders.
}
this.renderIsInProgress_ = true;
try {
this.rendered = true;
dom.startTextWidthCache();

if (!this.isEnabled()) {
// Apply disabled styles if needed.
this.updateDisabled();
}

if (this.isCollapsed()) {
this.updateCollapsed_();
}
this.workspace.getRenderer().render(this);
this.updateConnectionAndIconLocations();

if (opt_bubble !== false) {
const parentBlock = this.getParent();
if (parentBlock) {
parentBlock.render(true);
} else {
// Top-most block. Fire an event to allow scrollbars to resize.
this.workspace.resizeContents();
}
}

dom.stopTextWidthCache();
this.updateMarkers_();
} finally {
this.renderIsInProgress_ = false;
}
* @deprecated Renders are triggered automatically when the block is modified
* (e.g. fields are modified or inputs are added). Any calls to render()
* are no longer necessary. To be removed in v11.
*/
render() {
deprecation.warn('Blockly.BlockSvg.prototype.render', 'v10', 'v11');
this.queueRender();
renderManagement.triggerQueuedRenders();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/flyout_base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout {
}
block = blocks.appendInternal(
blockInfo as blocks.State,
this.workspace_
this.workspace_,
);
}
}
Expand Down
23 changes: 10 additions & 13 deletions core/workspace_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ import * as Xml from './xml.js';
import {ZoomControls} from './zoom_controls.js';
import {ContextMenuOption} from './contextmenu_registry.js';
import * as renderManagement from './render_management.js';
import * as deprecation from './utils/deprecation.js';

/** Margin around the top/bottom/left/right after a zoomToFit call. */
const ZOOM_TO_FIT_MARGIN = 20;
Expand Down Expand Up @@ -1224,24 +1225,20 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
// Currently does not support toolboxes in mutators.
this.toolbox_.setVisible(isVisible);
}
if (isVisible) {
const blocks = this.getAllBlocks(false);
// Tell each block on the workspace to mark its fields as dirty.
for (let i = blocks.length - 1; i >= 0; i--) {
blocks[i].markDirty();
}

this.render();
if (this.toolbox_) {
this.toolbox_.position();
}
} else {
if (!isVisible) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this one not needed? Setting the workspace visibility doesn't directly change the blocks or fields, so no rerender would be triggered.

Put another way: what case was this handling before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea what case this was handling before. But I can't think of a reason it's needed now! I manually tested showing and hiding the workspace, and everything looks correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dug into the blame, and the render existed originally when setVisible was implemented.: e8ab929 So maybe itw as necessary at some point? Or maybe it was just added cautiously.

I tested adding block to the workspace while the workspace was hidden, and that also works and renders things correctly. So I think this is safe to remove!

this.hideChaff(true);
}
}

/** Render all blocks in workspace. */
/**
* Render all blocks in workspace.
*
* @deprecated Renders are triggered automatically when the block is modified
* (e.g. fields are modified or inputs are added). Any calls to render()
* are no longer necessary. To be removed in v11.
*/
render() {
deprecation.warn('Blockly.WorkspaceSvg.prototype.render', 'v10', 'v11');
// Generate list of all blocks.
const blocks = this.getAllBlocks(false);
// Render each block.
Expand Down
2 changes: 1 addition & 1 deletion core/xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export function domToBlock(xmlBlock: Element, workspace: Workspace): Block {
*/
export function domToBlockInternal(
xmlBlock: Element,
workspace: Workspace
workspace: Workspace,
): Block {
// Create top-level block.
eventUtils.disable();
Expand Down