Assign variable UUID to field_variable dropdown. by marisaleung · Pull Request #1096 · RaspberryPiFoundation/blockly

marisaleung · 2017-05-10T20:41:28Z

Added static ID's for "Rename variable" and "Delete variable".
Set the internal representation of a variable to its UUID
Adjusted "FieldVariable.prototype.onItemSelected()" to use the UUID to extract the variable name.

This change is

rachel-fenichel · 2017-05-10T20:54:11Z

core/field_variable.js

 goog.require('goog.asserts');
 goog.require('goog.string');

+var RENAME_VARIABLE_ID = 'RENAME_VARIABLE';


https://media.giphy.com/media/l0IyajNbwh0naI9Jm/giphy.gif

More seriously, these should probably be constants in Blockly.Constants, and they should definitely have 'ID' in the text.

For reference, this is my fault and you should listen to Rachel.

Hahahaha I love this X).

Done (listened to Rachel.)

rachel-fenichel · 2017-05-10T20:57:50Z

core/field_variable.js

@@ -177,10 +164,14 @@ Blockly.FieldVariable.dropdownCreate = function() {
 */
 Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) {
  var itemText = menuItem.getValue();


itemText feels overloaded now. On line 166 (on the right) it's the ID, not the text. But on line 172 it's the text.

Agreed. I'm setting it to id for now.
In the next CL I will change field_variable's setValue() to take in an id since all items are now ids. Then we can get rid of itemText.

rachel-fenichel

A few small changes but generally looks good.

rachel-fenichel · 2017-05-11T00:31:32Z

core/constants.js

+ * variable...' and if selected, should trigger the prompt to rename a variable.
+ * @const {string}
+ */
+Blockly.RENAME_VARIABLE_ID = 'RENAME_VARIABLE';


Make this 'RENAME_VARIABLE_ID', and 'DELETE_VARIABLE_ID'

rachel-fenichel · 2017-05-11T00:38:48Z

core/variable_model.js

+ * @param {Blockly.VariableModel} var1 First variable to compare.
+ * @param {Blockly.VariableModel} var2 Second variable to compare.
+ */
+Blockly.VariableModel.prototype.compareByName = function(var1, var2) {


You can put this on Blockly.VariableModel instead of Blockly.VariableModel.prototype.

rachel-fenichel · 2017-05-11T00:39:23Z

core/field_variable.js

    }
-
-    // Call any validation function, and allow it to override.
-    itemText = this.callValidator(itemText);


Did you decide that validators don't make sense in the context of a variable dropdown?

Yes, the call does nothing. And the text comes directly from the variable itself so I can't imagine an instance in which a validator would be needed/useful.

RoboErikG · 2017-05-11T21:48:51Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

core/variable_model.js, line 84 at r2 (raw file):

 */
Blockly.VariableModel.compareByName = function(var1, var2) {
  if (var1.name < var2.name) {

This lost the case insensitive part. Check with Rachel if you're safe to use goog.string.caseInsensitiveCompare(var1.name, var2.name) and if not first convert both names to lowercase before comparing.

Comments from Reviewable

marisaleung · 2017-05-11T23:24:00Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

core/variable_model.js, line 84 at r2 (raw file):

Previously, RoboErikG wrote…

This lost the case insensitive part. Check with Rachel if you're safe to use goog.string.caseInsensitiveCompare(var1.name, var2.name) and if not first convert both names to lowercase before comparing.

Done.

Comments from Reviewable

RoboErikG · 2017-05-12T23:40:17Z

if Rachel's happy.

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

rachel-fenichel · 2017-05-15T17:29:12Z

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

core/field_variable.js, line 135 at r3 (raw file):

    var variableModelList = workspace.getVariablesOfType('');
    for (var i = 0; i < variableModelList.length; i++){
      if (createSelectedVariable && variableModelList[i].name == name) {

This comparison should be case-insensitive.

Comments from Reviewable

marisaleung · 2017-05-15T17:47:49Z

From Rachel:

marisaleung · 2017-05-15T17:50:02Z

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

core/field_variable.js, line 135 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This comparison should be case-insensitive.

Done.

Comments from Reviewable

marisaleung requested review from RoboErikG and rachel-fenichel May 10, 2017 20:41

rachel-fenichel reviewed May 10, 2017

View reviewed changes

marisaleung force-pushed the develop branch 3 times, most recently from d984d6b to b438c69 Compare May 11, 2017 00:29

rachel-fenichel reviewed May 11, 2017

View reviewed changes

marisaleung force-pushed the develop branch 3 times, most recently from 4d4109e to b7932f9 Compare May 11, 2017 18:02

marisaleung force-pushed the develop branch from b7932f9 to 4d8ecfe Compare May 11, 2017 23:23

Assign variable UUID to field_variable dropdown.

6492f29

marisaleung force-pushed the develop branch from 4d8ecfe to 6492f29 Compare May 15, 2017 17:44

marisaleung merged commit 8d5edb3 into RaspberryPiFoundation:develop May 15, 2017

Conversation

marisaleung commented May 10, 2017 • edited by RoboErikG Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoboErikG commented May 11, 2017

Uh oh!

marisaleung commented May 11, 2017

Uh oh!

RoboErikG commented May 12, 2017

Uh oh!

rachel-fenichel commented May 15, 2017

Uh oh!

marisaleung commented May 15, 2017

Uh oh!

marisaleung commented May 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marisaleung commented May 10, 2017 •

edited by RoboErikG

Loading