VariableMap and functions added.

[marisaleung]

This CL does the following:

• Replaces VariableList with VariableMap.

• Edits all workspace.js functions that used VariableList to use VariableMap.

• Adds the functions outlined by the Design Doc: createVariable(name, opt_type, opt_id), getVariable(name), getVariableById(id), deleteVariable(name), deleteVariableById(id), renameVariable(oldName, newName), renameVariableById(id, newName), getVariablesOfType(type), getVariableTypes()

• Tests that the functionality of every edited function works the same as it did prior to editing.

• Tests that the additional functions work at outlined by the Design Doc.

• Adjusts any calls made by files outside of workspace that directly or indirectly accessed variableList. The calls are adjusted so that original behavior is preserved.

---

This change is 

[RoboErikG]

Review status: 0 of 11 files reviewed at latest revision, 17 unresolved discussions.

---

core/variables.js, line 91 at r1 (raw file):

                 'Blockly.Variables.allUsedVariables');
  }
  return root.getVariablesOfType('');

allVariables should probably return a list containing every variable in the map. Generators will need this to create the set of initializers for everything. You can do this by iterating through the keys and adding each type's list of variables to the full list before returning it.

---

core/variables.js, line 165 at r1 (raw file):

*/
Blockly.Variables.generateUniqueName = function(workspace) {
  var variableList = workspace.getVariablesOfType('');

allVariables. This name should be unique across everything, not just the empty type.

---

core/workspace.js, line 84 at r1 (raw file):

   * that are not currently in use.
   */
  this.variableMap = {};

Make this private while you're at it. That way we won't have to break devs again if we ever change this data structure in the future.

---

core/workspace.js, line 197 at r1 (raw file):

    Blockly.Events.setGroup(false);
  }
  for (var key of Object.keys(this.variableMap)) {

Why do you need to do this much work? You should be able to just clear the map and let gc clean up the individual lists.

---

core/workspace.js, line 211 at r1 (raw file):

 * @param {boolean} clearList True if the old variable list should be cleared.
 */
Blockly.Workspace.prototype.updateVariableMap = function(clearList) {

Revert method name change. Let's keep as much of the API as we can.

---

core/workspace.js, line 227 at r1 (raw file):

    }
    else {
      varList.push({'name': variable, 'type': null, 'id': null});

Please add a TODO to use variable.type and variable.getId() once variable instances are storing more than just name.

---

core/workspace.js, line 279 at r1 (raw file):

  }
  var type = (variable || newVariable).type;
  for (var i = 0, tempvar; tempvar = this.variableMap[type][i]; i++) {

Instead of iterating through manually if you get the list for just the type of interest and you can use the old code as is.
var variableList = getVariablesOfType(type);

---

core/workspace.js, line 282 at r1 (raw file):

    // Delete variable_model if newName exists and this is the oldName,
    // such that the temp variable is not the oldCase or newName.
    if (variable && tempvar.name == variable.name && newVariable &&

Add a TODO to update all variable references to point to the new id once they're storing more than just name.

---

core/workspace.js, line 302 at r1 (raw file):

 * @param {string} newName New variable name.
 */
Blockly.Workspace.prototype.renameVariableById = function(id, newName) {

renameVariableInstance = function(variable, newName) where variable is an instance of variable including the id, name, and type might make the implementation easier. Fine to use this as a helper if there are places where you have the id without the full variable instance.

---

core/workspace.js, line 321 at r1 (raw file):

Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) {
  if (this.getVariable(name)) {
    return;

You should check that either the type/id aren't set or that they match the existing variable.

---

core/workspace.js, line 365 at r1 (raw file):

/**
 * Delete a variables by the passed in name and all of its uses from this

remove s on 'variables'

---

core/workspace.js, line 419 at r1 (raw file):

 * @private
 */
Blockly.Workspace.prototype.deleteVariableInternal_ = function(name) {

Not in scope for this CL, but in a followup CL you should convert all the internal calls to take a VariableModel or id instead of the name since the name may not correctly identify the variable. Also, if you take the model you only need to check the list with the correct type and can use variableList = getVariablesOfType()

---

core/workspace.js, line 431 at r1 (raw file):

      if (Blockly.Names.equals(variable.name, name)) {
        delete this.variableMap[key][i];
        this.variableMap[key].splice(i, 1);

Once the variable has been found and removed you can exit the loop.

---

core/workspace.js, line 622 at r1 (raw file):

/**
 * Find the variable names with the specified type. If type is null, return list
 *     of all variables.

if type is null, returns all variables with the empty string type.

---

core/workspace.js, line 628 at r1 (raw file):

 */
Blockly.Workspace.prototype.getVariablesOfType = function(type) {
  if (!type) {

Just do type = type || '';

---

core/workspace.js, line 635 at r1 (raw file):

  if (variable_list) {
    for (var variable of variable_list) {
      result_list.push(variable.name);

Prefer you push the VariableModel object. We're going to want to start using it more throughout the code so we can refer to the type and the id.

---

core/workspace.js, line 654 at r1 (raw file):

 */
Blockly.Workspace.prototype.getAllVariables = function() {
  var all_variables = [];

return Blockly.Variables.allVariables(this);

---

Comments from Reviewable

[rachel-fenichel]

Review status: 0 of 11 files reviewed at latest revision, 20 unresolved discussions.

---

core/variables.js, line 225 at r1 (raw file):

                });
          }
          else if (!Blockly.Procedures.isLegalName_(text, workspace)) {

What's going on here? Why do you care about procedures here?

---

core/workspace.js, line 84 at r1 (raw file):

Previously, RoboErikG wrote…

Make this private while you're at it. That way we won't have to break devs again if we ever change this data structure in the future.

+1

---

core/workspace.js, line 131 at r1 (raw file):

  // flyout, so the contents of the dropdown need to be correct.
  var variables = Blockly.Variables.allUsedVariables(block);
  for (var variable of variables) {

for...of is not supported in Internet Explorer: https://docs.microsoft.com/en-us/scripting/javascript/reference/for-dot-dot-dot-of-statement-javascript#requirements

---

core/workspace.js, line 211 at r1 (raw file):

Previously, RoboErikG wrote…

Revert method name change. Let's keep as much of the API as we can.

I'm less concerned about preserving the current API (it's not that old) than I am with making sure that the names match what's actually happening in the future. So I would rather switch to updateVariableMap, or else something generic like updateVariableStore.

If you are updating the name, update the parameter clearList as well.

---

core/workspace.js, line 214 at r1 (raw file):

  // TODO: Sort
  if (this.isFlyout) {
    return

Missing semicolon.

---

Comments from Reviewable

[rachel-fenichel]

Review status: 0 of 11 files reviewed at latest revision, 24 unresolved discussions.

---

tests/jsunit/workspace_test.js, line 27 at r1 (raw file):

var workspace;
var mockControl_;
Blockly.Msg.DELETE_VARIABLE = '%1';

You should change this message in setUp and tearDown, or (even better) only in test cases where it'll be used. Make sure you reset it at the end, and add a comment explaining why you need to define this (which function uses it, and why it's set to this particular value).

---

tests/jsunit/workspace_test.js, line 79 at r1 (raw file):

 */
function checkVariableValues(name, type, id) {
  assertNotUndefined(variable = workspace.getVariable(name));

I prefer to keep assignments out of asserts whenever possible. I would do

var variable = workspace.getVariable(name);
assertNotUndefined(variable);

---

tests/jsunit/workspace_test.js, line 82 at r1 (raw file):

  assertEquals(name, variable.name);
  assertEquals(type, variable.type);
  assertEquals(id, variable.getId());

Why do you access variable.name and variable.type directly, but the id though variable.getId()?

---

tests/jsunit/workspace_test.js, line 145 at r1 (raw file):

    blockB && blockB.dispose();
    blockA && blockA.dispose();
    workspaceTest_tearDown();

You may no longer need to dispose of the blocks manually, as they may be disposed of by workspace dispose. In that case you would also not need to declare them outside of the try.

Same applies to all of these tests.

---

Comments from Reviewable

[RoboErikG]

Review status: 0 of 11 files reviewed at latest revision, 24 unresolved discussions.

---

tests/jsunit/workspace_test.js, line 82 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why do you access variable.name and variable.type directly, but the id though variable.getId()?

It's because the ID is read only, so it has an accessor but no setter.

---

Comments from Reviewable

[marisaleung]

Review status: 0 of 11 files reviewed at latest revision, 24 unresolved discussions.

---

core/variables.js, line 91 at r1 (raw file):

Previously, RoboErikG wrote…

allVariables should probably return a list containing every variable in the map. Generators will need this to create the set of initializers for everything. You can do this by iterating through the keys and adding each type's list of variables to the full list before returning it.

Done.

---

core/variables.js, line 165 at r1 (raw file):

Previously, RoboErikG wrote…

allVariables. This name should be unique across everything, not just the empty type.

Done.

---

core/variables.js, line 225 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

What's going on here? Why do you care about procedures here?

Following Design Doc and checking that name is unique across all variables and procedures. If you want me to move that to a different CL, let me know.

---

core/workspace.js, line 84 at r1 (raw file):

Previously, RoboErikG wrote…

Make this private while you're at it. That way we won't have to break devs again if we ever change this data structure in the future.

Done.

---

core/workspace.js, line 131 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

for...of is not supported in Internet Explorer: https://docs.microsoft.com/en-us/scripting/javascript/reference/for-dot-dot-dot-of-statement-javascript#requirements

Discussed offline to use a format like this:
var keys = Object.keys(variableMap);
for (var i = 0; i < keys.length; i++) {

Changed each for loop to not use for...of

Done.

---

core/workspace.js, line 197 at r1 (raw file):

Previously, RoboErikG wrote…

Why do you need to do this much work? You should be able to just clear the map and let gc clean up the individual lists.

Hm I thought I had been running into a problem in the tests where garbage collection wasn't working. Anywho, this.variableMap_ = {} currently passes all the tests so I'll use that.

---

core/workspace.js, line 211 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

I'm less concerned about preserving the current API (it's not that old) than I am with making sure that the names match what's actually happening in the future. So I would rather switch to updateVariableMap, or else something generic like updateVariableStore.

If you are updating the name, update the parameter clearList as well.

I change it to updateVariableStore and changed clearList to clear. Let me know if you want me to revert or do something else.

---

core/workspace.js, line 214 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Missing semicolon.

Done.

---

core/workspace.js, line 227 at r1 (raw file):

Previously, RoboErikG wrote…

Please add a TODO to use variable.type and variable.getId() once variable instances are storing more than just name.

Done.

---

core/workspace.js, line 279 at r1 (raw file):

Previously, RoboErikG wrote…

Instead of iterating through manually if you get the list for just the type of interest and you can use the old code as is.
var variableList = getVariablesOfType(type);

Yay reusing code! Thank you for the reminder. Done.

---

core/workspace.js, line 282 at r1 (raw file):

Previously, RoboErikG wrote…

Add a TODO to update all variable references to point to the new id once they're storing more than just name.

Done.

---

core/workspace.js, line 302 at r1 (raw file):

Previously, RoboErikG wrote…

renameVariableInstance = function(variable, newName) where variable is an instance of variable including the id, name, and type might make the implementation easier. Fine to use this as a helper if there are places where you have the id without the full variable instance.

Done.

---

core/workspace.js, line 321 at r1 (raw file):

Previously, RoboErikG wrote…

You should check that either the type/id aren't set or that they match the existing variable.

1. If the variable does exist and the type/id isn't set, do I update them?
2. If they don't match the existing variable, do I update them or throw an error?

---

core/workspace.js, line 365 at r1 (raw file):

Previously, RoboErikG wrote…

remove s on 'variables'

Done.

---

core/workspace.js, line 431 at r1 (raw file):

Previously, RoboErikG wrote…

Once the variable has been found and removed you can exit the loop.

Done.

---

core/workspace.js, line 622 at r1 (raw file):

Previously, RoboErikG wrote…

if type is null, returns all variables with the empty string type.

Done. Function Description updated too.

---

core/workspace.js, line 628 at r1 (raw file):

Previously, RoboErikG wrote…

Just do type = type || '';

Done.

---

core/workspace.js, line 635 at r1 (raw file):

Previously, RoboErikG wrote…

Prefer you push the VariableModel object. We're going to want to start using it more throughout the code so we can refer to the type and the id.

Done.

---

core/workspace.js, line 654 at r1 (raw file):

Previously, RoboErikG wrote…

return Blockly.Variables.allVariables(this);

Actually, I have Blockly.Variables.allVariables() call this function instead of the other way around. I think it makes sense to keep the actual searching in workspace.js because variableMap_ is there. What do you think?

---

tests/jsunit/workspace_test.js, line 27 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

You should change this message in setUp and tearDown, or (even better) only in test cases where it'll be used. Make sure you reset it at the end, and add a comment explaining why you need to define this (which function uses it, and why it's set to this particular value).

Done.

---

tests/jsunit/workspace_test.js, line 79 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

I prefer to keep assignments out of asserts whenever possible. I would do

var variable = workspace.getVariable(name);
assertNotUndefined(variable);

Done. And fixed throughout the rest of the tests I created.

---

tests/jsunit/workspace_test.js, line 82 at r1 (raw file):

Previously, RoboErikG wrote…

It's because the ID is read only, so it has an accessor but no setter.

Done.

---

tests/jsunit/workspace_test.js, line 145 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

You may no longer need to dispose of the blocks manually, as they may be disposed of by workspace dispose. In that case you would also not need to declare them outside of the try.

Same applies to all of these tests.

Done.

---

Comments from Reviewable

[marisaleung]

Review status: 0 of 11 files reviewed at latest revision, 24 unresolved discussions.

---

core/workspace.js, line 84 at r1 (raw file):

Previously, marisaleung wrote…

Done.

Done.

---

core/workspace.js, line 419 at r1 (raw file):

Previously, RoboErikG wrote…

Not in scope for this CL, but in a followup CL you should convert all the internal calls to take a VariableModel or id instead of the name since the name may not correctly identify the variable. Also, if you take the model you only need to check the list with the correct type and can use variableList = getVariablesOfType()

Acknowledged!

---

Comments from Reviewable

[marisaleung]

Review status: 0 of 11 files reviewed at latest revision, 22 unresolved discussions.

---

core/workspace.js, line 321 at r1 (raw file):

Previously, marisaleung wrote…

1. If the variable does exist and the type/id isn't set, do I update them?
2. If they don't match the existing variable, do I update them or throw an error?

Offline we decided:
If Type and Id are defined in parameters and if there is a variable that already exists that don't match, throw an error.

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 1 of 11 files at r1, 1 of 4 files at r2.
Review status: 2 of 11 files reviewed at latest revision, 28 unresolved discussions.

---

core/variables.js, line 225 at r1 (raw file):

Previously, marisaleung wrote…

Following Design Doc and checking that name is unique across all variables and procedures. If you want me to move that to a different CL, let me know.

Where is that specified in the design doc? I don't think that's behaviour that we want.

---

core/variables.js, line 89 at r2 (raw file):

    console.warn('Deprecated call to Blockly.Variables.allVariables ' +
                 'with a block instead of a workspace.  You may want ' +
                 'Blockly.Variables.allUsedVariables');

Return early if it's a block, or else get the workspace from the block and update the message to say that you're making a best guess at the workspace.

This was a preexisting bug, but now it'll break instead of returning undefined.

---

core/workspace.js, line 132 at r2 (raw file):

  // flyout, so the contents of the dropdown need to be correct.
  var variables = Blockly.Variables.allUsedVariables(block);
  for (var i = 0, variable; variable = variables[i]; i++) {

Since there's now a distinction between a variable name (a string) and a variable (a variableModel), use "name" instead of "variable" here. That makes the following code

if (!this.getVariable(name)) {
  this.createVariable(name);
}

which reads more cleanly.

---

core/workspace.js, line 211 at r2 (raw file):

    return;
  }
  // Update the list in place so that the flyout's references stay correct.

This comment belongs closer to where the update is happening.

---

core/workspace.js, line 214 at r2 (raw file):

  var allVariables = Blockly.Variables.allUsedVariables(this);
  var varList = [];
  for (var i = 0, variable; variable = allVariables[i]; i++) {

As before, name instead of variable

---

core/workspace.js, line 256 at r2 (raw file):

         '". Both must be the same type.');
  }
  var type = (variable || newVariable || '').type;

What is ''.type?

---

core/workspace.js, line 259 at r2 (raw file):

  var variableList = this.getVariablesOfType(type);
  if (variable) {
    variableIndex = variableList.indexOf(variable);

When would variable be falsy?

---

Comments from Reviewable

[rachel-fenichel]

Review status: 2 of 11 files reviewed at latest revision, 29 unresolved discussions.

---

core/workspace.js, line 289 at r2 (raw file):

    // Only changing case, or renaming to a completely novel name.
    if (oldCase) {
      this.variableMap_[type][newVariableIndex].name = newName;

You can remove this case.

---

Comments from Reviewable

[marisaleung]

Review status: 2 of 11 files reviewed at latest revision, 29 unresolved discussions.

---

core/variables.js, line 225 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Where is that specified in the design doc? I don't think that's behaviour that we want.

https://docs.google.com/document/d/1ZCLus7iBrL7ZMhpmlCdrVDKXEpQtwqWdDjf_Ojm88yE/edit#heading=h.fcgc975dvzod

In the description of this.name

---

core/variables.js, line 89 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Return early if it's a block, or else get the workspace from the block and update the message to say that you're making a best guess at the workspace.

This was a preexisting bug, but now it'll break instead of returning undefined.

Done.

---

core/workspace.js, line 132 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Since there's now a distinction between a variable name (a string) and a variable (a variableModel), use "name" instead of "variable" here. That makes the following code

if (!this.getVariable(name)) {
  this.createVariable(name);
}

which reads more cleanly.

Done.

---

core/workspace.js, line 211 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This comment belongs closer to where the update is happening.

Done.

---

core/workspace.js, line 214 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

As before, name instead of variable

Done.

---

core/workspace.js, line 256 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

What is ''.type?

It is undefined which still works because creating a variable with an undefined type still sets the type to ''. But to make it more clear, I added an if else statement.

---

core/workspace.js, line 259 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

When would variable be falsy?

If we are trying to rename a variable that does not exist.

---

core/workspace.js, line 289 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

You can remove this case.

Done.

---

Comments from Reviewable

[rachel-fenichel]

Review status: 1 of 11 files reviewed at latest revision, 21 unresolved discussions.

---

core/variables.js, line 225 at r1 (raw file):

Previously, marisaleung wrote…

https://docs.google.com/document/d/1ZCLus7iBrL7ZMhpmlCdrVDKXEpQtwqWdDjf_Ojm88yE/edit#heading=h.fcgc975dvzod

In the description of this.name

Interesting. I'd like to discuss that, but I'll move it to the doc. Don't worry about it on this PR.

---

core/variables.js, line 89 at r2 (raw file):

Previously, marisaleung wrote…

Done.

Return an empty array so that you don't break calling code.

---

core/workspace.js, line 85 at r2 (raw file):

   * @private
   */
  this.variableMap_ = {};

Object.create(null)

---

Comments from Reviewable

[marisaleung]

Review status: 1 of 11 files reviewed at latest revision, 21 unresolved discussions.

---

core/variables.js, line 89 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Return an empty array so that you don't break calling code.

Done.

---

core/workspace.js, line 85 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Object.create(null)

Originally I was following javascript style guide rule:
https://engdoc.corp.google.com/eng/doc/devguide/js/styleguide.shtml?cl=head#features-objects-ctor

But I found this thread that OK's the use of Object.create(null). So this shall be done!

Done.

---

Comments from Reviewable

[rachel-fenichel]

---

Reviewed 1 of 2 files at r4.
Review status: 2 of 11 files reviewed at latest revision, 21 unresolved discussions.

---

Comments from Reviewable