Custom Context Menu on variables_get

[Chris Johnson]

In the past, I've added a custom context menu to variables_get with code similar to the following:

Blockly.Blocks['variables_get'].customContextMenu = function(options) {
  console.log('foo');
};

I recently merged with master in my Blockly fork, and I now see the following error message in the JavaScript console when I visit Variables in the toolbox:

Uncaught Error: Mixin will overwrite block members: ["customContextMenu"]
    at Blockly.BlockSvg.Blockly.Block.mixin (block.js:1063)
    at Blockly.BlockSvg.<anonymous> (extensions.js:75)
    at Object.Blockly.Extensions.apply (extensions.js:144)
    at Blockly.BlockSvg.Blockly.Block.jsonInit (block.js:1037)
    at Blockly.BlockSvg.init (blockly.js:367)
    at Blockly.BlockSvg.Blockly.Block (block.js:153)
    at new Blockly.BlockSvg (block_svg.js:98)
    at Blockly.WorkspaceSvg.newBlock (workspace_svg.js:485)
    at Object.Blockly.Xml.domToBlockHeadless_ (xml.js:522)
    at Object.Blockly.Xml.domToBlock (xml.js:455)

It looks as if the builtin context menu is upset with my custom context menu.

Is there a new recommended way to override the context menu of these builtin blocks? I don't see a way to use the new mixin mechanism, as I'm not the one that defined these blocks in the first place.

Thanks!

- Chris

[Chris Johnson]

I should add that I really just want to tweak the existing context menu. This is the code I had been using:

function contextMenuPlusPlus(options) {
  // add stuff to options
}

var oldContextMenu = Blockly.Blocks['variables_get'].customContextMenu;

Blockly.Blocks['variables_get'].customContextMenu = function(options) {

  oldContextMenu.call(this, options);

  contextMenuPlusPlus.call(this, options);

};

- Chris

[Rachel Fenichel]

Hey Chris,

One option is to redefine the variables_get block--that way you have access to the definition without needing to change core blockly.  You could make your own mixin (based on the old one) that adds your functions, and reference that in the extension field instead.

As long as you load your blocks after the default blocks, redefinition should work.  Of course you could also just use a different name for the block.

Hope this helps,

Rachel

[Chris Johnson]

'Rachel Fenichel' via Blockly sent me the following 7.9K:

> One option is to redefine the variables_get block--that way you have access
> to the definition without needing to change core blockly. You could make
> your own mixin (based on the old one) that adds your functions, and
> reference that in the extension field instead.
>
> As long as you load your blocks after the default blocks, redefinition
> should work. Of course you could also just use a different name for the
> block.

Redefining worked great. I didn't realize I could do that with these blocks.

One thing is that I now get a warning like this:

Block definition #0 in JSON array overwrites prior definition of "variables_get".

I don't like warnings. I'd like to add an option to defineBlocksWithJsonArray to allow redefinitions to be ignored. Would you consider a pull request for this change? Or would I be wasting my time?

Thanks!

- Chris

[Rachel Fenichel]

Hey Chris,

Redefining blocks is possible in the web library because they're just being put directly into a JavaScript array.  I need to check what our behaviour is on Android and iOS before I look at your PR.  I'm not convinced that being able to redefine blocks is a desirable behaviour in the general case.

If you already have users who have saved workspaces, and those saved workspaces have 'variables_get' blocks in them, redefining is a way to make sure those workspaces can still be loaded.  

If you're not worried about loading old workspaces, or if you're able to trivially update all old workspaces to use a new name for 'variables_get' blocks, you should give your block a different type instead.  That will avoid the warnings without suppressing a whole class of warnings.

Cheers,

Rachel

[Chris Johnson]

'Rachel Fenichel' via Blockly sent me the following 5.3K:

> If you're not worried about loading old workspaces, or if you're able to
> trivially update all old workspaces to use a new name for 'variables_get'
> blocks, you should give your block a different type instead. That will
> avoid the warnings without suppressing a whole class of warnings.

Blockly.Blocks['variables_get'] and Blockly.Blocks['variables_set'] appear to be hardcoded in various places in core. I'm concerned that defining new names will force customizers like me to drift increasingly away from the master branch, which is really no fun. Variables and procedures are the only blocks for which I must rely considerably on builtin behavior. From what I can tell, you folks have wrapped them up in magic. But good magic.

Am I misreading the source? Maybe swapping in new getters and setters requires fewer changes than I think?

Thanks for your help on this.

- Chris

[Rachel Fenichel]

Good point--renaming variables would be especially problematic.

[Chris Johnson]

For the time being, I'm going to go with the approach I suggested in https://groups.google.com/forum/#!topic/blockly/yUBEymLKBbk. I hijack the builtin blocks' init functions and follow them up with my own code to tailor the context menu, hue, etc.

function customizeBlock(id, hue) {
  var oldInitializer = Blockly.Blocks[id].init;
  Blockly.Blocks[id].init = function() {
    oldInitializer.call(this);
    this.setColour(hue);

    var oldContextMenu = this.customContextMenu;
    this.customContextMenu = function(options) {
      oldContextMenu.call(this, options);
      contextMenuPlusPlus.call(this, options);
    };

    // wrap domToMutation, mutationToDom, etc.
  }
}

This works. If it's a really bad idea, I'd welcome your input!

- Chris

[Cory Diers]

Hi Chris,

Rachel and I were talking about this issue yesterday, and I think there's a "more correct" answer here. Like Rachel mentioned, redefining blocks at runtime is somewhat problematic (What do you do if you redefine a block in the Toolbox, but there are already blocks in the workspace?) However, I don't think you need to. I think you should just change the definition of your block. The "default" blocks you'll find in the blocks folder are not required, they're just defaults. If you want custom behavior on your variables block, you should swap out the block definition. There is one quirk about variables - the only blocks that will go into the variable category are "variables_get", "variables_set", and "variables_change". So, if you're using the block definition files directly, you should replace the definition of "variables_get" in variables.js. It is important (for now) that it has that name, but the definition can change easily, as long as it still has a variable field. If you're trying to avoid merge conflicts, I might suggest to define "custom_variables.js" as a copy of variables.js and include that one in your project (or in your build file, if you need blocks_compressed.)

You're right - variables_ get, set, and change are hard-coded several places in the code base, and I'm increasingly feeling like we need to add a mechanism for changing that. But I think the "right" way to deal with this is to change the definition of the variables_get block at the JSON level. That will remove the error, and while there may be a merge conflict down the road, we strongly encourage custom block definitions for basically every application, so it would be the place they are the most appropriate.