Adding BlockDefinition and BlockTemplate

[AnmAtAnm]

• BlockDefinition is an in memory representation of the same definitions described by JSON block definitions. Definitions are added to the BlockFactory with addJsonDefinition(..).

• BlockTemplate is a description of a block that is about to be created, and includes a reference to a block definition, mutable state, and children to attach.

• BlockFactory.obtain() replaces BlockFactory.obtainBlock() (deprecated), and takes in a BlockTemplate:

  Block number = factory.obtain(block().ofType("math_number"));
  Block numberShadow = factory.obtain(block().shadow().copyOf(number).withId("numberShadow"));

• Fixes #418, using BlockLoadingException at all stages of block loading, replacing all instances of BlocklyParserException (and BlockLoadingException is no longer an IOException).

• loadWorkspaceContents(), loadToolboxContents(), and loadTrashContents() no longer throw errors, but do return a success/fail boolean. Errors are logged is they occur.

---

This change is 

[rachel-fenichel]

Reviewed 1 of 42 files at r1. Review status: 1 of 42 files reviewed at latest revision, 15 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 32 at r2 (raw file):

/**
 * Definition of a type of block, usually defined in JSON.  See

When would it not be defined in JSON, for the android and iOS cases?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 33 at r2 (raw file):

/**
 * Definition of a type of block, usually defined in JSON.  See
 * <a href="https://developers.google.com/blockly/guides/create-custom-blocks/define-blocks">the guide on block definitions</a>

nit line length

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 57 at r2 (raw file):

     * Initializes the definition from a string of JSON.
     * @param jsonStr The JSON definition as a string.
     * @throws JSONException If JSON is malformed or does not include expected a attributes.

nit: extra "a" before "attributes"

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 66 at r2 (raw file):

     * Initializes the definition from a JSON object.
     * @param json The JSON object with the definition.
     * @throws JSONException If JSON does not include expected a attributes.

nit: extra "a" before "attributes"

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 74 at r2 (raw file):

        String tmpName = mJson.optString("type");
        String warningPrefix = "";
        if (tmpName == null) {

Interesting. Out of curiosity, why not just fail if there was no type name? I think that's what you implemented on the web.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 77 at r2 (raw file):

            // Generate definition name that will be consistent across runs
            int jsonHash = json.toString().hashCode();
            tmpName = "auto-" + Integer.toHexString(jsonHash);

warningPrefix is not being set in this case.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 79 at r2 (raw file):

            tmpName = "auto-" + Integer.toHexString(jsonHash);
        } else if(isValidType(tmpName)) {
            warningPrefix = "Type \"" + tmpName + "\": ";

This warning prefix seems uninformative. How about something slightly more verbose, like "Warning when defining block of type 'blocktype':"?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 82 at r2 (raw file):

        } else {
            String valueQuotedAndEscaped = JSONObject.quote(tmpName);
            throw new IllegalArgumentException("Invalid block type name: " + valueQuotedAndEscaped);

Note that for now the invalid name will always be the empty string, so this message will always be: Invalid block type name: ""

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 110 at r2 (raw file):

     */
    public String getName() {
        return mName;

Why "name" instead of "type"?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 121 at r2 (raw file):

    /**
     * @return A new output connection for a new block of this type.

", or null if this type of block does not have an output connection."

Same for the next two methods.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 145 at r2 (raw file):

    /**
     * @return A new list of {@link Input} objects for a new block of this type, complete with

Why is this reparsed every time?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 169 at r2 (raw file):

// Split on all argument indices of the form "%N" where N is a number from 1 to // the number of args without removing them.

I'm not sure what you mean by "without removing them". Why would you remove indices? Or maybe args?

// Split on all argument indices without removing them. // Argument indices take the form "%N" where N is a number from 1 to the number of args

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 1 of 42 files at r1. Review status: 2 of 42 files reviewed at latest revision, 20 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 55 at r3 (raw file):

    private String mTooltip = null;
    private String mComment = null;
    private boolean mHasContextMenu;

Why does this not get a default value?

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 74 at r3 (raw file):

     * @param id The globally unique identifier for this block.
     * @param isShadow Whether the block should be a shadow block (default input value block).
     * @throws BlockLoadingException When the {@link BlockDefinition} throws errors.

Does this have to note that it throws a NullPointerException?

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 80 at r3 (raw file):

            throws BlockLoadingException {
        if (factory == null || definition == null || id == null) {
            throw new NullPointerException();

Why not a BlockLoadingException? It seems like a NullPointerException has less information.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 719 at r3 (raw file):

    };

    private void rebuildConnectionList() {

Doc

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 744 at r3 (raw file):

    /**
     * Connects to given children, or throws a descriptive BlockLoadingException.

Note that this may connect a block and set the shadow behind that block.

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 3 of 4 files at r3. Review status: 5 of 42 files reviewed at latest revision, 42 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 45 at r2 (raw file):

/**
 * Helper class for building a set of master blocks and then obtaining copies of them for use in

Is this still true?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 60 at r3 (raw file):

     * </pre>
     */
    public static BlockTemplate block() {

This naming is really confusing. The function's name is block() and it's a function on a BlockFactory, so I would expect it to return a Block, not a BlockTemplate.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 118 at r3 (raw file):

    public boolean isBlockIdInUse(String id) {
        WeakReference<Block> priorBlockRef = mBlockRefs.get(id);
        Block priorBlock = priorBlockRef == null ? null : priorBlockRef.get();

Why is this two lines instead of just return priorBlockRef != null

?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 130 at r3 (raw file):

        String definitionName = definition.getName();
        if (mDefinitions.containsKey(definitionName)) {
            throw new IllegalArgumentException("Definition already defined. Must remove first.");

Include the name of the definition in this message instead of adding it in your wrapper below.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 240 at r3 (raw file):

    /**
     * Removes a block type from the factory. If the Block is still in use by the workspace this

"If any block of this type is still in use..."

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 252 at r3 (raw file):

    /**
     * Creates a block of the specified type using one of {@link BlockDefinition}s registered with

one of {@link BlockDefinitions}s --> a {@link BlockDefinition}

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 267 at r3 (raw file):

    public Block obtainBlock(String definitionName, @Nullable String id) {
        // Validate id is available.
        if (isBlockIdInUse(id)) {

Either here or in isBlockIdInUse, explicitly check if id is null.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 296 at r3 (raw file):

        // Existing instance not found. Constructing a new Block.
        BlockDefinition definition;
        boolean isShadow = template.mIsShadow == null ? false : template.mIsShadow;

Why not just require templates to set mIsShadow to either true or false?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 306 at r3 (raw file):

                xml = xml.replace("<block", "<block id=\"" + escapedId + "\"");
                block = BlocklyXmlHelper.loadOneBlockFromXml(xml, this);
            } catch (BlocklySerializerException e) {

Is there an equivalent exception to catch if it failed to deserialize the new block from XML?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 314 at r3 (raw file):

            if (template.mDefinition != null) {
                if (template.mDefinitionName != null
                        && !template.mDefinitionName.equals(template.mDefinition.getName())) {

Again, why is this check not the BlockTemplate's responsibility?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 340 at r3 (raw file):

    /**
     * @return The list of known blocks that can be created.

List of known block types

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 349 at r3 (raw file):

     * Loads and adds block templates from a raw file resource.
     *
     * Block definitions from resources will never be passed to generators

"will never be" sounds like this is intentional. The bug looks like we intend to fix it, just not soon, so say "are not currently"

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 464 at r3 (raw file):

            // If the id was empty the BlockFactory will just generate one.

            // These two are the same object, but the first keeps the XmlBlockTemplate type.

I don't understand what's going on here.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 586 at r3 (raw file):

                eventType = parser.next();
            }
            // Should never reach here, since this is called from a workspace fromXml function.

Is this comment still true?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

            return requested;
        }
        return UUID.randomUUID().toString();  // Assuming no collisions.

You could check that there isn't a collision and do this in a loop until you get something unique (which would be the first time most of the time).

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 677 at r3 (raw file):

         * Generally only used during XML deserialization.
         *
         * This method is package private because the API of this method is subject to change. Do not

Nit: line lengths, in all of this comment.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 701 at r3 (raw file):

            }

            // Find duplicate values.

???

I think this comment is saying the same thing as the comment just after the else, and that one is closer to the code it's describing. Remove this one.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 730 at r3 (raw file):

        /**
         * Sets a block next children immediately after creation.

?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 733 at r3 (raw file):

         * Generally only used during XML deserialization.
         *
         * This method is package private because the API of this method is subject to change. Do not

Nit: line length, here and in the rest of this function.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 129 at r3 (raw file):

    /**
     * Applies the mutable state described by a template. Called after extensions are applied to the

Is there a definition of the term "mutable state" anywhere?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 400 at r3 (raw file):

    /**
     * @return

Missing doc, here and for the next method.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 445 at r3 (raw file):

    }

    private void checkDefinitionAndCopySourceUnset() {

Missing doc.

I would personally use checkDefinitionAndCopySourceNotSet rather than Unset

---

Comments from Reviewable

[rachel-fenichel]

All files reviewed!

---

Reviewed 28 of 42 files at r1, 8 of 11 files at r2, 1 of 4 files at r3. Review status: all files reviewed at latest revision, 45 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 321 at r3 (raw file):

    public boolean loadToolboxContents(int toolboxJsonResId) {
        boolean success = mWorkspace.loadToolboxContents(toolboxJsonResId);
        updateToolbox();

skip updateToolbox() if not successful. Same for the next bunch of functions.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 1919 at r3 (raw file):

                } catch (IOException e) {
                    Log.e(TAG, "Failed to initialize toolbox blocks from assets.", e);
                    toolboxSuccess = false;

toolboxSuccess isn't being used.

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/BlocklyControllerTest.java, line 192 at r3 (raw file):

        Block shadow = mBlockFactory.obtain(
                block().shadow().copyOf(target).withId("connectShadow"));

If target is not a shadow, is the new block's shadow state true or false? That is, does the order of shadow() and copyOf() matter?

---

Comments from Reviewable

[RoboErikG]

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

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 80 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why not a BlockLoadingException? It seems like a NullPointerException has less information.

Neither is going to give any additional information, the main difference is if it's a runtime exception or not. I actually think an IllegalArgumentException might be more appropriate here with the message "Tried to instantiate a block but factory, definition, or id was null.".

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 327 at r3 (raw file):

    public void setPosition(float x, float y) {
        if (Float.isNaN(x) || Float.isInfinite(x) || Float.isNaN(y) || Float.isInfinite(y)) {
            throw new IllegalArgumentException();

Message? "Position must be a real, finite number."

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 503 at r3 (raw file):

                    IOOptions.WRITE_ALL_BLOCKS_WITHOUT_ID);
            return BlocklyXmlHelper.loadOneBlockFromXml(xml, mFactory);
        } catch (BlocklySerializerException | BlockLoadingException e) {

Mutli-catch is only supported on 19+.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 32 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

When would it not be defined in JSON, for the android and iOS cases?

When it's defined in code using BlockDefinition?

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 145 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why is this reparsed every time?

Agreed, but adding an InputDefinition can come in a followup CL.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 169 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

> // Split on all argument indices of the form "%N" where N is a number from 1 to > // the number of args without removing them. I'm not sure what you mean by "without removing them". Why would you remove indices? Or maybe args? // Split on all argument indices without removing them. // Argument indices take the form "%N" where N is a number from 1 to the number of args

String.split usually removes whatever it's splitting on from the results, so it's useful to point out that tokenize splits with capture.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 118 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why is this two lines instead of just return priorBlockRef != null ?

I think you'd want 'return priorBlockRef != null && priorBlockRef.get() != null;'

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 191 at r3 (raw file):

                addDefinition(def);
                blockAddedCount++;
            } else {

This behavior is confusing if the developer is using block libraries. I'd prefer a last one wins with warning so that a library can replace default blocks without having to first remove all the blocks you want to replace.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 467 at r3 (raw file):

            // The other type safe alternative is to override each method used. https://goo.gl/zKbYjo
            final XmlBlockTemplate templateWithChildren = new XmlBlockTemplate();
            final BlockTemplate template = templateWithChildren.ofType(type).withId(id);

Why do you need a BlockTemplate reference? XmlBlockTemplate extends BlockTemplate so it already is a BlockTemplate.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

You could check that there isn't a collision and do this in a loop until you get something unique (which would be the first time most of the time).

I'll buy the first user to find a collision a box of cookies. =)

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 740 at r3 (raw file):

         * @return This block descriptor, for chaining.
         */
        BlockTemplate withNextChild(Block child, Block shadow) {

This can be private now

---

blocklylib-core/src/main/java/com/google/blockly/model/BlocklyCategory.java, line 283 at r3 (raw file):

            }
            return result;
        } catch (IOException | XmlPullParserException e) {

split catch for API < 19

---

blocklylib-core/src/main/java/com/google/blockly/model/WorkspacePoint.java, line 65 at r3 (raw file):

        super(x, y);
        if (Float.isNaN(x) || Float.isInfinite(x) || Float.isNaN(y) || Float.isInfinite(y)) {
            throw new IllegalArgumentException();

Error message "Point must be a real, finite number."

---

blocklylib-core/src/main/java/com/google/blockly/utils/JsonUtils.java, line 25 at r3 (raw file):

 * Static utility methods for working with JSON.
 */
public class JsonUtils {

Is this class ever used?

---

Comments from Reviewable

[AnmAtAnm]

Review status: 23 of 42 files reviewed at latest revision, 54 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 321 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

skip updateToolbox() if not successful. Same for the next bunch of functions.

Will now throw (again) and skip

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 1919 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

toolboxSuccess isn't being used.

Removed.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 55 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why does this not get a default value?

Fixed.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 74 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Does this have to note that it throws a NullPointerException?

The parameters are already declared NonNull. The NPW runtime exception is more of an assert, and is a developer error, as opposed to a file/input problem.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 80 at r3 (raw file):

Previously, RoboErikG wrote…

Neither is going to give any additional information, the main difference is if it's a runtime exception or not. I actually think an IllegalArgumentException might be more appropriate here with the message "Tried to instantiate a block but factory, definition, or id was null.".

Applied Erik's suggestion. It was basically a runtime asserts for the @NonNull attributes.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 327 at r3 (raw file):

Previously, RoboErikG wrote…

Message? "Position must be a real, finite number."

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 503 at r3 (raw file):

Previously, RoboErikG wrote…

Mutli-catch is only supported on 19+.

Tested. Apparently new tools compile this correctly for API 16.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 719 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Doc

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/Block.java, line 744 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Note that this may connect a block *and* set the shadow behind that block.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 32 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

When would it not be defined in JSON, for the android and iOS cases?

I was thinking the BlockDefinition class could be overridden, but that is probably not something we want to support. I've marked BlockDefinition final, and someone will need to edit that source (implicitly invalidating our need to support it) to provide an alternative implementation.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 33 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

nit line length

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 57 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

nit: extra "a" before "attributes"

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 66 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

nit: extra "a" before "attributes"

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 74 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Interesting. Out of curiosity, why not just fail if there was no type name? I think that's what you implemented on the web.

Blockly.defineBlocksWithJsonArray() skips JSON objects without 'type'. The init functions in Blockly.Blocks are closer to BlockDefinition objects.

Having unnamed block definitions probably isn't useful in most normal usage, but can be useful in testing to quickly generate a block with a specific type.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 77 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

warningPrefix is not being set in this case.

It is intentionally left as the empty string. Since the generated typename is not referenceable, I didn't see the need to add it to the prefix.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 79 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This warning prefix seems uninformative. How about something slightly more verbose, like "Warning when defining block of type 'blocktype':"?

The "warning" bit comes from the Log.w(..) level. The prefix is also used in exception messages, which are distinctly not warnings. Renamed the variable as logPrefix.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 82 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Note that for now the invalid name will always be the empty string, so this message will always be: Invalid block type name: ""

Accurate message.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 110 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why "name" instead of "type"?

It is not the type of the the BlockDefinition instance. Renamed it getTypeName() and mTypeName, and also BlockTemplate.mTypeName.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 121 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

", or null if this type of block does not have an output connection." Same for the next two methods.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 145 at r2 (raw file):

Previously, RoboErikG wrote…

Agreed, but adding an InputDefinition can come in a followup CL.

It is not parsed during construction, and I didn't feel the need to create a new class to store the parsed values. Of course, doing so would allow early failures.

Added TODO.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 169 at r2 (raw file):

Previously, RoboErikG wrote…

String.split usually removes whatever it's splitting on from the results, so it's useful to point out that tokenize splits with capture.

reworded.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 45 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Is this still true?

Rewrote with example.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 60 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This naming is really confusing. The function's name is block() and it's a function on a BlockFactory, so I would expect it to return a Block, not a BlockTemplate.

Looked at by itself, yes, it is non-standard. But read it in context, as in the example above.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 118 at r3 (raw file):

Previously, RoboErikG wrote…

I think you'd want 'return priorBlockRef != null && priorBlockRef.get() != null;'

Updated with Erik's form. mBlockRefs might not have WeakReference in its map, so might return null.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 130 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Include the name of the definition in this message instead of adding it in your wrapper below.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 191 at r3 (raw file):

Previously, RoboErikG wrote…

This behavior is confusing if the developer is using block libraries. I'd prefer a last one wins with warning so that a library can replace default blocks without having to first remove all the blocks you want to replace.

Already changed to match new web behavior.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 240 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

"If any block of this type is still in use..."

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 252 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

one of {@link BlockDefinitions}s --> a {@link BlockDefinition}

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 267 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Either here or in isBlockIdInUse, explicitly check if id is null.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 296 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Why not just require templates to set mIsShadow to either true or false?

Because of of the template's role in the following copy: factory.obtain(block.copyOf(otherBlock)). The shadow state is defined by the copy source, not the template.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 306 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Is there an equivalent exception to catch if it failed to deserialize the new block from XML?

BlockLoadingException, which this method already throws. If interested, I can add a catch to add context that this was from a copy.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 314 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Again, why is this check not the BlockTemplate's responsibility?

As written, it is the template's responsibility and is checked via BlockTemplate.checkDefinitionAndCopySourceUnset(). However, if we allow class extensions to BlockTemplate, I'd rather do the check twice.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 340 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

List of known block *types*

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 349 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

"will never be" sounds like this is intentional. The bug looks like we intend to fix it, just not soon, so say "are not currently"

Removed this method. Now, the only way to add definitions from resources is via new BlockFactory(Context, int[]). I updated the equivalent comment there.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 464 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

I don't understand what's going on here.

See erik's comment below.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 467 at r3 (raw file):

Previously, RoboErikG wrote…

Why do you need a BlockTemplate reference? XmlBlockTemplate extends BlockTemplate so it already is a BlockTemplate.

return values in chaining methods. See the linked StackOverflow discussion. There is no clean answer. The best answer is to override each method with the corrected return type.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 586 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Is this comment still true?

Good catch.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

Previously, RoboErikG wrote…

I'll buy the first user to find a collision a box of cookies. =)

Make that two box of cookies (though I did stop and pause when writing this, hence the comment).

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 677 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Nit: line lengths, in all of this comment.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 701 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

??? I think this comment is saying the same thing as the comment just after the else, and that one is closer to the code it's describing. Remove this one.

Removed. I have no idea what that referred to.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 730 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

?

Rewrote.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 733 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Nit: line length, here and in the rest of this function.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 740 at r3 (raw file):

Previously, RoboErikG wrote…

This can be private now

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlocklyCategory.java, line 283 at r3 (raw file):

Previously, RoboErikG wrote…

split catch for API < 19

This style works, now.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 129 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Is there a definition of the term "mutable state" anywhere?

Added here.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 400 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Missing doc, here and for the next method.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 421 at r3 (raw file):

    /** Correctly typed reference to this for return values. */
    @SuppressWarnings("unchecked")
    protected T getThis() {

Removed.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 445 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Missing doc. I would personally use checkDefinitionAndCopySourceNotSet rather than Unset

Renamed and documented.

---

blocklylib-core/src/main/java/com/google/blockly/model/Workspace.java, line 175 at r1 (raw file):

Previously, RoboErikG wrote…

As discussed offline, the boolean approach is okay for these methods.

Reverted to throwing exceptions up until the points where we know the InputStream comes from a compile time resources, mostly in BlocklyActivityHelper. Such methods will throw IllegalStateExceptions and do not need to be caught (will crash the app).

---

blocklylib-core/src/main/java/com/google/blockly/model/WorkspacePoint.java, line 65 at r3 (raw file):

Previously, RoboErikG wrote…

Error message "Point must be a real, finite number."

Done.

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/BlocklyControllerTest.java, line 192 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

If target is not a shadow, is the new block's shadow state true or false? That is, does the order of shadow() and copyOf() matter?

It should not matter. shadow() will set mIsShadow from null (inheriting from copy) to true. Noted in doc comment for shadow().

---

blocklylib-core/src/main/java/com/google/blockly/utils/JsonUtils.java, line 25 at r3 (raw file):

Previously, RoboErikG wrote…

Is this class ever used?

Huh... I guess I removed that code.

---

Comments from Reviewable

[rachel-fenichel]

Reviewed 20 of 20 files at r4. Review status: all files reviewed at latest revision, 21 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/android/AbstractBlocklyActivity.java, line 555 at r4 (raw file):

    /**
     * Reloads the block definitions and toolbox contents specified by

This only reloads the toolbox contents, not the block definitions.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 405 at r4 (raw file):

        // TODO(#58): Save the rest of the state.

        // Success!

Comment is now wrong--success is not guaranteed at this point.

(Failure is always an option!)

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 1911 at r4 (raw file):

        }

        private void loadBlockDefinitionsFromAssets(BlockFactory factory, List<String> assetPaths) {

Doc, here and on loadToolbox.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 77 at r2 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

It is intentionally left as the empty string. Since the generated typename is not referenceable, I didn't see the need to add it to the prefix.

It seems like it still may be useful if you want to distinguish between multiple failing blocks when loading an array. I don't feel strongly about this.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 145 at r2 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

It is not parsed during construction, and I didn't feel the need to create a new class to store the parsed values. Of course, doing so would allow early failures. Added TODO.

I agree with Erik that we don't have to resolve it in this PR.

I wonder how much overhead having stored objects for each block type creates.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 60 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Looked at by itself, yes, it is non-standard. But read it in context, as in the example above.

factory.obtainBlockFrom(template().ofType("math_number")...) would be readable and accurate.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 306 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

BlockLoadingException, which this method already throws. If interested, I can add a catch to add context that this was from a copy.

Nope, it's fine--my eye was just caught by asymmetry.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Make that two box of cookies (though I did stop and pause when writing this, hence the comment).

You are probabilistically correct, the best kind of correct. But it still seems like such an easy solution that we should just implement it. Erik, I believe you have an anecdote about a world-ending button that is relevant here.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 94 at r4 (raw file):

     * (https://github.com/google/blockly-android/issues/525). Instead, use
     * {@link #addJsonDefinitions(String)} to load from assets.
     * @deprecated Call default constructor, and prefer block definitions in assets over resources.

Worth logging a warning when this is used? It would provide a pointer to a developer whose generators don't work.

---

Comments from Reviewable

[RoboErikG]

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

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

You are probabilistically correct, the best kind of correct. But it still seems like such an easy solution that we should just implement it. Erik, I believe you have an anecdote about a world-ending button that is relevant here.

I don't remember which anecdote you're referring to, but if anyone ever uses Blockly to program a doomsday machine I think we have bigger issues. ;)

I think this is a case where even a simple solution isn't worth it since a collision is extremely unlikely and if there ever is one it will corrupt one Blockly program, which may or may not be restorable depending on when they last saved.

---

Comments from Reviewable

[RoboErikG]

Once remaining comments are addressed

---

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 467 at r3 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

return values in chaining methods. See the linked StackOverflow discussion. There is no clean answer. The best answer is to override each method with the corrected return type.

As mentioned, you aren't using chaining with a subclass method in the current code so should be safe to remove.

---

Comments from Reviewable

[AnmAtAnm]

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

blocklylib-core/src/main/java/com/google/blockly/android/AbstractBlocklyActivity.java, line 555 at r4 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This only reloads the toolbox contents, not the block definitions.

Fixed. Also rewrote with better context to the referenced method.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 405 at r4 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Comment is now wrong--success is not guaranteed at this point. (Failure is always an option!)

Removed.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 1911 at r4 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Doc, here and on loadToolbox.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockDefinition.java, line 145 at r2 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

I agree with Erik that we don't have to resolve it in this PR. I wonder how much overhead having stored objects for each block type creates.

I would guess Classes and their methods can be a little heavy, but probably not much worse (and possibly better) than the JSON with all of the individual syntax tree objects.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 60 at r3 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

factory.obtainBlockFrom(template().ofType("math_number")...) would be readable and accurate.

In that case, removing template() in place of new BlockTemplate(). The vast majority of uses of this are in testing, anyway. And as long as you have a properly configured IDE, the keystrokes are most the same, even if the line length is yuge.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 467 at r3 (raw file):

Previously, RoboErikG wrote…

As mentioned, you aren't using chaining with a subclass method in the current code so should be safe to remove.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 649 at r3 (raw file):

Previously, RoboErikG wrote…

I don't remember which anecdote you're referring to, but if anyone ever uses Blockly to program a doomsday machine I think we have bigger issues. ;) I think this is a case where even a simple solution isn't worth it since a collision is extremely unlikely and if there ever is one it will corrupt one Blockly program, which may or may not be restorable depending on when they last saved.

It won't corrupt until the block is constructed and added. I'll add the check. (technically correct > probabilistically correct)

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockFactory.java, line 94 at r4 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Worth logging a warning when this is used? It would provide a pointer to a developer whose generators don't work.

Done.

---

Comments from Reviewable

[rachel-fenichel]

LGTM after the tiniest nits on comments.

---

Reviewed 26 of 26 files at r5. Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/BlocklyController.java, line 1956 at r5 (raw file):

        /**
         * Loads the toolbox from {@link #mToolboxResId}, {@link #mToolboxXml}, or
         * {@link #mToolboxAssetPath}. If loading fails, throw an IllegalStateException since this

nit: doubled "this"

---

blocklylib-core/src/main/java/com/google/blockly/model/BlockTemplate.java, line 29 at r5 (raw file):

/**
 * Template of a block, describing the initial state of a new block. The most methods return the

Nit: "The most methods"?

---

Comments from Reviewable