From bc7f87f088f83b565e3020ac3a8626f1aa0a3b2a Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Wed, 14 Aug 2024 13:10:22 -0700 Subject: [PATCH 1/4] fix: improve variable deletion behaviors. --- blocks/variables.ts | 9 +++++---- blocks/variables_dynamic.ts | 9 +++++---- core/field_variable.ts | 7 ++++++- core/variables.ts | 15 ++++++++++++--- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/blocks/variables.ts b/blocks/variables.ts index 8ac038fb2ce..987c5ab2fdc 100644 --- a/blocks/variables.ts +++ b/blocks/variables.ts @@ -165,11 +165,12 @@ const deleteOptionCallbackFactory = function ( block: VariableBlock, ): () => void { return function () { - const workspace = block.workspace; const variableField = block.getField('VAR') as FieldVariable; - const variable = variableField.getVariable()!; - workspace.deleteVariableById(variable.getId()); - (workspace as WorkspaceSvg).refreshToolboxSelection(); + const variable = variableField.getVariable(); + if (variable) { + Variables.deleteVariable(variable.getWorkspace(), variable, block); + } + (block.workspace as WorkspaceSvg).refreshToolboxSelection(); }; }; diff --git a/blocks/variables_dynamic.ts b/blocks/variables_dynamic.ts index ff94d8c96c4..9c1167a19af 100644 --- a/blocks/variables_dynamic.ts +++ b/blocks/variables_dynamic.ts @@ -176,11 +176,12 @@ const renameOptionCallbackFactory = function (block: VariableBlock) { */ const deleteOptionCallbackFactory = function (block: VariableBlock) { return function () { - const workspace = block.workspace; const variableField = block.getField('VAR') as FieldVariable; - const variable = variableField.getVariable()!; - workspace.deleteVariableById(variable.getId()); - (workspace as WorkspaceSvg).refreshToolboxSelection(); + const variable = variableField.getVariable(); + if (variable) { + Variables.deleteVariable(variable.getWorkspace(), variable, block); + } + (block.workspace as WorkspaceSvg).refreshToolboxSelection(); }; }; diff --git a/core/field_variable.ts b/core/field_variable.ts index 23ea7c15a42..042299dc293 100644 --- a/core/field_variable.ts +++ b/core/field_variable.ts @@ -27,6 +27,7 @@ import * as fieldRegistry from './field_registry.js'; import * as internalConstants from './internal_constants.js'; import type {Menu} from './menu.js'; import type {MenuItem} from './menuitem.js'; +import {WorkspaceSvg} from './workspace_svg.js'; import {Msg} from './msg.js'; import * as parsing from './utils/parsing.js'; import {Size} from './utils/size.js'; @@ -514,7 +515,11 @@ export class FieldVariable extends FieldDropdown { return; } else if (id === internalConstants.DELETE_VARIABLE_ID && this.variable) { // Delete variable. - this.sourceBlock_.workspace.deleteVariableById(this.variable.getId()); + const workspace = this.variable.getWorkspace(); + Variables.deleteVariable(workspace, this.variable, this.sourceBlock_); + if (workspace instanceof WorkspaceSvg) { + workspace.refreshToolboxSelection(); + } return; } } diff --git a/core/variables.ts b/core/variables.ts index 5da228f6cc2..e4adad52a72 100644 --- a/core/variables.ts +++ b/core/variables.ts @@ -714,15 +714,20 @@ export function getVariableUsesById(workspace: Workspace, id: string): Block[] { * * @param workspace The workspace from which to delete the variable. * @param variable The variable to delete. + * @param triggeringBlock The block from which this deletion was triggered, if + * any. Used to exclude it from checking and warning about blocks + * referencing the variable being deleted. */ export function deleteVariable( workspace: Workspace, variable: IVariableModel, + triggeringBlock?: Block, ) { // Check whether this variable is a function parameter before deleting. const variableName = variable.getName(); const uses = getVariableUsesById(workspace, variable.getId()); - for (let i = 0, block; (block = uses[i]); i++) { + for (let i = uses.length - 1; i >= 0; i--) { + const block = uses[i]; if ( block.type === 'procedures_defnoreturn' || block.type === 'procedures_defreturn' @@ -734,9 +739,12 @@ export function deleteVariable( dialog.alert(deleteText); return; } + if (block === triggeringBlock) { + uses.splice(i, 1); + } } - if (uses.length > 1) { + if (uses.length) { // Confirm before deleting multiple blocks. const confirmText = Msg['DELETE_VARIABLE_CONFIRMATION'] .replace('%1', String(uses.length)) @@ -747,7 +755,8 @@ export function deleteVariable( } }); } else { - // No confirmation necessary for a single block. + // No confirmation necessary when the block that triggered the deletion is + // the only block referencing this variable. workspace.getVariableMap().deleteVariable(variable); } } From 91fe3e998598456f093f0ff8f10424a046dfa6db Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Wed, 14 Aug 2024 14:26:27 -0700 Subject: [PATCH 2/4] fix: don't prompt about deletion of only 1 variable block when triggered programmatically. --- core/variables.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/variables.ts b/core/variables.ts index e4adad52a72..779afca33db 100644 --- a/core/variables.ts +++ b/core/variables.ts @@ -744,7 +744,7 @@ export function deleteVariable( } } - if (uses.length) { + if ((triggeringBlock && uses.length) || uses.length > 1) { // Confirm before deleting multiple blocks. const confirmText = Msg['DELETE_VARIABLE_CONFIRMATION'] .replace('%1', String(uses.length)) @@ -756,7 +756,8 @@ export function deleteVariable( }); } else { // No confirmation necessary when the block that triggered the deletion is - // the only block referencing this variable. + // the only block referencing this variable or if only one block referencing + // this variable exists and the deletion was triggered programmatically. workspace.getVariableMap().deleteVariable(variable); } } From bddeebf567bb5f11b8f7c37d3a740fb818fc2cdf Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Wed, 14 Aug 2024 14:49:50 -0700 Subject: [PATCH 3/4] fix: include the triggering block in the count of referencing blocks --- core/variables.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/variables.ts b/core/variables.ts index 779afca33db..bad87df0be4 100644 --- a/core/variables.ts +++ b/core/variables.ts @@ -747,7 +747,7 @@ export function deleteVariable( if ((triggeringBlock && uses.length) || uses.length > 1) { // Confirm before deleting multiple blocks. const confirmText = Msg['DELETE_VARIABLE_CONFIRMATION'] - .replace('%1', String(uses.length)) + .replace('%1', String(uses.length + (triggeringBlock ? 1 : 0))) .replace('%2', variableName); dialog.confirm(confirmText, (ok) => { if (ok && variable) { From 523389021fc67e3c381fd373cbe2bb8d4af117b4 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Wed, 14 Aug 2024 15:00:10 -0700 Subject: [PATCH 4/4] fix: only count the triggering block as a referencing block if it's not in the flyout --- core/flyout_button.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index e73403d77a0..dfc7b950747 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -179,7 +179,7 @@ export class FlyoutButton implements IASTNodeLocationSvg { fontWeight, fontFamily, ); - this.height = fontMetrics.height; + this.height = this.height || fontMetrics.height; if (!this.isFlyoutLabel) { this.width += 2 * FlyoutButton.TEXT_MARGIN_X;