Cursor ast update #2409

alschmiedt · 2019-04-25T17:53:05Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

Proposed Changes

Contains:

Adds ability to exit toolbox by pushing 'E'
When moving from stack to workspace places cursor above stack
Add Tests
Fixes for AST Node class from Keyboard nav ast node #2391
Refactors Cursor and Cursor_Svg to use AST_Node ( needs review )

Reason for Changes

Test Coverage

Tested on:

Additional Information

…e testing.

Kb exit toolbox

…e the recently selected stack.

* Set node version explicitly for travis * rebuild * Tweak travis config

When moving from a stack to the workspace, place the cursor just above the recently selected stack.

…into cursor_ast_update

…ckly into warnings_fix

Warnings fix

Add tests for keyboard handling in the toolbox

rachel-fenichel

A general comment: we need a way to describe an AST node "for" a given connection, block, etc. Representing? Pointing to? Wrapping? Pick one and be consistent in all comments. (I like "pointing to")

Several comments start with "Pattern". Those are comments that apply to multiple places in the code, but that I didn't tag every single spot. Please update anything you find with that pattern in code you touched in this PR, but you don't need to hunt down every use of it in your code.

rachel-fenichel · 2019-04-25T18:36:33Z

core/navigation.js

@@ -66,9 +68,36 @@ Blockly.Navigation.STATE_TOOLBOX = 3;

 /**
 * The current state the user is in.
+ * Initialized to workspace state since a user enters navigation mode by shift
+ * clicking on a block or workspace.


Add type annotation.

rachel-fenichel · 2019-04-25T18:43:41Z

core/ast_node.js

  /*
   * The type of the location.
-   * @type String
+   * One of Blockly.ASTNode.types
+   * @type string


Pattern: use @type {string} instead of @type string, and for other type annotations on properties. I think it's mostly here and in navigation.js.

rachel-fenichel · 2019-04-25T18:49:03Z

core/ast_node.js

 };

+/**
+ * The amount to move the workspace coordinate.


Please clarify.

rachel-fenichel · 2019-04-25T18:49:26Z

core/ast_node.js

 /**
 * Create an ast node from a field.
- * @param {?Blockly.Field} field The location of the ast node.
- * @return {?Blockly.ASTNode} An ast node for a field.
+ * @param {Blockly.Field} field The location of the ast node.


Pattern: move to a consistent version of AST Node in comments (from the current mix of astnode, ast node, ASTNode, AST Node, AST node)

rachel-fenichel · 2019-04-25T18:51:20Z

core/ast_node.js

  if (connection.type === Blockly.INPUT_VALUE) {
-    astNode = new Blockly.ASTNode(Blockly.ASTNode.types.INPUT, connection);
+    astNode = Blockly.ASTNode.createInputNode(connection.getParentInput());


Pattern: since you aren't doing anything to the astNode after the if/else, return the new node immediately, and return null at the end.

rachel-fenichel · 2019-04-25T19:04:50Z

core/cursor_svg.js

@@ -82,7 +83,7 @@ Blockly.CursorSvg.CURSOR_REFERENCE = null;

 /**
 * Parent svg element.
- * This is generally a block unless the cursor is on the workspace
+ * This is generally a block unless the cursor is on the workspace.


"This is generally a block's svg root, unless the cursor is on the workspace."

rachel-fenichel · 2019-04-25T19:05:23Z

core/cursor_svg.js

@@ -146,15 +147,16 @@ Blockly.CursorSvg.prototype.setParent_ = function(newParent) {

 /**
 * Show the cursor using coordinates.
- * @param {?goog.math.Coordinate} position The position of the cursor on the


Clean up comment (you left a word)

rachel-fenichel · 2019-04-25T19:06:08Z

core/cursor_svg.js

-Blockly.CursorSvg.prototype.update_ = function(position) {
-  this.showWithAnything_(position);
+Blockly.CursorSvg.prototype.update_ = function() {
+  this.showWithAnything_();


Consider moving all ofshowWithAnything into here and deleting that function.

rachel-fenichel · 2019-04-25T19:06:38Z

core/navigation.js

+};
+
+/**
+ * Get the insertion connection.


Explain what the insertion connection is.

rachel-fenichel · 2019-04-25T19:07:02Z

core/navigation.js

@@ -279,14 +320,16 @@ Blockly.Navigation.resetFlyout = function() {

 /**
 * Finds the best connection.
- * @param {!Blockly.Block} block The block to be connected.
- * @param {!Blockly.Connection} connection The connection to connect to.
+ * @param {Blockly.Block} block The block to be connected.


Explain what the best connection is

alschmiedt and others added 17 commits April 19, 2019 15:02

Refactors cursor to use ast nodes

0bb86f3

Small fixes from code review

5a0817e

Move cursor to workspace when E pressed in toolbox or flyout.

bff6eda

Fixes helper functions

8eb1c28

Add a navigation test file; use stubs to replace a function in astnod…

bf792fc

…e testing.

Merge pull request #2398 from rachel-fenichel/kb_exit_toolbox

76f7c45

Kb exit toolbox

When moving from a stack to the workspace, place the cursor just abov…

4371d67

…e the recently selected stack.

Set node version explicitly for travis; rebuild (#2403)

c4d0170

* Set node version explicitly for travis * rebuild * Tweak travis config

Merge pull request #2402 from rachel-fenichel/cursor_pos_ws

57cf9de

When moving from a stack to the workspace, place the cursor just above the recently selected stack.

Fixes compile errors

c27595a

Merge branch 'cursor_ast_update' of https://github.com/google/blockly …

1958390

…into cursor_ast_update

Merge branch 'cursor_ast_update' of https://github.com/alschmiedt/blo…

5007b8b

…ckly into warnings_fix

Add tests for keyboard handling in the toolbox

b2f79af

Cleanup

629147c

Code review fixes

986aa53

Merge pull request #2404 from alschmiedt/warnings_fix

d826baf

Warnings fix

Merge pull request #2405 from rachel-fenichel/tests/navigation_toolbox

9f3bf9b

Add tests for keyboard handling in the toolbox

rachel-fenichel reviewed Apr 25, 2019

View reviewed changes

Code review fixes

93aea9d

rachel-fenichel approved these changes Apr 25, 2019

View reviewed changes

alschmiedt merged commit efbfdf7 into keyboard_nav Apr 25, 2019

rachel-fenichel mentioned this pull request Jul 15, 2019

July 2019 release candidate (Q2 2019) #2644

Merged

alschmiedt deleted the cursor_ast_update branch October 3, 2019 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor ast update #2409

Cursor ast update #2409

alschmiedt commented Apr 25, 2019

rachel-fenichel left a comment

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

rachel-fenichel Apr 25, 2019

alschmiedt Apr 25, 2019

Cursor ast update #2409

Cursor ast update #2409

Conversation

alschmiedt commented Apr 25, 2019

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

rachel-fenichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment