google / blockly-android

Blockly for Android
Apache License 2.0
673 stars 209 forks source link

Function/Procedure blocks (WIP) #595

Closed AnmAtAnm closed 7 years ago

AnmAtAnm commented 7 years ago

Adds the block definitions and supporting Mutators for the function blocks. This does not include MutatorUIs / Fragments, nor does it update the ProcedureManager when the blocks are added to the workspace. Hence, the toolbox category is commented out in this PR.

Also, new convention for Mutator.Factorys is to create a singleton static instance, rather than a nested class that needs to be instantiated. These stateless classes have no variables; no other instances will be pull into the static allocation. If the Mutator class is referenced (loading the static Factory instance), there is a good chance it is intentional with intent to instantiate the Mutator later in the application's lifecycle.


This change is Reviewable

RoboErikG commented 7 years ago

Minor nits, then :lgtm_strong:


Review status: 0 of 31 files reviewed at latest revision, 5 unresolved discussions.


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

         * Warning: It is possible this toolbox will be loaded during {@link Builder#build()},
         * before the application has a chance to register any {@link Mutator}s or
         * {@link BlockExtension}s. Make sure, if use this convenience {@code Builder} method, the

Needs some grammar cleanup. "If using this {@code Builder} method, make sure the referenced blocks..."


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

    //       since this is called from so many public methods.
    private static String getProcedureName(Block block) {
        Field nameField = block.getFieldByName("name");

s/"name"/"NAME"/ ?


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

    protected List<String> mArguments;

    protected AbstractProcedureMutator(Mutator.Factory factory, BlocklyController controller) {

Should mController be set here instead of in onAttached?


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


    public ProcedureDefinitionNoReturnMutator(Mutator.Factory factory, BlocklyController controller) {

nit line length


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


                @Override
                public ProcedureDefinitionWithReturnMutator newMutator(BlocklyController controller) {

nit line length


Comments from Reviewable

AnmAtAnm commented 7 years ago

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


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

Previously, RoboErikG wrote…
Needs some grammar cleanup. "If using this {@code Builder} method, make sure the referenced blocks..."

Done.


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

Previously, RoboErikG wrote…
s/"name"/"NAME"/ ?

New constant PROCEDURE_NAME_FIELD, referenced by ProcedureCategoryFactory.


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

Previously, RoboErikG wrote…
Should mController be set here instead of in onAttached?

Done.


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

Previously, RoboErikG wrote…
nit line length

Done.


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

Previously, RoboErikG wrote…
nit line length

Done.


Comments from Reviewable