Scratch-blocks: field_matrix does not use colors of the block containing it

[ErikMejerHansen]

Expected Behavior

field_matrix should follow the colors of the block containing it.

Actual Behavior

It keeps its own greenish color:

I've tried to solve the issue in the PR that references this issue, but my solution feels like a hack. Because the parentBlock property isn't set on field_matix's sourceBlock_ when the affected functions are initially called I've added guards to protect against 'undefined'. This solves the issue, but feels wrong.

In contrast field_angle also references sourceBlock_.parentBlock_ but does not need the guards against parentBlock being undefined. I'm guessing that is because it only references sourceBlock_.parentBlock_ in showEditor_ which is called at a later stage than init and setValue where field_matrix sets its colors.

So perhaps the issue is that setParent should be called earlier? But that change seems like it would be above my paygrade :)

Steps to Reproduce

Add a block containing a field_matrix to an extension and the the extensions colors.
Something along the lines of:

getInfo: function () {
        return {
            blocks: [
                {
                    arguments: {
                        MATRIX: {
                            defaultValue: '0101011111111110111000100', 
                            type: 'matrix'
                        }
                    },
                    blockType: 'command',
                    opcode: 'show',
                    text: 'Show [MATRIX]'
                }
            ],
            colour: '#3D7DD9',
            colourSecondary: '#0D4E8E',
            colourTertiary: '#033D6C',
            id: 'myMatixTest',
            name: 'Matrix test',
            targetTypes: ['hub']
        };
    }

Operating System and Browser

Mac OS 10.13.6 (17G65) Chrome Version 70.0.3538.77 (Official Build) (64-bit)

[picklesrus]

I took a look at #1764 and see what you mean :-/ . We aggressively call updateMatrix_ in places where I didn't think we would need to. I tried removing them and, sure enough, we do not render things quite right... I'm going to ask @ericrosenbaum if he has any thoughts since (I think) he implemented this field.

[picklesrus]

Ahah. I think I understand - we do want to update the view in many of these cases (i.e. call updateMatrix_) but not every time we update the model (i.e. call setValue). setValue will get called when the block is being constructed from XML and not everything needed to update the view will exist by then.

This seems to work for me: https://github.com/picklesrus/scratch-blocks/commit/b91fcfc642f514eafb903cebb06dc4c34f220545

Thoughts?

[ErikMejerHansen]

Way more elegant!
Looks like a call to setShadowColour is needed somewhere to give the shadow block (is that the correct term?) the correct color.

Or is that supposed to happen by some other means?

[picklesrus]

Interesting - I did that part by putting the field inside a different shadow. In other words, I defined a new "matrix" block and set the colors differently... But maybe that doesn't work when constructing this through an extension?

[ErikMejerHansen]

Yeah, looks like you don't need that call if you do something like:

Blockly.Blocks['matrix'] = {
  /**
   * Block for matrix value.
   * @this Blockly.Block
   */
  init: function() {
    this.jsonInit({
      "message0": "%1",
      "args0": [
        {
          "type": "field_matrix",
          "name": "MATRIX"
        }
      ],
      "outputShape": Blockly.OUTPUT_SHAPE_ROUND,
      "output": "Number",
      "extensions": ["colours_pen"]
    });
  }
};

But if you do something like:

const extension = {
    getInfo: function () {
        return {
            blocks: [
                {
                    arguments: {
                        MATRIX: {
                            defaultValue: '0101011111111110111000100', // Heart
                            type: 'matrix'
                        }
                    },
                    blockType: 'command',
                    filter: [],
                    opcode: 'show',
                    text: 'Show [MATRIX]'
                }
            ],
            colour: '#3D7DD9',
            colourSecondary: '#0D4E8E',
            colourTertiary: '#033D6C',
            id: 'myMatixTest',
            name: 'Matrix test',
            targetTypes: []
        };
    },
    show: function (){

    }
};
defaultVM.extensionManager._registerInternalExtension(extension);

it does seem like you need it (I should not be using _registerInternalExtension).

For dropdown menus I think the vm takes care of it in runtime._buildMenuForScratchBlocks colors are set from the catagoryInfo object.

[picklesrus]

That makes sense. Yes, I think you're right about the vm during the conversion to scratch blocks. I think we're going to need something similar to the menu building code you referenced for shadows in extensions. I'm curious to get @cwillisf 's opinion on this though.

@cwillisf: When making extension blocks that have shadows, (e.g. the shadow that contains the matrix field for the microbit extension), should we be generating the shadow's definition in the vm (right now I think we just get it from the blocks) so we can set the color appropriately? Let me know if you need more context - not sure how easy this thread is to follow if you haven't had your head in it already :)

[cwillisf]

@picklesrus I think it would make sense to generate the shadows in the VM. The main obstacle is that I'm not sure how to do that correctly ;)

In general I think it makes sense for the VM (and in particular the extension manager) to be as authoritative as possible when it builds block JSON and XML -- I think that approach will decrease the chances that someone will need to troubleshoot in multiple locations when a block doesn't look right or work correctly.