From 07d662462585bcb089d4f8b82bfb4d385a0a1bc2 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 12:50:00 -0500 Subject: [PATCH 1/6] Fixing bug where VarCreate event does not fire when adding a block with pre-existing variables from the flyout into the workspace. --- core/flyout_base.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/flyout_base.js b/core/flyout_base.js index 1ec79c31b..7e01a6b10 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -606,6 +606,7 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { Blockly.Flyout.prototype.createBlock = function(originalBlock) { var newBlock = null; Blockly.Events.disable(); + var variablesBeforeCreation = this.workspace_.getAllVariables(); this.targetWorkspace_.setResizesEnabled(false); try { newBlock = this.placeNewBlock_(originalBlock); @@ -615,9 +616,28 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { Blockly.Events.enable(); } + var variablesAfterCreation = this.workspace_.getAllVariables(); + var variablesToFireVarCreate = []; + if (variablesBeforeCreation.length != variablesAfterCreation.length) { + for (var i = 0; i < variablesAfterCreation.length; i++) { + var variable = variablesAfterCreation[i]; + // for any variable that is present in the list of variables + // after creation but is not present in the list of variables before + // creation, add the variable to the list we will traverse to + // fire the VarCreate event + if (!variablesBeforeCreation.includes(variable)) { + variablesToFireVarCreate.push(variable); + } + } + } + if (Blockly.Events.isEnabled()) { Blockly.Events.setGroup(true); Blockly.Events.fire(new Blockly.Events.Create(newBlock)); + for(var i = 0; i < variablesToFireVarCreate.length; i++) { + var thisVariable = variablesToFireVarCreate[i]; + Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); + } } if (this.autoClose) { this.hide(); From 71205de2efa94f996d80adec1c5d3a96365a1ed2 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 13:27:29 -0500 Subject: [PATCH 2/6] Target workspace is the one that carries the variables we want. VarCreate gets fired when a block with a new default variable is dragged out into the workspace. --- core/flyout_base.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 7e01a6b10..84919a7e4 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -606,7 +606,7 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { Blockly.Flyout.prototype.createBlock = function(originalBlock) { var newBlock = null; Blockly.Events.disable(); - var variablesBeforeCreation = this.workspace_.getAllVariables(); + var variablesBeforeCreation = this.targetWorkspace_.getAllVariables(); this.targetWorkspace_.setResizesEnabled(false); try { newBlock = this.placeNewBlock_(originalBlock); @@ -616,7 +616,7 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { Blockly.Events.enable(); } - var variablesAfterCreation = this.workspace_.getAllVariables(); + var variablesAfterCreation = this.targetWorkspace_.getAllVariables(); var variablesToFireVarCreate = []; if (variablesBeforeCreation.length != variablesAfterCreation.length) { for (var i = 0; i < variablesAfterCreation.length; i++) { From c950225897135c0d3e284e493d29baa1737c14c5 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 13:37:30 -0500 Subject: [PATCH 3/6] Minor refactoring --- core/flyout_base.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 84919a7e4..b05b30134 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -617,7 +617,7 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { } var variablesAfterCreation = this.targetWorkspace_.getAllVariables(); - var variablesToFireVarCreate = []; + var newVariables = []; if (variablesBeforeCreation.length != variablesAfterCreation.length) { for (var i = 0; i < variablesAfterCreation.length; i++) { var variable = variablesAfterCreation[i]; @@ -626,7 +626,7 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { // creation, add the variable to the list we will traverse to // fire the VarCreate event if (!variablesBeforeCreation.includes(variable)) { - variablesToFireVarCreate.push(variable); + newVariables.push(variable); } } } @@ -634,8 +634,8 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { if (Blockly.Events.isEnabled()) { Blockly.Events.setGroup(true); Blockly.Events.fire(new Blockly.Events.Create(newBlock)); - for(var i = 0; i < variablesToFireVarCreate.length; i++) { - var thisVariable = variablesToFireVarCreate[i]; + for(var i = 0; i < newVariables.length; i++) { + var thisVariable = newVariables[i]; Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); } } From ee142e92bbf0437eb9a261723cd2b4c1efb1bfad Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 13:43:25 -0500 Subject: [PATCH 4/6] Fixing comment style. --- core/flyout_base.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index b05b30134..fe9a17bcf 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -621,10 +621,10 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { if (variablesBeforeCreation.length != variablesAfterCreation.length) { for (var i = 0; i < variablesAfterCreation.length; i++) { var variable = variablesAfterCreation[i]; - // for any variable that is present in the list of variables + // For any variable that is present in the list of variables // after creation but is not present in the list of variables before // creation, add the variable to the list we will traverse to - // fire the VarCreate event + // fire the VarCreate event. if (!variablesBeforeCreation.includes(variable)) { newVariables.push(variable); } From aae1b1bb74c8dfd658cfce3cfb10c390353390a3 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 14:51:11 -0500 Subject: [PATCH 5/6] Addressing PR comment. Moving functionality to get newly added variables into a helper function. --- core/flyout_base.js | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index fe9a17bcf..305be33e0 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -596,6 +596,33 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { } }; +/** + * Helper function to get the list of variables that have been added to the + * workspace after adding a new block, using the given list of variables that + * were in the workspace before the new block was added. + * @param {!Array.} originalVariables The array of + * variables that existed in the workspace before adding the new block. + * @return {!Array.} The new array of variables that were + * freshly added to the workspace after creating the new block, or [] if no + * new variables were added to the workspace. + * @private + */ +Blockly.Flyout.prototype.getAddedVariables_ = function(originalVariables) { + var allCurrentVariables = this.targetWorkspace_.getAllVariables(); + var addedVariables = []; + if (originalVariables.length != allCurrentVariables.length) { + for (var i = 0; i < allCurrentVariables.length; i++) { + var variable = allCurrentVariables[i]; + // For any variable that is present in allCurrentVariables but not + // present in originalVariables, add the variable to addedVariables. + if (!originalVariables.includes(variable)) { + addedVariables.push(variable); + } + } + } + return addedVariables; +}; + /** * Create a copy of this block on the workspace. * @param {!Blockly.BlockSvg} originalBlock The block to copy from the flyout. @@ -616,24 +643,12 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { Blockly.Events.enable(); } - var variablesAfterCreation = this.targetWorkspace_.getAllVariables(); - var newVariables = []; - if (variablesBeforeCreation.length != variablesAfterCreation.length) { - for (var i = 0; i < variablesAfterCreation.length; i++) { - var variable = variablesAfterCreation[i]; - // For any variable that is present in the list of variables - // after creation but is not present in the list of variables before - // creation, add the variable to the list we will traverse to - // fire the VarCreate event. - if (!variablesBeforeCreation.includes(variable)) { - newVariables.push(variable); - } - } - } + var newVariables = this.getAddedVariables_(variablesBeforeCreation); if (Blockly.Events.isEnabled()) { Blockly.Events.setGroup(true); Blockly.Events.fire(new Blockly.Events.Create(newBlock)); + // Fire a VarCreate event for each (if any) new variable created. for(var i = 0; i < newVariables.length; i++) { var thisVariable = newVariables[i]; Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); From 143d95222c66267f4290ba7395e18225116d2fe5 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 22 Nov 2017 15:22:34 -0500 Subject: [PATCH 6/6] Fixing type annotation. --- core/flyout_base.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 305be33e0..c56ea6253 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -600,9 +600,9 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { * Helper function to get the list of variables that have been added to the * workspace after adding a new block, using the given list of variables that * were in the workspace before the new block was added. - * @param {!Array.} originalVariables The array of + * @param {!Array.} originalVariables The array of * variables that existed in the workspace before adding the new block. - * @return {!Array.} The new array of variables that were + * @return {!Array.} The new array of variables that were * freshly added to the workspace after creating the new block, or [] if no * new variables were added to the workspace. * @private