Procedure additions/mutations => Variable updates

Refactored variable storage in the WorkspaceStats to use a new VariableInfo object.
Refactored procedure mutators to use a shared ProcedureInfo object for the mutator description.
Procedure blocks additions and changes now automatically create variables. Both of these produce changes on the ProcedureCustomCategory and VariableCustomCategory, respectively.
Fail gracefully on autoload.

This change is

Review status: 0 of 22 files reviewed at latest revision, 9 unresolved discussions, some commit checks broke.

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 322 at r1 (raw file):

        final ProcedureInfo oldProcInfo = definitionMutator.getProcedureInfo();
        final String newProcedureName = updatedProcedureInfo.getProcedureName();
        final boolean isFuncRename = originalProcedureName.equals(newProcedureName);

This needs a !, it's a rename if the names are not equal.

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 355 at r1 (raw file):


                // Mutate each procedure call
                List<Block> procedureCalls = mProcedureReferences.get(originalProcedureName);

in a rename the old procedure name is never removed from mProcedureReferences or mProcedureNameManager. You may want a variation on removeDefinition that takes the procedure name and call it at the end to do cleanup of the old name.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 96 at r1 (raw file):

                    VariableInfoImpl varInfo = mVariableInfoMap.get(i);
                    varInfo.removeProcedure(oldName);
                    varInfo.addProcedure(newName);

Is this adding the procedure to every variable in the workspace? I think you want to remove it from all the variables in the old list and then add it to all the variables in the new list, regardless of the name changing.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 257 at r1 (raw file):

     * Attempts to add a variable to the workspace.
     * @param requestedName The preferred variable name. Usually the user name.
     * @param allowRename Whether the variable name should be rename

"can be renamed"

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 276 at r1 (raw file):

        final String mDisplayName;
        /** FieldVariables that are set to the variable. */
        ArrayList<WeakReference<FieldVariable>> mFields = null;

Why do you keep creating and deleting mFields and mProcedures? Why not just leave them at 0 when there's nothing in them?

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 326 at r1 (raw file):

        }

        boolean removeField(FieldVariable newField) {

s/newField/oldField

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 340 at r1 (raw file):

                if (field == newField) {
                    mFields.remove(i);
                    --count;

nit: --count is unnecessary since you're returning. Fine to leave in for future safety. Missing check for mFields.isEmpty if this is the last variable in the list.

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

        return mArguments;
    }

nit ws

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

        List<String> arguments = mProcedureInfo.getArguments();
        if (!arguments.isEmpty()) {
            sb.append("with:"); // message BKY_PROCEDURES_BEFORE_PARAMS

nit, replace 'with:' with a string resource.

Comments from Reviewable

Just some questions/nits then

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

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 360 at r2 (raw file):

                definitionMutator.mutate(updatedProcedureInfo);
                if (isFuncRename) {
                    mProcedureNameManager.remove(originalProcedureName);

Fine for now, but might want a TODO to call a cleanup method at the end in case more things get added later.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 276 at r1 (raw file):

Previously, RoboErikG wrote…

Why do you keep creating and deleting mFields and mProcedures? Why not just leave them at 0 when there's nothing in them?

Still needs followup.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 90 at r2 (raw file):

        public void onProcedureMutated(ProcedureInfo oldInfo, ProcedureInfo newInfo) {
            String oldName = oldInfo.getProcedureName();
            String newName = newInfo.getProcedureName();

Just to check, does this still work if the list of args is what changed?

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 258 at r2 (raw file):

     * Attempts to add a variable to the workspace.
     * @param requestedName The preferred variable name. Usually the user name.
     * @param allowRename Whether the variable name can be rename.

nit: missing d on renamed.

Comments from Reviewable

Review status: 0 of 23 files reviewed at latest revision, 12 unresolved discussions, some commit checks pending.

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 322 at r1 (raw file):

Previously, RoboErikG wrote…

This needs a !, it's a rename if the names are not equal.

Done.

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 355 at r1 (raw file):

Previously, RoboErikG wrote…

in a rename the old procedure name is never removed from mProcedureReferences or mProcedureNameManager. You may want a variation on removeDefinition that takes the procedure name and call it at the end to do cleanup of the old name.

Done.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 96 at r1 (raw file):

Previously, RoboErikG wrote…

Is this adding the procedure to every variable in the workspace? I think you want to remove it from all the variables in the old list and then add it to all the variables in the new list, regardless of the name changing.

Fixed.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 257 at r1 (raw file):

Previously, RoboErikG wrote…

"can be renamed"

Done.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 276 at r1 (raw file):

Previously, RoboErikG wrote…

Why do you keep creating and deleting mFields and mProcedures? Why not just leave them at 0 when there's nothing in them?

Memory usage. While the lists are tiny, the VariableInfo objects are long lived objects where many/most variable probably don't need both lists, and if they list at least 1, I would not expect them to go down to zero (or vice versa) very often.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 326 at r1 (raw file):

Previously, RoboErikG wrote…

s/newField/oldField

Done.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 340 at r1 (raw file):

Previously, RoboErikG wrote…

nit: --count is unnecessary since you're returning. Fine to leave in for future safety. Missing check for mFields.isEmpty if this is the last variable in the list.

Removed.

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

Previously, RoboErikG wrote…

nit ws

Weird. My editor was configured to remove these.

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

Previously, RoboErikG wrote…

nit, replace 'with:' with a string resource.

Done.

Comments from Reviewable

Review status: 0 of 25 files reviewed at latest revision, 14 unresolved discussions, some commit checks pending.

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

    private final NameManager mProcedureNameManager = new NameManager.ProcedureNameManager();

    // Used determine the visibility of procedures_ifreturn block in the ProcedureCustomCategory.

nit, missing 'to' after 'Used'

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


    /**
     * @return All the registered procedure definition blocks, keyed by

keyed by...?

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 257 at r1 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Done.

Still not done.

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 90 at r2 (raw file):

Previously, RoboErikG wrote…

Just to check, does this still work if the list of args is what changed?

Still needs followup. Does this work correctly if the name doesn't change but the number or names of parameters does?

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

     * Updates the block using the mutation in the XML.
     *
     * @param parser The parser with the {@code <mutation>} element.

Do you need to escape the lt/gt signs?

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

    /**
     * Called when the mutator is attached to a block. It will make sure the procedure name on the
     * block's name field is in synch with the mutator's PRocedureInfo, and register a listener on

nits, usually sync without the h, PRocedureInfo capitlization typo.

Comments from Reviewable

Review status: 0 of 25 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

blocklylib-core/src/main/java/com/google/blockly/android/control/ProcedureManager.java, line 360 at r2 (raw file):

Previously, RoboErikG wrote…