Skip to content

Cache dynamic dropdown options #3036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
samelhusseini merged 7 commits into from
Sep 18, 2019
Merged

Cache dynamic dropdown options #3036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c404be
Cache dynamic dropdown options and only re-generate on initialization…
samelhusseini Sep 17, 2019
44c5455
Merge branch 'develop' into dynamic_dropdown_cache
samelhusseini Sep 18, 2019
1fa3b06
PR comments
samelhusseini Sep 18, 2019
2f75db6
Other way around
samelhusseini Sep 18, 2019
4b89799
nit
samelhusseini Sep 18, 2019
6da2d4f
PR comments.
samelhusseini Sep 18, 2019
d749a60
Throws in constructor
samelhusseini Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 34 additions & 18 deletions core/field_dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,15 @@ goog.require('Blockly.utils.userAgent');

/**
* Class for an editable dropdown field.
* @param {(!Array.<!Array>|!Function)} menuGenerator An array of options
* for a dropdown list, or a function which generates these options.
* @param {(!Array.<!Array>|!Function)} menuGenerator A non-empty array of
* options for a dropdown list, or a function which generates these options.
* @param {Function=} opt_validator A function that is called to validate
* changes to the field's value. Takes in a language-neutral dropdown
* option & returns a validated language-neutral dropdown option, or null to
* abort the change.
* @extends {Blockly.Field}
* @constructor
* @throws {TypeError} If `menuGenerator` options are incorrectly structured.
*/
Blockly.FieldDropdown = function(menuGenerator, opt_validator) {
if (typeof menuGenerator != 'function') {
Expand All @@ -68,15 +69,22 @@ Blockly.FieldDropdown = function(menuGenerator, opt_validator) {
this.menuGenerator_ = menuGenerator;

/**
* The currently selected index. A value of -1 indicates no option
* has been selected.
* A cache of the most recently generated options.
* @type {Array.<!Array.<string>>}
* @private
*/
this.generatedOptions_ = null;
Copy link
Contributor

@BeksOmega BeksOmega Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you look into having this be a map/dictionary? I think there are some loops that could be elliminated that way.

Copy link
Contributor Author

@samelhusseini samelhusseini Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the key of this map be? If it's the value, it means you can't have multiple options with the same value, label is the same. I don't think we set that restrictions right now.

Copy link
Contributor

@BeksOmega BeksOmega Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the key of this map be?

It would be keyed by value. This is what the dropdown currently uses, it's just inefficient and uses for-loops instead of a map.

We may not currently throw errors, but the language-neutral keys of your dropdown have to be unique, otherwise your dropdown breaks:

SameValues

Steps to reproduce

  1. Add the following block definition to the playground.
{
    "type": "test_dropdowns_same_value",
    "message0": "same value %1",
    "args0": [
      {
        "type": "field_dropdown",
        "name": "FIELDNAME",
        "options": [
          [ "first item", "ITEM1" ],
          [ "second item", "ITEM2" ],
          [ "third item", "ITEM2" ],
          [ "fourth item", "ITEM2" ],
          [ "fifth item", "ITEM2" ],
        ]
      }
    ],
    "style": "fields_dropdowns"
  }
  1. Add the block to the workspace.
  2. Use the dropdown.
  3. Observe the bad behavior.

If we were to switch it to a map we could definitely get rid of the loop in doClassValidation_. We could also probably get rid of the selectedIndex_ property and just use the value instead, elliminating the loop in doValueUpdate_.


/**
* The currently selected index. The field is initialized with the
* first option selected.
* @type {number}
* @private
*/
this.selectedIndex_ = -1;
this.selectedIndex_ = 0;

this.trimOptions_();
var firstTuple = this.getOptions()[0];
var firstTuple = this.getOptions(false)[0];

// Call parent's constructor.
Blockly.FieldDropdown.superClass_.constructor.call(this, firstTuple[1],
Expand Down Expand Up @@ -230,7 +238,8 @@ Blockly.FieldDropdown.prototype.widgetCreate_ = function() {
menu.setRightToLeft(this.sourceBlock_.RTL);
menu.setRole('listbox');

var options = this.getOptions();
var options = this.getOptions(false);
this.selectedMenuItem_ = null;
for (var i = 0; i < options.length; i++) {
var content = options[i][0]; // Human-readable text or image.
var value = options[i][1]; // Language-neutral value.
Expand Down Expand Up @@ -421,15 +430,19 @@ Blockly.FieldDropdown.prototype.isOptionListDynamic = function() {

/**
* Return a list of the options for this dropdown.
* @return {!Array.<!Array>} Array of option tuples:
* @param {boolean=} opt_useCache For dynamic options, whether or not to use the
* cached options or to re-generate them.
* @return {!Array.<!Array>} A non-empty array of option tuples:
* (human-readable text or image, language-neutral name).
* @throws If generated options are incorrectly structured.
* @throws {TypeError} If generated options are incorrectly structured.
*/
Blockly.FieldDropdown.prototype.getOptions = function() {
Blockly.FieldDropdown.prototype.getOptions = function(opt_useCache) {
if (this.isOptionListDynamic()) {
var generatedOptions = this.menuGenerator_.call(this);
Blockly.FieldDropdown.validateOptions_(generatedOptions);
return generatedOptions;
if (!this.generatedOptions_ || !opt_useCache) {
this.generatedOptions_ = this.menuGenerator_.call(this);
Blockly.FieldDropdown.validateOptions_(this.generatedOptions_);
}
return this.generatedOptions_;
}
return /** @type {!Array.<!Array.<string>>} */ (this.menuGenerator_);
};
Expand All @@ -442,7 +455,7 @@ Blockly.FieldDropdown.prototype.getOptions = function() {
*/
Blockly.FieldDropdown.prototype.doClassValidation_ = function(opt_newValue) {
var isValueValid = false;
var options = this.getOptions();
var options = this.getOptions(true);
for (var i = 0, option; option = options[i]; i++) {
// Options are tuples of human-readable text and language-neutral values.
if (option[1] == opt_newValue) {
Expand All @@ -468,7 +481,7 @@ Blockly.FieldDropdown.prototype.doClassValidation_ = function(opt_newValue) {
*/
Blockly.FieldDropdown.prototype.doValueUpdate_ = function(newValue) {
Blockly.FieldDropdown.superClass_.doValueUpdate_.call(this, newValue);
var options = this.getOptions();
var options = this.getOptions(true);
for (var i = 0, option; option = options[i]; i++) {
if (option[1] == this.value_) {
this.selectedIndex_ = i;
Expand Down Expand Up @@ -501,7 +514,7 @@ Blockly.FieldDropdown.prototype.render_ = function() {
this.imageElement_.style.display = 'none';

// Show correct element.
var options = this.getOptions();
var options = this.getOptions(true);
var selectedOption = this.selectedIndex_ >= 0 &&
options[this.selectedIndex_][0];
if (selectedOption && typeof selectedOption == 'object') {
Expand Down Expand Up @@ -576,7 +589,7 @@ Blockly.FieldDropdown.prototype.getText_ = function() {
if (this.selectedIndex_ < 0) {
return null;
}
var options = this.getOptions();
var options = this.getOptions(true);
var selectedOption = options[this.selectedIndex_][0];
if (typeof selectedOption == 'object') {
return selectedOption['alt'];
Expand All @@ -587,13 +600,16 @@ Blockly.FieldDropdown.prototype.getText_ = function() {
/**
* Validates the data structure to be processed as an options list.
* @param {?} options The proposed dropdown options.
* @throws If proposed options are incorrectly structured.
* @throws {TypeError} If proposed options are incorrectly structured.
* @private
*/
Blockly.FieldDropdown.validateOptions_ = function(options) {
if (!Array.isArray(options)) {
throw TypeError('FieldDropdown options must be an array.');
}
if (!options.length) {
throw TypeError('FieldDropdown options must not be an empty array.');
}
var foundError = false;
for (var i = 0; i < options.length; ++i) {
var tuple = options[i];
Expand Down
18 changes: 18 additions & 0 deletions tests/blocks/test_blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,3 +1210,21 @@ Blockly.TestBlocks.removeDynamicDropdownOption_ = function() {
}
})
};

Blockly.Blocks['test_dropdowns_dynamic_random'] = {
Copy link
Contributor

@BeksOmega BeksOmega Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fun hehe.

init: function() {
var dropdown = new Blockly.FieldDropdown(this.dynamicOptions);
this.appendDummyInput()
.appendField('dynamic random')
.appendField(dropdown, 'OPTIONS');
},

dynamicOptions: function() {
var random = Math.floor(Math.random() * 10) + 1;
var options = [];
for (var i = 0; i < random; i++) {
options.push([String(i), String(i)]);
}
return options;
}
};
1 change: 1 addition & 0 deletions tests/playground.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,6 +1468,7 @@ <h1>Blockly Playground</h1>
<block type="test_dropdowns_dynamic"></block>
<button text="Add option" callbackKey="addDynamicOption"></button>
<button text="Remove option" callbackKey="removeDynamicOption"></button>
<block type="test_dropdowns_dynamic_random"></block>
<label text="Other"></label>
<block type="test_dropdowns_long"></block>
<block type="test_dropdowns_images"></block>
Expand Down