Skip to content

Update connect to work with multiple connecting blocks#2905

Merged
alschmiedt merged 2 commits into
RaspberryPiFoundation:developfrom
alschmiedt:fix_connecting
Aug 26, 2019
Merged

Update connect to work with multiple connecting blocks#2905
alschmiedt merged 2 commits into
RaspberryPiFoundation:developfrom
alschmiedt:fix_connecting

Conversation

@alschmiedt
Copy link
Copy Markdown
Contributor

@alschmiedt alschmiedt commented Aug 23, 2019

The basics

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

The details

Resolves

There were three cases when connecting multiple blocks that were failing:

  1. When there is a marker in the middle of a stack and the cursor is on a previous connection:
  • The block with the cursor should be inserted into the stack.
  1. When there is a cursor in the middle of a stack and the marker is on a previous connection:
  • The stack should be moved to the location of the marker
  • The block with the cursor attached should be inserted into the stack
  1. When there is a marker in the middle of a stack and the cursor is on the next connection:
  • The block with the cursor should be inserted into the stack

Proposed Changes

Reason for Changes

Test Coverage

Tested on:

Documentation

Additional Information

There are a few more bugs around connecting inline inputs that will be addressed in a different PR.

@alschmiedt alschmiedt requested a review from RoboErikG August 23, 2019 21:27
Comment thread core/keyboard_nav/navigation.js Outdated
/**
* Tries to connect the given connections.
*
* We use the cursor to represent both the current connection and the target
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having one of the parameters called targetConnection makes this sentence really confusing since you're referring to the targetConnection property on each of the two connections being passed. Not sure how to clean that up...

Comment thread core/keyboard_nav/navigation.js Outdated
if (!movingConnection || !targetConnection) {
return false;
}
var markerTarget = targetConnection.targetConnection;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This rename also makes the code confusing. Because you didn't want to call it targetTarget you've implicitly renamed target to marker, which means when you use them both down below it's confusing how they're related. Change movingConnection and targetConnection to use names that can be continued with their Targets. Maybe movingConnection and toConnection or moving/destination?

Comment thread core/keyboard_nav/navigation.js Outdated

// When connecting a next and previous connection try inserting the moving
// connection into the stack before trying to connect them.
if (markerTarget &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is assuming the targetConnection is always a previous connection, which isn't the case at the end of a stack. It gets caught by later cases, but it's better practice to explicitly check things instead of depending on failures.

Comment thread core/keyboard_nav/navigation.js Outdated
* were connected, false otherwise.
* @package
*/
Blockly.navigation.connect = function(movingConnection, targetConnection) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could make this method a bit more readable by having it do the following:

  1. On the moving connection, get the best inferior connection or null.
  2. If 1 succeeded, on the destination connection, get the best superior connection or null.
  3. If 1 and 2 succeeded try connecting those two.
  4. Else, try connecting the moving and destination connections.

Picking the best inferior would be:

  1. If it's already an inferior connection use it.
  2. If it's the next connection on a block use its previous/output connection if it exists.

Picking the best superior would be:

  1. If already superior use it.
  2. Use its target if it exists.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this take into account #2 on the list? With the inferior connection being on the destination block? Also, if we have two next connections then it will find a way to connect those blocks, I don't particularly mind this but just throwing it out there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right, it doesn't. Let's stick with your version, but maybe organize it to be more readable instead of more efficient?

@alschmiedt alschmiedt merged commit a14275c into RaspberryPiFoundation:develop Aug 26, 2019
@alschmiedt alschmiedt deleted the fix_connecting branch May 29, 2020 15:33
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.

2 participants