Merge ProcedureManager mDefinitions and mReferences maps

This is a WIP on functions. I replaced the two maps with one, in an effort to reduce the number of parallel keys I need to maintain. Future step is to do the same with the NameManager, making it a map-like structure to ProcedureBlocks or VariableInfo objects. This also centralizes the canonization of names, and reduces the "surface area" of the name synchronization problem seen during procedure rename.

Also, much more ProcedureManager JavaDocs I missed on the last pass.

This change is

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

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

    private class ProcedureBlocks {
        String mName;
        Block mDefinition;

Should the definition block also be final?

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

     *
     * @param block The block queried.
     * @return The list of argument for defined or referenced by this block, if it is a procedure

@return sentence is confusing.

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

     * Queries whether a procedure block contains a matching registered definition block. Similiar
     * to {@link #isProcedureDefined(String)}, except it takes in a procedure block. If the block is
     * a procedure definition, if ensure the block is the same registered definition block.

s/if/it

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

     *         definition or reference mutator.
     */
    public boolean containsDefinition(Block procedureBlock) {

Why isn't this also named isProcedureDefined?

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

     *         definition or reference mutator.
     */
    public boolean isReferenceRegistered(Block procedureRefBlock) {

This seems to only be used for testing. package scope and @VisibleForTesting?

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

        }
        validateProcedureBlocksMatch(procBlocks.mDefinition, procedureReferenceBlock, false);
        // TODO: validate definition & reference ProcedureInfo match

Do you still need this TODO?

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.

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

Previously, RoboErikG wrote…

Should the definition block also be final?

Hmmm... Makes sense. Done.

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

Previously, RoboErikG wrote…

@return sentence is confusing.

Rewrote.

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

Previously, RoboErikG wrote…

s/if/it

Done.

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

Previously, RoboErikG wrote…

Why isn't this also named isProcedureDefined?

I guess I was thinking this query was asking about this specific block. Did it contain this block? Where the other was asking whether the procedure name was defined by any block. As I describe this, I think I prefer this current name.

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

Previously, RoboErikG wrote…

This seems to only be used for testing. package scope and @VisibleForTesting?

Good catch.

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

Previously, RoboErikG wrote…

Do you still need this TODO?

Removed.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 17 unresolved discussions.

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

Previously, AnmAtAnm (Andrew n marshall) wrote…

Good catch.

Did you need the public scope still?

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

    }

    private class ProcedureBlocks {

remove s -> ProcedureBlock

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

        if (found) {
            if (referenceBlock.getType().equals(CALL_WITH_RETURN_BLOCK_TYPE)) {
                --mCountOfDefinitionsWithReturn;

You removed the increment from addReference. Need to also remove the decrement from here.

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

     */
    public void addDefinition(Block definitionBlock) {
        Log.d(TAG, "addDefinition " + getProcedureName(definitionBlock) + " block "

Nit: remove log

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


        if (!isDefinition(definitionBlock)) {
            throw new IllegalArgumentException("Block is not a procedure definition: " + definitionBlock);

nit: line length

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

        }
        mProcedureNameManager.addName(canonicalProcName);
        mProcedureBlocks.put(canonicalProcName, new ProcedureBlocks(definitionBlock));

Should you check if it has a return and increment mCountOfDefinitionsWithReturn?

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

                mObservers.get(i).onProcedureBlocksRemoved(procBlocks.mName, resultBlocks);
            }
        }

Check if it has a return and decrement mCountOfDefinitionsWithReturn

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

        final ProcedureInfo oldProcInfo = definitionMutator.getProcedureInfo();
        String newProcedureNameRequested = updatedProcedureInfo.getProcedureName();
        Log.d(TAG, "Definition "+Integer.toHexString(definition.hashCode())+"\n\tnewProcedureNameRequested = " + newProcedureNameRequested);

Nit: Logs

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

        Log.d(TAG, "\tisFuncRename = " + isFuncRename);
        final String newProcedureName = !isFuncRename ? originalProcedureName
                : mProcedureNameManager.generateUniqueName(newProcedureNameRequested, true);

Does this allow changing the case? eg procedurename -> procedureName

blocklylib-core/src/main/java/com/google/blockly/android/ui/fieldview/BasicFieldInputView.java, line 48 at r1 (raw file):

        public void afterTextChanged(Editable s) {
            if (mInputField != null) {
                if (mInputField.getBlock() != null) { Log.d(TAG, "Block "+Integer.toHexString(mInputField.getBlock().hashCode()) + ": New input view text: " + s); }

log

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

        ProcedureInfo xmlInfo = ProcedureInfo.parseImpl(parser);
        FieldInput nameField = getNameField();
        Log.d("ProcedureDef", "parseAndValidateMutationXml for block " + Integer.toHexString(mBlock.hashCode()) + ":"

logs

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

                Log.d("ProcedureDef", "Block " + Integer.toHexString(mBlock.hashCode())
                        + ": updateBlock(): field name: " + nameField.getText()
                        + ", new name: " + mProcedureInfo.getProcedureName()

logs

Comments from Reviewable

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

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

Previously, RoboErikG wrote…