BlockFactory: Adding JSON/JavaScript import support

[AnmAtAnm]

Updates the BlockFactory main workspace based on the preview block generated from manual construction.

This is a continuation of PR #1216 by @JC-Orozco, rebased on latest develop branch. Bring it latest code up to Google styling and standards.

---

This change is 

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[JC-Orozco]

In the "Block Definition:" dropdown selector the manual mode is incorrectly labeled as "Manual JSON...". This could either be Manual JavaScript or Manual JSON depending if the previous mode was JSON or JavaScript

[JC-Orozco]

demos/blockfactory/factory.js, line 140 at r3 (raw file):

Previously, JC-Orozco (Juan Carlos Orozco) wrote…

I can shed some light into the reason for updateBlocksFlag and updateBlocksFlagDelay. Beware that Andrew changed the original workflow and reintroduced the Block Definition: manual edit mode. In this explanation I will assume my original version that allowed direct edit on Block Definition: either JSON mode or JavaScript mode. I think we still need this flags to break the round trip conversion loop even with the manual mode reintroduced.
First a conceptual explanation. (Substitute text code for JSON code or JavaScript code depending on the current mode).
The original flow of the Block Factory editor is as follows (triggered by a change in the mainWorkspace):
Block definition in blockly (mainWorkspace) -> text code (JSON or JavaScript) -> block preview (previewWorkspace)
My program adds a converter that takes a preview block and generates a Block definition DOM data structure. So a new flow is introduced as follows (triggered by a change in the code text editor that as per the previous flow generates a preview block):
block preview (previewWorkspace) -> Block definition in blockly (mainWorkspace)
The problem is that since this last flow changes the mainWorkspace the original flow described above gets triggered and an round trip loop was generated that had the effect of deleting the latest code editor change.
My solution to avoid this round loop is introduce this flags updateBlocksFlag and updateBlocksFalgDelayed. I tried first with a single flag but I needed to be sure that two events occurred before clearing the flag. The preview block is generated and the new Block definition in mainWorkspace is generated. So I created two flags, but for some reason I keep getting a complete loop triggered, that is why I introduced a delayed flag which finally solved the round trip problem. I did not have time to learn with much detail the complete flow of the Block Factory application so once I found out that a state flag and a delayed flag solved this problem I moved on. I did think that a delayed flag could be a good idea to avoid too many code to block definition translations which may be hard on the processing power of the computer so a delay may help pace this translation which is generated on each keystroke. But that was just a conceptual idea I did not check if this particular delayed flag improved on that problem.

While revisiting this issue, I came up with a theory of why I was not able to break this loop with a single flag. I think the mainWorkspace blocks to code conversion is triggered more than once while doing the preview block to block definition (mainWorkspace DOM) conversion, possibly each time a new node is created on the mainWorkspace DOM or for some other reason, but the posible fact that multiple events are being triggered for a single code to block definition conversion could be the reason the only way I was able to break the loop is with a delayed flag. If this is the case a simple solution would be to use a single state flag again and disable mainWorkspace blocks to code conversion until the complete DOM of the preview to block definition process is generated. Thus avoiding the multiple triggering blocks to code conversion. This could also result in an optimized performance by avoiding unnecessary intermediate blocks to code conversions.

---

Comments from Reviewable

[AnmAtAnm]

@JC-Orozco The drop-down now selects either JSON or JavaScript, not both. There is a separate "Manual JavaScript" mode that has been disabled in anything file local file:// hosting, because of the security issue surrounding the eval() statement.

---

Review status: 2 of 6 files reviewed at latest revision, 18 unresolved discussions.

---

Comments from Reviewable

[AnmAtAnm]

Review status: 2 of 6 files reviewed at latest revision, 18 unresolved discussions.

---

demos/blockfactory/factory.js, line 140 at r3 (raw file):

Previously, JC-Orozco (Juan Carlos Orozco) wrote…

While revisiting this issue, I came up with a theory of why I was not able to break this loop with a single flag. I think the mainWorkspace blocks to code conversion is triggered more than once while doing the preview block to block definition (mainWorkspace DOM) conversion, possibly each time a new node is created on the mainWorkspace DOM or for some other reason, but the posible fact that multiple events are being triggered for a single code to block definition conversion could be the reason the only way I was able to break the loop is with a delayed flag. If this is the case a simple solution would be to use a single state flag again and disable mainWorkspace blocks to code conversion until the complete DOM of the preview to block definition process is generated. Thus avoiding the multiple triggering blocks to code conversion. This could also result in an optimized performance by avoiding unnecessary intermediate blocks to code conversions.

@JC-Orozco Thanks for the info. I think we are going to accept this as is in the sort term. I will revisit with your notes in mind at some future time. (As I rush off to copy these notes into the attached issue.)

---

Comments from Reviewable

[JC-Orozco]

demos/blockfactory/block_definition_extractor.js, line 38 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Done.

attrs is replaced by opt_attrs on this definition but not on the second and third use of this parameter in line 93 and 94

---

Comments from Reviewable

[AnmAtAnm]

Review status: 2 of 6 files reviewed at latest revision, 18 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 38 at r3 (raw file):

Previously, JC-Orozco (Juan Carlos Orozco) wrote…

attrs is replaced by opt_attrs on this definition but not on the second and third use of this parameter in line 93 and 94

Whoops. Thanks, JC!

---

Comments from Reviewable

[JC-Orozco]

OK Andrew sounds reasonable, I just saw also a related comment by Rachel. There is then a bug in the manual JSON mode: If you click JavaScript mode first and then Manual JSON... mode the editor will show JavaScript instead of JSON. It is however correctly trying to interpret it as JSON when one modifies the code text.

---

Comments from Reviewable

[JC-Orozco]

I just read your issue 1268: #1268
That will solve this problem and I agree it is an improvement on the UI. You could even clear and gray out the "Manual Edit" checkbox or toggle button on the JavaScript mode and add a small question mark popup message to explain why it is grayed out until the new safe javascript eval is in place. Just an idea.

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 1 of 1 files at r5.
Review status: 2 of 6 files reviewed at latest revision, 23 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 7 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Rewrote.

Move the new fileoverview into here and remove the fileoverview tag + text here.

---

demos/blockfactory/block_definition_extractor.js, line 42 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Done.

I'm acking comments that got moved away from their relevant code. Reviewable didn't handle reordering well this time.

---

demos/blockfactory/block_definition_extractor.js, line 52 at r5 (raw file):

BlockDefinitionExtractor.buildBlockFactoryWorkspace = function(block) {
  var workspaceXml = goog.dom.createDom('xml');
  var inline = 'AUTO'; // When block.inputsInlineDefault === undefined

This chunk for figuring out if it's inline or external should be a helper function. It also doesn't need to be happening here instead of in factoryBase_, where all of the other parsing is happening.

---

demos/blockfactory/block_definition_extractor.js, line 58 at r5 (raw file):

    inline = 'EXT';
  }
  var connections = 'NONE';

Same comment for this part, figuring out connection types.

---

demos/blockfactory/block_definition_extractor.js, line 87 at r5 (raw file):

 * @param {Map<String,String>} opt_attrs Optional list of attributes.
 * @param {string?} opt_text Optional inner text.
 * @return {Element} The newly created element.

@Private

---

demos/blockfactory/block_definition_extractor.js, line 170 at r5 (raw file):

    var field = fieldRow[i];
    var fieldDefElement = null;
    if (field instanceof Blockly.FieldLabel) {

Make this if-elseif-elseif chain into a separate function that returns the element or null, so that the logic in this parseFields_ function is easier to see.

---

demos/blockfactory/block_definition_extractor.js, line 191 at r5 (raw file):

          field.src_, field.width_, field.height_, field.text_);
    } else if (field instanceof Blockly.FieldVariable) {
      // FieldVariable must be before FieldDropdown.

Is this because FieldVariable extends FieldDropdown? If so, add that to the comment.

---

demos/blockfactory/block_definition_extractor.js, line 197 at r5 (raw file):

      fieldDefElement = BlockDefinitionExtractor.fieldDropdown_(field);
    }

What if fieldDefElement is null here?

---

demos/blockfactory/block_definition_extractor.js, line 226 at r5 (raw file):

    var input = block.inputList[i];
    var align = 'LEFT'; // Left alignment is the default.
    if (input.align || input.align === 0) {

Why wrap in this if? It seems like it's just checking the same things as the two internal if statements, and there's no matching else.

---

demos/blockfactory/block_definition_extractor.js, line 251 at r5 (raw file):

 *
 * @param {Blockly.Block} block The example block for the extracted style.
 * @param {string} connections Define block connections. Options: NONE, LEFT,

"Define block connections" -> "A string describing which connections the block has. Options are: ..."

---

demos/blockfactory/block_definition_extractor.js, line 288 at r5 (raw file):

  factoryBaseEl.append(helpUrlValue);

  if (connections === 'LEFT') {

This should go in the helper that figures out which connections are allowed.

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 3 of 4 files at r4, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

demos/blockfactory/factory_utils.js, line 91 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Done.

I don't see it yet.

---

Comments from Reviewable

[AnmAtAnm]

Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 7 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Move the new fileoverview into here and remove the fileoverview tag + text here.

Done.

---

demos/blockfactory/block_definition_extractor.js, line 52 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This chunk for figuring out if it's inline or external should be a helper function. It also doesn't need to be happening here instead of in factoryBase_, where all of the other parsing is happening.

Done.

---

demos/blockfactory/block_definition_extractor.js, line 58 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Same comment for this part, figuring out connection types.

Done.

---

demos/blockfactory/block_definition_extractor.js, line 87 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

@Private

Done.

---

demos/blockfactory/block_definition_extractor.js, line 170 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Make this if-elseif-elseif chain into a separate function that returns the element or null, so that the logic in this parseFields_ function is easier to see.

Done.

---

demos/blockfactory/block_definition_extractor.js, line 191 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Is this because FieldVariable extends FieldDropdown? If so, add that to the comment.

Done.

---

demos/blockfactory/block_definition_extractor.js, line 197 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

What if fieldDefElement is null here?

Shouldn't be possible since we've only loaded Blockly defined classes, so I'm now throwing an error in buildFieldElement_(..).

---

demos/blockfactory/block_definition_extractor.js, line 226 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why wrap in this if? It seems like it's just checking the same things as the two internal if statements, and there's no matching else.

@JC-Orozco What was the intention behind this code?

---

demos/blockfactory/block_definition_extractor.js, line 251 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

"Define block connections" -> "A string describing which connections the block has. Options are: ..."

Removed connections and inline.

---

demos/blockfactory/block_definition_extractor.js, line 288 at r5 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This should go in the helper that figures out which connections are allowed.

Done.

---

demos/blockfactory/factory_utils.js, line 91 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

I don't see it yet.

Done for realz.

---

Comments from Reviewable

[AnmAtAnm]

Review status: 3 of 6 files reviewed at latest revision, 14 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 271 at r7 (raw file):

    var input = block.inputList[i];
    var align = 'LEFT'; // Left alignment is the default.
    if (input.align || input.align === 0) {

@rachel-fenichel @JC-Orozco This is where that code moved to. (Sorry, I shouldn't have moved it before this was resolved.)

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 401 at r7 (raw file):

 * @private
 */
BlockDefinitionExtractor.buildFieldLabel = function(text) {

All of these private functions lost their underscores. Please put them back.

---

Comments from Reviewable

[JC-Orozco]

demos/blockfactory/block_definition_extractor.js, line 226 at r5 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

@JC-Orozco What was the intention behind this code?

If connection.check_ is undefined then add a Shadow block with null type. There may be more elegant ways to write this, I am avoiding the "can not read length of undefined" error. I do not remember if there are cases when connection.check_ can be undefined because sometimes I write code like this just in case (sort of defensive programming) but maybe I did it because there are cases were connection.check_ can be undefined.

---

Comments from Reviewable

[JC-Orozco]

demos/blockfactory/block_definition_extractor.js, line 226 at r5 (raw file):

Previously, JC-Orozco (Juan Carlos Orozco) wrote…

If connection.check_ is undefined then add a Shadow block with null type. There may be more elegant ways to write this, I am avoiding the "can not read length of undefined" error. I do not remember if there are cases when connection.check_ can be undefined because sometimes I write code like this just in case (sort of defensive programming) but maybe I did it because there are cases were connection.check_ can be undefined.

@rachel-fenichel There is a matching else.

---

Comments from Reviewable

[rachel-fenichel]

Review status: 5 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

demos/blockfactory/block_definition_extractor.js, line 226 at r5 (raw file):

Previously, JC-Orozco (Juan Carlos Orozco) wrote…

@rachel-fenichel There is a matching else.

@JC-Orozco the question was about a different if/else, which is now in parseInputs. Reviewable doesn't handle moving chunks of code around mid-review. Anyway, it looks like Andrew resolved it.

---

Comments from Reviewable

[rachel-fenichel]

ship it!

---

Review status: 5 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[AnmAtAnm]

Review status: 5 of 6 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

demos/blockfactory/block_definition_extractor.js, line 401 at r7 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

All of these private functions lost their underscores. Please put them back.

Whoops. Bad search & replace.

---

Comments from Reviewable