Skip to content

VarCreate bugfix - #1477

Merged
rachel-fenichel merged 6 commits into
RaspberryPiFoundation:developfrom
kchadha:varcreate-bugfix
Nov 22, 2017
Merged

VarCreate bugfix#1477
rachel-fenichel merged 6 commits into
RaspberryPiFoundation:developfrom
kchadha:varcreate-bugfix

Conversation

@kchadha

@kchadha kchadha commented Nov 22, 2017

Copy link
Copy Markdown
Contributor

The basics

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

The details

Resolves

Resolves #1395

Proposed Changes

Fixes the bug where VarCreate does not get fired when dragging a block with a variable out into the workspace:

  • check for any new variables when creating a new block from the flyout
  • issue VarCreate event for any new variables (not previously in the workspace)

Reason for Changes

Dragging out a block with a variable into the workspace was not issuing a VarCreate event even though a variable was getting added to the workspace.

Test Coverage

Tried steps to reproduce issue #1395 and saw that the VarCreate event is getting fired once for each new variable being added to the workspace. VarCreate event does not retrigger when dragging blocks referring to existing variables out into the workspace.

Tested on:

  • Desktop Chrome

Additional Information

…th pre-existing variables from the flyout into the workspace.
…eate gets fired when a block with a new default variable is dragged out into the workspace.
@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Comment thread core/flyout_base.js Outdated
Blockly.Events.enable();
}

var variablesAfterCreation = this.targetWorkspace_.getAllVariables();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Make this into a helper function: Blockly.Flyout.prototype.getAddedVariables(variablesBeforeCreation) that returns the list of new variables.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

@kchadha

kchadha commented Nov 22, 2017

Copy link
Copy Markdown
Contributor Author

I signed it!

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

Comment thread core/flyout_base.js Outdated
* 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.<Blockly.VariableModel>} originalVariables The array of

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

!Array.<!Blockly.VariableModel>, here and on the return. And then we're done, and I'll merge it as soon as travis finishes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants