Custom handling when the block is disposed

[e-gloo]

Is your feature request related to a problem? Please describe.

If I understood correctly (and has it is in the code), we cannot yet make a variable referenced by a field other than a FieldVariable. I needed a way to make 2 fields, a text field and a dropdown field, being linked somehow, so whenever we change the value in the text field, it is updated in the dropdown field at the same time, mainly when the value selected is the same as the value from the text field. In order to achieve this, when the block of the text field is created, it creates a variable with the same id as the block id, and the same name as the text field value. Then, when the value of the text field is updated, it renames the appropriate variable with the new value. From there, the block of the dropdown field listen when the variable is renamed, and then update the current value if needed.
Everything works just fine, except the block of the text field is disposed. As the block does not get the "delete" event of himself, it can't delete the related variable, and the value will never be updated in the dropdown field.

Describe the solution you'd like

There's already init function at the block definition, maybe we could have destroy function which would be called when the block is disposed, then we would have the same principle has a constructor/destructor.

Describe alternatives you've considered

Other ideas:

• Create a new type of extension that would be called when the block is disposed instead of when the block is created.
• Dispose the block after the onchange function is called which would let the current block get the event of it's deletion.

Additional context

Note that this is pretty much a workaround a field not being able to reference a variable. It would be better to get this feature as the variable's name would depend on the text field value, and calling refreshVariableName would update the dropdown field value. However, we would still need a way to delete the variable when the block of the text field is disposed.

Any other workarounds or ideas to handle this. The only thing I want to avoid if possible, is to handle this in the workspace change listener, as I would need to handle every disposed block in there.

Thank you.

[BeksOmega]

Hello, thank you for filing this :D Would you be able to post your current code for handling this? I'm curious about why you're using a dropdown field with a variable, and not either a variable field or a dynamic dropdown field.

The only thing I want to avoid if possible, is to handle this in the workspace change listener, as I would need to handle every disposed block in there.

Could you elaborate on this? If you want to add extra functionality when blocks are disposed, event listeners are definitely the best way to do so =)

Best wishes,
--Beka

[e-gloo]

Hello!

Would you be able to post your current code for handling this?

Sure! This is an example with a css class block. One block defines a css class, and the other has a dropdown with all the defined classes. If it's still unclear, don't hesitate to tell me, I will try to explain differently or update my code.

// cssClassBlocks.json
[
  {
    "type": "style_class_def",
    "message0": "class name %1 %2 %3",
    "args0": [
      {
        "type": "field_input",
        "name": "CLASS_NAME",
        "text": "unamed"
      },
      {
        "type": "input_dummy"
      },
      {
        "type": "input_statement",
        "name": "STYLES",
        "check": "style_stmt"
      }
    ],
    "previousStatement": "style_class_def",
    "nextStatement": "style_class_def",
    "colour": 230,
    "tooltip": "",
    "helpUrl": "",
    "extensions": ["style_class_def_on_change"]
  },
  {
    "type": "style_class_name",
    "message0": "class %1",
    "args0": [
      {
        "type": "input_value",
        "name": "CLASSES",
        "check": "style_class_name"
      }
    ],
    "output": "style_class_name",
    "colour": 230,
    "tooltip": "",
    "helpUrl": "",
    "extensions": [
      "style_value_class_dynamic_options",
      "style_class_def_on_change_name"
    ]
  }
]

// cssClass.js
import * as Blockly from "blockly/core";
import {
  dynamicDropdownFromBlockExt,
  dynamicOptionsFromVariableOfType,
} from "../../utils/dropdownBlockExtension";
import classBlocks from "./cssClassBlocks.json";

Blockly.defineBlocksWithJsonArray(classBlocks);

Blockly.Extensions.register("style_value_class_dynamic_options", function () {
  this.getInput("CLASSES").appendField(
    new Blockly.FieldDropdown(
      dynamicOptionsFromVariableOfType(this, "style_class_def")
    ),
    "CLASS_NAME"
  );
});

Blockly.Extensions.register("style_class_def_on_change", function () {
  this.setOnChange(
    dynamicDropdownFromBlockExt(this, "style_class_def", "CLASS_NAME")
  );
});

// dropdownBlockExtension.js
export function dynamicOptionsFromVariableOfType(block, varType) {
  return () => {
    // Get all variable of the block type we want
    const variables = block.workspace.getVariablesOfType(varType);
    let options = [];
    // if no variable exists, then default value
    if (!variables.length) {
      return [["unamed", "unamed"]];
    }
    // variable name as option text, merge of id and name as option value
    for (const variable of variables) {
      options.push([variable.name, `${variable.getId()};;;${variable.name}`]);
    }
    return options;
  };
}

export function createOrRenameVariable(workspace, name, type, id) {
  // check if the variable exists before creating it
  let variable = workspace.getVariableById(id);
  if (!variable) {
    variable = workspace.createVariable(name, type, id);
  } else {
    // if already exists, then rename it
    workspace.renameVariableById(id, name);
  }
}

export function updateCurrentFieldValue(event, field, varId, oldName, newName) {
  const currentValue = field.getValue();
  // if current dropdown value is the value linked to the current updated variable
  if (currentValue === `${varId};;;${oldName}`) {
    field.getOptions(false);
    if (event === "var_delete") {
      field.doValueUpdate_("unamed");
      field.forceRerender();
    } else {
      field.doValueUpdate_(`${varId};;;${newName}`);
      field.forceRerender();
    }
  }
}

export function dynamicDropdownFromBlockExt(block, blockType, blockFieldName) {
  return (event) => {
    if (block.isInFlyout) {
      return;
    }
    if (
      // if the current block is changed and the block is the type we want
      event.type === "change" &&
      event.blockId === block.id &&
      block.type === blockType
    ) {
      // then create or rename a variable on block field new value
      createOrRenameVariable(
        block.workspace,
        event.newValue,
        block.type,
        block.id
      );
    } else if (
      // if a variable of the type we want is created or renamed
      ["var_create", "var_rename"].includes(event.type) &&
      event.varId !== undefined &&
      block.workspace.getVariableById(event.varId).type === blockType
    ) {
      // then update current value to the new variable name if it's the current name
      updateCurrentFieldValue(
        event.type,
        block.getField(blockFieldName),
        event.varId,
        event.oldName,
        event.newName
      );
    } else if (
      // if a variable of the type we want is deleted
      event.type === "var_delete" &&
      event.varType === blockType &&
      event.varId !== block.id
    ) {
      // then remove the variable from the options
      updateCurrentFieldValue(
        event.type,
        block.getField(blockFieldName),
        event.varId,
        event.varName,
        event.varName
      );
    }
  };
}

Could you elaborate on this? If you want to add extra functionality when blocks are disposed, event listeners are definitely the best way to do so =)

I agree, but I think that it's better to handle the event inside the block listener itself rather than handling each block that is disposed in the listener of the workspace. I'm using the same mechanic for different types of block, so I think it would be better to handle this inside the block.

In any case, I think it would be better for a block to know when it is disposed, so it can execute some cleaning if needed, like removing environment variables/context that depend on it. What do you think?

[cpcallen]

@e-gloo: thanks for the sample code. I will let Beka look at it more closely.

But, taking a step back: I am wondering if variables are the right fit for what you are trying to do here. AIUI Variables are specifically designed to model mutable user variables—i.e. mutable state—in the generated code (e.g. let statements in JavaScript), and here it seems like you are using them just to link declarations with usages of your CSS classes. That's not unreasonable, but I wonder whether CSS declarations are more akin to functions rather than variables. That probably gets you a separate use-block for each CSS class, though, rather than a single block with a dropdown, which might not be quite what you are looking for.

[BeksOmega]

Hi @e-gloo Thanks again for the code! This was very helpful for understanding your use case.

I think that using Variables and FieldVariables for this is actually the right approach. Personally, I think of FieldVariables as more like FieldIdentifiers because they don't actually have any specific functionality for modeling mutable state, that's just what they happened to be built for originally.

I also think you're correct that blocks should receive their own dispose events so that they can clean up after themselves. A refactor of the procedure's system I've been planning actually relies on this functionality existing, and I didn't realize that it's not supported haha.

Fixing this is actually a very simple change, it just involves moving the code for removing the change listener to after the delete event is fired. However, if external developers have an onchange listener that uses a denylist rather than an allowlist, it will cause an extra trigger of their onchange listener that is unintended, which may cause bugs. So I'm not sure if this would be considered a breaking change, and as such if/when we want to do it. @maribethb

For now, you can definitely take the workspace-change-listener approach. I know it's not ideal :/ But it will allow you to achieve your intended behavior.

[e-gloo]

Thank you @cpcallen and @BeksOmega for your time and answers!

@cpcallen: I thought about it at the beginning as well, but I just found it so handy to be able to switch classes by a dropdown, knowing that you probably get more classes in an html document than functions in a basic exercise to learn the notion. I don't know yet, maybe I'm wrong and it would be better to use it as function...

@BeksOmega: I thought about Variables and FieldVariables as well first, but visually it would be the same, and it's not a bad thing, I just thought that it would be clearer for the student to separate the notions visually. However, using FieldVariables means that I would need to override the ContextMenu to remove the rename and delete option OR renaming the class from the FieldVariables should rename in the FieldTextInput as well OR I would need to implement the TypedVariable plugin to create a variable manually, typed as a class, and this creates a class definition block which I put my styles in it, and then I can use the FieldVariables, which means that is possible to use a class that has not been defined if we don't use the class block created. I looking the create something as much error proof as possible, so that is not really what I want, but it's not like it would crash or something, so I don't know.

For the event part, I suggested a destroy function similar to the init function, but which would be call when the block is disposed, so in the dispose function, we could check something like if (this.destroy) {this.destroy()}. What do you think of that?

In any case, I would like to thank both of you again, for your help and interest, you made me think differently!

[maribethb]

The approach Beka suggested that allows a block to receive its own delete event seems reasonable. Yes, it would be a breaking change.

I don't think it's likely that we add another function to the block definition API, since there is a simpler way to achieve this (with the event change listeners), but I appreciate you filing this and starting the discussion!

As for priority, since this can be achieved with a workspace change listener, I would normally say it's not that high. But if this is needed for the shared procedures work, and since we are currently doing a major release next, then it may make sense to do this now, but I don't know where we've landed wrt timing on the shared procedures work.

[e-gloo]

Hi @maribethb, thank you for your answer! I understand, and for now I'm using the workspace event listener and it's working. I hope it won't get ugly when I will have many other blocks but for now it's working just fine.

Thank you for your time!

[maribethb]

This was closed by #6337