Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connecting block to parent in domToBlockHeadless_ #4461

Merged
merged 6 commits into from
Jan 20, 2021

Conversation

moniika
Copy link
Contributor

@moniika moniika commented Nov 18, 2020

The basics

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

The details

Resolves

Fixes #2135

Proposed Changes

  • Connects block to it's parent in domToBlockHeadless_ rather than in the parent block's instantiation (allows for parent to be connected before field tag is processed)
    • Before: Block was connected it it's parent after it was fully loaded (which caused issues with fields validators that depended on the connected state of the block)
    • After: Block is connected to parent as it is being loaded, before it's field values are loaded

Reason for Changes

  • Solves issues with loading blocks with field validators that depend on the parent being connected (and accessing parent fields) from xml
  • Solves issues with loading dynamic dropdowns that depend on a connected parent from xml

Test Coverage

Ran tests and tested in advanced playground with the following block:

<xml xmlns="https://developers.google.com/blockly/xml">
  <variables>
    <variable>list</variable>
  </variables>
  <block type="controls_repeat_ext">
    <value name="TIMES">
      <block type="math_number">
        <field name="NUM">123</field>
      </block>
    </value>
    <statement name="DO">
      <block type="test_dropdowns_dynamic_connect_dependant">
        <field name="OPTIONS">wcjS31{oxnJqsPUgQ+_m_type_key</field>
        <next>
          <block type="lists_getIndex">
            <mutation statement="true" at="true"></mutation>
            <field name="MODE">REMOVE</field>
            <field name="WHERE">FROM_START</field>
            <value name="VALUE">
              <block type="variables_get">
                <field name="VAR">list</field>
              </block>
            </value>
            <value name="AT">
              <block type="lists_getIndex">
                <mutation statement="false" at="true"></mutation>
                <field name="MODE">GET</field>
                <field name="WHERE">FROM_START</field>
                <value name="VALUE">
                  <block type="variables_get">
                    <field name="VAR">list</field>
                  </block>
                </value>
                <value name="AT">
                  <block type="math_number">
                    <field name="NUM">123</field>
                  </block>
                </value>
              </block>
            </value>
            <next>
              <block type="test_dropdowns_dynamic_connect_dependant">
                <field name="OPTIONS">T1_y8l67gg$DXQ=u8Q~2_type_key</field>
              </block>
            </next>
          </block>
        </next>
      </block>
    </statement>
  </block>
</xml>

Documentation

Additional Information

The generator tests were very helpful for testing this change as it caught many edge cases of xml generation.

Depends on #4571

@moniika moniika marked this pull request as draft November 18, 2020 01:55
@moniika moniika changed the title Fix attempt Connecting child blocks before setting fields in domToBlockHeadless_ Nov 20, 2020
@moniika moniika changed the title Connecting child blocks before setting fields in domToBlockHeadless_ Connecting block to parent in domToBlockHeadless_ Jan 8, 2021
@BeksOmega
Copy link
Collaborator

With the new connection checker being more flexible, I wonder about connection checks that depend on the state of a parent block's fields. Wouldn't this mess with that?

@rachel-fenichel rachel-fenichel self-assigned this Jan 8, 2021
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.

I'm nervous about this because of backwards compatibility, but agree that making our parsing more clearly defined is a good move.

core/xml.js Outdated Show resolved Hide resolved
core/xml.js Outdated Show resolved Hide resolved
core/xml.js Outdated Show resolved Hide resolved
core/xml.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@moniika
Copy link
Contributor Author

moniika commented Jan 12, 2021

Rebased after merging change for parsing tags in explicit order.

@moniika moniika marked this pull request as ready for review January 12, 2021 00:18
@moniika moniika marked this pull request as draft January 12, 2021 00:28
@moniika
Copy link
Contributor Author

moniika commented Jan 12, 2021

With the new connection checker being more flexible, I wonder about connection checks that depend on the state of a parent block's fields. Wouldn't this mess with that?

I don't think this would be an issue if the connection checker depends on the parent block's fields.
The xml (#4571) is loaded in an explicit order, such that, the input/statement/value fields that create the child block are processed after the field tags are processed.

However, if the connection depends on the child block's fields, then those wont be loaded yet.
(Ex: a block with boolean field that must be true in order for it's output to be connected to parent)
(Ex: a block whose field value must match the field value of the parent in order for it to be connected)
(^ In the previous release, these cases worked only if the parent field in question was listed before the child block in xml)

Other potential new issue (introduced in #4571):

  • field validator depends on connected child blocks (used to work only if the field tag was listed before the statement/value/input tag in the xml)

Other Remaining unsolved issues (not new issues):

  • child field validator depends on parent's other connected statement/input/value child blocks
  • field validator depends on all blocks in the workspace being loaded

@moniika moniika marked this pull request as ready for review January 12, 2021 01:26
@moniika moniika merged commit c94f40d into google:develop Jan 20, 2021
@moniika moniika deleted the dynamic-dropdown branch April 6, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants