Cache dynamic dropdown options #3036

samelhusseini · 2019-09-17T21:30:15Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

Proposed Changes

Only generate options during inialization (as we need the first tuple) and when a use clicks on the field to open it. (ie: when the editor is shown / widget is created).

Reason for Changes

Be more efficient about calling a dynamic dropdown function and not break in places where we expect certain options to be available.

Test Coverage

Tested the playground, and added a new test dropdown block that returns a random number of items every time the generate method is called.

Tested on:

Desktop Chrome

Documentation

Additional Information

… and render.

samelhusseini · 2019-09-17T21:31:12Z

@BeksOmega FYI.

I'm also a little surprised that we don't support an empty options list, but that can be dealt with in a different change.

BeksOmega

Overall looks good! I'm glad you went back and found this one.

core/field_dropdown.js

BeksOmega · 2019-09-17T23:25:39Z

core/field_dropdown.js

          tuple);
    }
  }
+  if (!options.length) {


Thank you! Uncaught errors are the worst.

BeksOmega · 2019-09-17T23:27:16Z

core/field_dropdown.js

+   * @type {Array.<!Array.<string>>}
+   * @private
+   */
+  this.generatedOptions_ = null;


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

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.

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:

Steps to reproduce

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" }

Add the block to the workspace.

Use the dropdown.

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_.

core/field_dropdown.js

BeksOmega · 2019-09-17T23:31:01Z

tests/blocks/test_blocks.js

  })
 };
+
+Blockly.Blocks['test_dropdowns_dynamic_random'] = {


Looks fun hehe.

BeksOmega · 2019-09-17T23:40:37Z

I'm also a little surprised that we don't support an empty options list, but that can be dealt with in a different change.

Do you have a use case in mind? I think that's it generally good to oppose that behavior so that you avoid dealing with nulls in code generation. I'm having trouble thinking of a situation where the limitation can't be designed around (e.g. variables), but I could be missing something.

RoboErikG · 2019-09-18T00:39:47Z

Do you have a use case in mind? I think that's it generally good to oppose that behavior so that you avoid dealing with nulls in code generation. I'm having trouble thinking of a situation where the limitation can't be designed around (e.g. variables), but I could be missing something.

I mentioned to @samelhusseini that I think our APIs made this easy to do with dynamic dropdown functions. Looking at our existing code, an empty options list is allowed by validateOptions_ but then we assume it's non-empty a few lines later in the constructor. It's probably better if we handle an empty list in one spot instead of expecting every option generator to check for it.

BeksOmega · 2019-09-18T15:18:51Z

It's probably better if we handle an empty list in one spot instead of expecting every option generator to check for it.

I agree that it is a bit annoying to have to design around this limitation (as with most limitations hehe) but I think allowing empty lists and nulls would actually end up creating /more/ work (for us, and outside developers) in the longrun.

Firstly, if outside developers don't handle this at option-generation-time they'll end up having to handle it at code-generation-time. That is a much trickier spot to find bugs. I think we're actually helping them by notifying them before it gets to that point.

Secondly, allowing empty lists and nulls creates a lot more work for us, because now we have to check for that.

We'd have to start checking for empty lists all over the dropdown field, and special case them. For example:

Constructor would have to pass something different to the super.
getText_ would have to return '' or BLANK or some placeholder.
And render_ would have to special-case somehow.

We'd also no longer be able to confidently call field.getValue() in a generic context, because it could return null. Making getValue() non-null saves us a lot of work, because it means you can remove if(field.getValue()) checks.

Two other small points:

That getOptions()[0] line has been around since the dawn of Blockly, and people seem mostly cool with it.
Displaying an empty dropdown is pretty buggy looking behavior anyway. If I were using a block programming language, I'd report that.

In Conclusion:
I think that throwing errors for empty lists is the proper way to handle things.

It saves outside developers the headache in the long run. It makes Blockly less fragile and more elegant. And I can't think of a usecase for empty lists (at least one that doesn't have a better solution).

RoboErikG · 2019-09-18T16:26:50Z

@BeksOmega good points, especially about it never working with an empty list. If @samelhusseini agrees then I'm okay with requiring a non-empty list of options and updating validateOptions_ to explicitly check for an empty list and throw a meaningful error in that case.

samelhusseini · 2019-09-18T17:50:02Z

I agree with the points made here. This PR adds a check in validateOptions and throws if the list returned is empty. I'll add further code comments so that we're more explicit about this.

alschmiedt · 2019-09-18T19:15:08Z

core/field_dropdown.js

+ *     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.


nit: Could we add the type here? @throws {Type}

Cache dynamic dropdown options and only re-generate on initialization…

3c404be

… and render.

samelhusseini mentioned this pull request Sep 17, 2019

During domToWorkspace() child block with FieldDropdown is in workspace's topblock #2926

Closed

BeksOmega reviewed Sep 17, 2019

View reviewed changes

Merge branch 'develop' into dynamic_dropdown_cache

44c5455

samelhusseini added 2 commits September 18, 2019 11:00

PR comments

1fa3b06

Other way around

2f75db6

samelhusseini assigned alschmiedt Sep 18, 2019

nit

4b89799

alschmiedt approved these changes Sep 18, 2019

View reviewed changes

samelhusseini added 2 commits September 18, 2019 12:45

PR comments.

6da2d4f

Throws in constructor

d749a60

samelhusseini merged commit c524980 into RaspberryPiFoundation:develop Sep 18, 2019

samelhusseini deleted the dynamic_dropdown_cache branch September 18, 2019 20:05

BeksOmega mentioned this pull request Sep 18, 2019

Setting the Value of a Dropdown is Inefficient #2493

Open

BeksOmega mentioned this pull request Sep 26, 2019

Dynamic dropdowns are not responding to programmatic setValue calls correctly #3099

Open

alschmiedt mentioned this pull request Jan 7, 2020

Investigate how fields cache options #3553

Closed

Cache dynamic dropdown options #3036

Cache dynamic dropdown options #3036

Uh oh!

Conversation

samelhusseini commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Uh oh!

samelhusseini commented Sep 17, 2019

Uh oh!

BeksOmega left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BeksOmega Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

BeksOmega Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

samelhusseini Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeksOmega Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BeksOmega Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

BeksOmega commented Sep 17, 2019

Uh oh!

RoboErikG commented Sep 18, 2019

Uh oh!

BeksOmega commented Sep 18, 2019

Uh oh!

RoboErikG commented Sep 18, 2019

Uh oh!

samelhusseini commented Sep 18, 2019

Uh oh!

alschmiedt Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

samelhusseini Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samelhusseini commented Sep 17, 2019 •

edited

Loading

samelhusseini Sep 18, 2019 •

edited

Loading