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.

Sign up for GitHub

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

Jump to bottom

fix!: have disposing be true from start of dispose #7891

Merged
merged 1 commit into from
Mar 13, 2024
Merged

fix!: have disposing be true from start of dispose #7891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions core/block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class Block implements IASTNodeLocation, IDeletable {
/**
* Is the current block currently in the process of being disposed?
*/
private disposing = false;
protected disposing = false;

/**
* Has this block been fully initialized? E.g. all fields initailized.
Expand Down Expand Up @@ -319,7 +319,7 @@ export class Block implements IASTNodeLocation, IDeletable {
* children of this block.
*/
dispose(healStack: boolean) {
if (this.isDeadOrDying()) return;
this.disposing = true;

// Dispose of this change listener before unplugging.
// Technically not necessary due to the event firing delay.
Expand All @@ -342,15 +342,13 @@ export class Block implements IASTNodeLocation, IDeletable {
* E.g. does not fire events, unplug the block, etc.
*/
protected disposeInternal() {
if (this.isDeadOrDying()) return;

this.disposing = true;
if (this.onchangeWrapper_) {
this.workspace.removeChangeListener(this.onchangeWrapper_);
}

this.workspace.removeTypedBlock(this);
this.workspace.removeBlockById(this.id);
this.disposing = true;

if (typeof this.destroy === 'function') this.destroy();

Expand Down
4 changes: 2 additions & 2 deletions core/block_svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,7 @@ export class BlockSvg
* @param animate If true, show a disposal animation and sound.
*/
override dispose(healStack?: boolean, animate?: boolean) {
if (this.isDeadOrDying()) return;
this.disposing = true;

Tooltip.dispose();
ContextMenu.hide();
Expand All @@ -795,7 +795,7 @@ export class BlockSvg
* E.g. does trigger UI effects, remove nodes, etc.
*/
override disposeInternal() {
if (this.isDeadOrDying()) return;
this.disposing = true;
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 pretty sure that this one is redundant and can be removed, since you do a super call immediately and Block.prototype.disposeInternal also sets this.disposing = true.

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 agree, but to future-proof this (in case we ever add things before calling super) and make it extremely clear, I think it's reasonable to leave it in.

If this were more expensive I would remove it. But since it's cheap I think it's reasonable to leave.

super.disposeInternal();

if (common.getSelected() === this) {
Expand Down
9 changes: 6 additions & 3 deletions core/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,10 @@ export class Connection implements IASTNodeLocationWithBlock {
dispose() {
// isConnected returns true for shadows and non-shadows.
if (this.isConnected()) {
// Destroy the attached shadow block & its children (if it exists).
this.setShadowStateInternal();
if (this.isSuperior()) {
// Destroy the attached shadow block & its children (if it exists).
this.setShadowStateInternal();
}

const targetBlock = this.targetBlock();
if (targetBlock && !targetBlock.isDeadOrDying()) {
Expand Down Expand Up @@ -600,6 +602,8 @@ export class Connection implements IASTNodeLocationWithBlock {
this.shadowDom = shadowDom;
this.shadowState = shadowState;

if (this.getSourceBlock().isDeadOrDying()) return;

const target = this.targetBlock();
if (!target) {
this.respawnShadow_();
Expand All @@ -608,7 +612,6 @@ export class Connection implements IASTNodeLocationWithBlock {
}
} else if (target.isShadow()) {
target.dispose(false);
if (this.getSourceBlock().isDeadOrDying()) return;
this.respawnShadow_();
if (this.targetBlock() && this.targetBlock()!.isShadow()) {
this.serializeShadow(this.targetBlock());
Expand Down
6 changes: 3 additions & 3 deletions core/xml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,13 @@ export function blockToDom(
if (!block.isEnabled()) {
element.setAttribute('disabled', 'true');
}
if (!block.isDeletable() && !block.isShadow()) {
if (!block.isOwnDeletable()) {
element.setAttribute('deletable', 'false');
}
if (!block.isMovable() && !block.isShadow()) {
if (!block.isOwnMovable()) {
element.setAttribute('movable', 'false');
}
if (!block.isEditable()) {
if (!block.isOwnEditable()) {
element.setAttribute('editable', 'false');
}

Expand Down
22 changes: 17 additions & 5 deletions tests/mocha/block_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ suite('Blocks', function () {

function createTestBlocks(workspace, isRow) {
const blockType = isRow ? 'row_block' : 'stack_block';
const blockA = workspace.newBlock(blockType);
const blockB = workspace.newBlock(blockType);
const blockC = workspace.newBlock(blockType);
const blockA = workspace.newBlock(blockType, 'a');
const blockB = workspace.newBlock(blockType, 'b');
const blockC = workspace.newBlock(blockType, 'c');

if (isRow) {
blockA.inputList[0].connection.connect(blockB.outputConnection);
Expand Down Expand Up @@ -386,8 +386,14 @@ suite('Blocks', function () {

test('Child is shadow', function () {
const blocks = this.blocks;
blocks.C.setShadow(true);
blocks.C.dispose();
blocks.B.inputList[0].connection.setShadowState({
'type': 'row_block',
'id': 'c',
});

blocks.B.dispose(true);

// Even though we're asking to heal, it will appear as if it has not
// healed because shadows always get destroyed.
assertDisposedNoheal(blocks);
Expand Down Expand Up @@ -423,8 +429,14 @@ suite('Blocks', function () {

test('Child is shadow', function () {
const blocks = this.blocks;
blocks.C.setShadow(true);
blocks.C.dispose();
blocks.B.nextConnection.setShadowState({
'type': 'stack_block',
'id': 'c',
});

blocks.B.dispose(true);

// Even though we're asking to heal, it will appear as if it has not
// healed because shadows always get destroyed.
assertDisposedNoheal(blocks);
Expand Down
Loading