Closed RoboErikG closed 7 years ago
Reviewed 4 of 30 files at r1. Review status: 4 of 30 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 43 at r2 (raw file):
*/ public class LuaActivity extends AbstractBlocklyActivity { private static final String TAG = "SplitActivity";
wrong class
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 47 at r2 (raw file):
private static final String SAVED_WORKSPACE_FILENAME = "lua_workspace.xml"; // Add custom blocks to this list. private static final List<String> BLOCK_DEFINITIONS = Arrays.asList(
DefaultBlocks.ALL_BLOCK_DEFINITIONS
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 114 at r2 (raw file):
@Override protected int getActionBarMenuResId() { return R.menu.turtle_actionbar;
This has items for Turtle specific workspaces, like "Android", "Lacey Curves" and "Paint Strokes".
blocklylib-core/src/main/java/com/google/blockly/android/AbstractBlocklyActivity.java, line 397 at r2 (raw file):
*/ @NonNull abstract protected CodeGenerationRequest.LanguageDefinition getBlockGeneratorLanguage();
Let this default to the JavaScript language definition. I don't see us wanting to ship the JavaScript assets separately, and that we are pushing JavaScript for the Google runtime.
Comments from Reviewable
Review status: 4 of 30 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 89 at r1 (raw file):
Thoughts on whether this should get its own file?
Definitely think it deserves it's own file, probably with the JavaScript default nested within.
Comments from Reviewable
Reviewed 1 of 30 files at r1. Review status: 5 of 30 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 92 at r2 (raw file):
public final String mLanguageFilename; public final String mLanguageNamespace;
This would be a great place to define reserved words in a way that the NameManagers can access them. https://github.com/google/blockly/blob/master/generators/javascript.js#L45 https://github.com/google/blockly/blob/master/generators/lua.js#L46
Comments from Reviewable
Review status: 5 of 30 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 2 at r2 (raw file):
/* * Copyright 2015 Google Inc. All Rights Reserved.
year
blocklylib-core/src/main/assets/background_compiler.html, line 12 at r2 (raw file):
<script> function import_generator_language(filename) { console.log('importing generator language ' + filename);
Remove log.
blocklylib-core/src/main/java/com/google/blockly/android/BlocklyActivityHelper.java, line 91 at r2 (raw file):
protected BlocklyController mController; protected CodeGeneratorManager mCodeGeneratorManager; protected String mCodeGeneratorLanguage = "javascript_compressed.js";
Where's this used?
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 87 at r2 (raw file):
* Defines the core language file to be used in code generation. To be used by the generator * Blockly needs to know the path to the file and the object namespace that has the generator * functions. For example: {}"javascript_compressed.js", "Blockly.JavaScript"}
typo {}
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 97 at r2 (raw file):
* * @param filename The path to the language file relative to * file:///android_assets/background_compiler.html.
This is the path to the generator file right?
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorService.java, line 216 at r2 (raw file):
@JavascriptInterface public String getGeneratorLanguageFilename() { if (mGeneratorLanguage == null) {
Seems strange that this defaults to javascript.
Either remove or set it on instantiation.
blocklylib-core/src/main/java/com/google/blockly/model/DefaultBlocks.java, line 43 at r2 (raw file):
* Standard definition for the JavaScript language generator. */ public static final CodeGenerationRequest.LanguageDefinition JAVASCRIPT_LANGUAGE_DEF
Shouldn't this be defined in LanguageDefinition?
blocklytest/src/androidTest/assets/lua/generators/test_blocks.js, line 22 at r2 (raw file):
/** * @fileoverview Generating Lua for colour blocks.
Update comments.
blocklytest/src/androidTest/java/com/google/blockly/android/codegen/CodeGeneratorServiceTest.java, line 79 at r2 (raw file):
Arrays.asList(new String[] {"lua/generators/test_blocks.js"})); mManager.requestCodeGeneration(request); Mockito.verify(mCallback, Mockito.timeout(500)).onFinishCodeGeneration("local _ = 'test'\n");
line length
Comments from Reviewable
Review status: 3 of 21 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 2 at r2 (raw file):
year
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 43 at r2 (raw file):
wrong class
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 47 at r2 (raw file):
DefaultBlocks.ALL_BLOCK_DEFINITIONS
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/LuaActivity.java, line 114 at r2 (raw file):
This has items for Turtle specific workspaces, like "Android", "Lacey Curves" and "Paint Strokes".
Done.
blocklylib-core/src/main/assets/background_compiler.html, line 12 at r2 (raw file):
Remove log.
Done.
blocklylib-core/src/main/java/com/google/blockly/android/AbstractBlocklyActivity.java, line 397 at r2 (raw file):
Let this default to the JavaScript language definition. I don't see us wanting to ship the JavaScript assets separately, and that we are pushing JavaScript for the Google runtime.
Done.
blocklylib-core/src/main/java/com/google/blockly/android/BlocklyActivityHelper.java, line 91 at r2 (raw file):
Where's this used?
Done.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 89 at r1 (raw file):
Definitely think it deserves it's own file, probably with the JavaScript default nested within.
Done.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 87 at r2 (raw file):
typo {}
Done.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 92 at r2 (raw file):
This would be a great place to define reserved words in a way that the NameManagers can access them. https://github.com/google/blockly/blob/master/generators/javascript.js#L45 https://github.com/google/blockly/blob/master/generators/lua.js#L46
Added a TODO
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 97 at r2 (raw file):
This is the path to the generator file right?
Correct.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorService.java, line 216 at r2 (raw file):
Seems strange that this defaults to javascript. Either remove or set it on instantiation.
added some checks in the request to require it and made this throw if it's missing.
blocklylib-core/src/main/java/com/google/blockly/model/DefaultBlocks.java, line 43 at r2 (raw file):
Shouldn't this be defined in LanguageDefinition?
Done.
blocklytest/src/androidTest/assets/lua/generators/test_blocks.js, line 22 at r2 (raw file):
Update comments.
Done.
blocklytest/src/androidTest/java/com/google/blockly/android/codegen/CodeGeneratorServiceTest.java, line 79 at r2 (raw file):
line length
Done.
Comments from Reviewable
Review status: 3 of 21 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.
Comments from Reviewable
Reviewed 2 of 30 files at r1, 14 of 27 files at r3, 1 of 2 files at r4, 1 of 1 files at r6. Review status: 20 of 21 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.
blocklydemo/src/main/java/com/google/blockly/android/demo/SplitActivity.java, line 30 at r5 (raw file):
import com.google.blockly.android.codegen.CodeGenerationRequest; import com.google.blockly.model.BlocklySerializerException; import com.google.blockly.model.DefaultBlocks;
unused?
blocklydemo/src/main/java/com/google/blockly/android/demo/StylesActivity.java, line 22 at r5 (raw file):
import com.google.blockly.android.codegen.CodeGenerationRequest; import com.google.blockly.android.codegen.LoggingCodeGeneratorCallback; import com.google.blockly.model.DefaultBlocks;
unused?
blocklydemo/src/main/java/com/google/blockly/android/demo/flyout/AlwaysOpenToolboxActivity.java, line 24 at r5 (raw file):
import com.google.blockly.android.codegen.LoggingCodeGeneratorCallback; import com.google.blockly.android.demo.R; import com.google.blockly.model.DefaultBlocks;
Unused?
blocklydemo/src/main/java/com/google/blockly/android/demo/flyout/NoCategoriesToolboxActivity.java, line 11 at r5 (raw file):
import com.google.blockly.android.codegen.LoggingCodeGeneratorCallback; import com.google.blockly.android.demo.R; import com.google.blockly.model.DefaultBlocks;
Unused?
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 44 at r6 (raw file):
*/ public CodeGenerationRequest(String xml, CodeGeneratorCallback callback, LanguageDefinition generatorsLanguage, List<String> blockDefinitionsFilenames,
Might as well mark these params @NotNull.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/LanguageDefinition.java, line 17 at r6 (raw file):
public final String mLanguageFilename; public final String mLanguageNamespace;
JavaDoc these public fields. Also, mLanguageNamespace => mGeneratorRef, per offline.
Comments from Reviewable
with nits.
Review status: 20 of 21 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.
Comments from Reviewable
Review status: 14 of 18 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.
blocklydemo/src/main/java/com/google/blockly/android/demo/SplitActivity.java, line 30 at r5 (raw file):
unused?
Done.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGenerationRequest.java, line 44 at r6 (raw file):
Might as well mark these params @NotNull.
Done.
blocklylib-core/src/main/java/com/google/blockly/android/codegen/LanguageDefinition.java, line 17 at r6 (raw file):
JavaDoc these public fields. Also, mLanguageNamespace => mGeneratorRef, per offline.
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/StylesActivity.java, line 22 at r5 (raw file):
unused?
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/flyout/AlwaysOpenToolboxActivity.java, line 24 at r5 (raw file):
Unused?
Done.
blocklydemo/src/main/java/com/google/blockly/android/demo/flyout/NoCategoriesToolboxActivity.java, line 11 at r5 (raw file):
Unused?
Done.
Comments from Reviewable
This adds a LanguageDefinition that includes the path to the base language and the object namespace that language uses and makes it required for code generation. Adds a test for generating Lua.
This change is