Standardize Field Configuration

[BeksOmega]

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

@RoboErikG mentioned here that he would like to move to a new system for configuring field customizations.

I think this is a good idea, but I have some questions so I thought I'd move it to an issue.

Describe the solution you'd like

I think I'd rather start using an opt_config parameter on the constructor with a map of the configuration options. The keys should be the same as the JSON config which will also let you use the same code for both.

Describe alternatives you've considered

@RoboErikG

Open questions
Should this change only to apply to future fields? Or should the currently available field constructors change to support this?

Should the opt_config accept tooltips and other parameters that apply to all fields?

Should there be a hook so you can change the options at runtime (e.g. setOptions(config))?

Should the currently available hooks (e.g. setSpellcheck) be removed, deprecated, or left?

[RoboErikG]

What makes the most sense to me (and @rachel-fenichel may overrule me):

• Use this pattern for future fields that have non-trivial configuration. We can discuss what counts as non-trivial on a field by field basis.
• For existing fields that have 2+ config parameters adding an opt_config to the constructor makes sense, but is low priority.
• Existing configuration methods should be left. Slightly fewer lines of code in block inits doesn't really justify the churn of asking developers to update.
• I'm slightly against adding setOptions. Configuring fields outside of init adds a lot of complexity. Do you have a good use case in mind?

[rachel-fenichel]

+1 to @RoboErikG

[BeksOmega]

Ok I have a revised proposal! Some things are added and some things are changed, so tell me what you think of it.

Proposal: Field Configuration Paradigm

Field configuration is not consistent. There are four places where field configuration is possible: JSON definition, JavaScript constructor, JavaScript constants, and JavaScript hooks.

Definitions and Constructors

The JSON definition and the JavaScript constructor should be equivalent in terms of function. Anything you can configure with one you should be able to configure with the other. They should both cover all configurations exposed by constants and hooks.

[EDIT] JavaScript definitions should support configuration by taking in an opt_config parameter. This means that the standard editable field constructor should look like:
Field(opt_value, opt_validator, opt_config) or Field(opt_value, opt_config) for non-editable fields. This allows the configuration of a field to be easily extended, without taking in more and more parameters.

Constants

JavaScript constants modify all field instances of a given type and cannot be changed while Blockly is running. They do not need to cover all configurations available in the definition or constructor. They should only be available when there is a logical default value for a configuration, and it would be reasonable for an outside developer to set this property globally.

• A FieldTextInput.SPELLCHECK property would be a good constant because an outside developer could reasonably want TextInput fields to spellcheck by default.
• A FieldLabel.CLASS property would be a bad constant because it doesn't make sense to apply a class globally when we already (mostly) provide css.

Hooks

JavaScript hooks modify individual fields, and can be changed while Blockly is running. They do not need to cover all configurations available in the definition or constructor, and they do not need to cover all constants. They should only be available for configurations that can be changed while Blockly is running without adding a lot of complexity.

• FieldTextInput.setSpellcheck is a good hook because it is a relatively unimportant visual change that doesn't affect the value.
• FieldAngle.setWrap would be a bad hook because the wrap does affect the value.

All hooks should return "this" (the field) to support method chaining.

tl:dr

Constants are globals; hooks work while Blockly's running; defs and constructors cover all constants and hooks + extras.

(Some) Discussion Points

[Definitions and constructors] should both cover all configurations exposed by constants and hooks.

Does anyone forsee problems with this, or have opinions about this?

All hooks should return "this" (the field) to support method chaining.

I think this would be a good addition, and it would work backwards compatibly (I don't think there are any config methods that return). But I know there was disagreement about it on the PR.

[RoboErikG]

Regarding hooks returning 'this' I think we decided against it. Chaining configuration methods is typically used when there's some form of finalization that applies all the changes you've made (like an apply() or a build() method). Because the configuration change would immediately trigger a redraw in most cases I'd rather have it not return this and be clear that each operation is an atomic change.

[BeksOmega]

I agree that not adding the method chaining would make it clearer that each change is atomic, but I'm not sure that that's necessary.

For example jQuery allows method chaining for its configuration methods:

$('#my-div')
  .css('background', 'blue')
  .height(100)
  .fadeIn(200);

Plus Blockly does as well, chaining already exists in block initialization:

this.appendDummyInput()
        .appendField(new Blockly.FieldLabelSerializable("first text"), "TEXT1")
        .appendField(new Blockly.FieldColour("#ff0000"), "COLOUR")
        .appendField(new Blockly.FieldLabelSerializable("second text"), "TEXT2");

and colour field configuration:

var field = new Blockly.FieldColour("#ff0000");
        .setColours(myColourArray, myTitlesArray)
        .setColumns(3);

I think it's definitely more readable, rather than more confusing. But that's just my two cents, I'll drop it now hehe.

I forgot to mention in my previous post that once I get confirmation I'll write up a hit list of things that need changing. So now it has officially been mentioned :D

[rachel-fenichel]

I think most constants should not control the behaviour of all fields of that type. In the spellcheck case, for instance, I can see having some text inputs that need to be spellchecked and others that don't, on the same workspace. Where we do have constants that apply to all fields of a type, like the ones for angle fields, I think it's an accident and/or mistake.

[BeksOmega]

In the spellcheck case, for instance, I can see having some text inputs that need to be spellchecked and others that don't, on the same workspace.

I absolutely agree, and we do currently support setting the spellcheck at the instance level, just not at a field-type level.

I think maybe a better way to phrase what I was suggesting would be to say: "Constants would be a way to change the defaults of a field type. Then those defaults could be overriden on a per-instance basis using the JSON definition or JS constructor".

Let me reframe this with a couple of options.

Defaults (at the field-type level) can be manipulated by:

1. Changing the default value of an instance property. E.g. set Blockly.FieldTextInput.prototype.spellcheck_ = true
2. Changing the value of a constant property. E.g set
   Blockly.FieldTextInput.SPELLCHECK = true

Then there are options for documentation:
A) Don't document how to manipulate the defaults of a field type.
B) Do document how to manipulate the defaults of a field type.

I'm cool with any combination of the options. I'll try to include backwards compat details in my hitlist.