Insertion markers in Blockly! by rachel-fenichel · Pull Request #1987 · RaspberryPiFoundation/blockly

rachel-fenichel · 2018-07-27T00:52:21Z

The basics

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

The details

Proposed Changes

Add the insertion marker manager class
Mark the dragged connection manager class deprecated, and remove the only use of it
(I propose leaving it in until we have a clear upgrade strategy for developers)
Add utility functions that are called by the insertion marker manager

Reason for Changes

Blockly should have insertion markers! They look snazzy on Scratch Blocks, and the plan was always to backport them to Blockly.

Test Coverage

Unit tests pass.
Tested on:

Desktop Chrome

Additional Information

You can play with this on my demo site for now. I reserve the right to update that site without warning.

AnmAtAnm

Stepping back, is there any reason we need to support the original behavior for existing applications? Put another way, are we confident that this change is positive change for all developers and their users.

Off the top of my head, I can't think of any way in which the previous behavior is preferable, but I want our answer to be explicit and shared by everyone on our team.

AnmAtAnm · 2018-07-27T01:03:46Z

core/block.js

+  }
+  this.isInsertionMarker_ = insertionMarker;
+  if (this.isInsertionMarker_) {
+    // TODO: Why is this in Block.js instead of block_svg.js?


Move this TODO/question to the the setColour method in block.js?

As I said, this isn't for review yet. I'm not done with cleanup.

AnmAtAnm · 2018-07-27T01:49:36Z

core/block_render_svg.js

+
+/**
+ * Visual effect to show that if the dragging block is dropped, this block will
+ * be replaced.  If a shadow block it will disappear.  Otherwise it will bump.


Comma after

If a shadow block

AnmAtAnm · 2018-07-27T01:54:33Z

blockly_uncompressed.js

@@ -34,82 +34,83 @@ this.BLOCKLY_BOOT = function(root) {
    dir = this.BLOCKLY_DIR.match(/[^\/]+$/)[0];
  }
  // Execute after Closure has loaded.
-goog.addDependency("../../../" + dir + "/core/block.js", ['Blockly.Block'], ['Blockly.Blocks', 'Blockly.Comment', 'Blockly.Connection', 'Blockly.Events.BlockChange', 'Blockly.Events.BlockCreate', 'Blockly.Events.BlockDelete', 'Blockly.Events.BlockMove', 'Blockly.Extensions', 'Blockly.Input', 'Blockly.Mutator', 'Blockly.Warning', 'Blockly.Workspace', 'Blockly.Xml', 'goog.array', 'goog.asserts', 'goog.math.Coordinate', 'goog.string']);


I know this is the only compressed file that should be impacted by your changes, but it doesn't seem right to only update one of the files. I would rather see all three updated in a single followup push.

If I don't update this file it's not testable.

ewpatton · 2018-07-27T11:13:22Z

Two thoughts from my point of view: 1. Allowing for the old behavior would be nice. We will have to update videos and tutorial GIFs, which will take time, but I think App Inventor users would appreciate the new functionality. We did something similar with the grid and made it user toggleable. 2. The insertion marker is only the currently selected block. This doesn't seem quite right when dragging/inserting a stack of blocks since the to-be-displaced blocks move only for the size of the single block, not the whole stack. It would be nice if the blocks were displaced by the height of the whole stack.

…

On Jul 26, 2018, at 21:57, Andrew n marshall ***@***.***> wrote: @AnmAtAnm approved this pull request. Stepping back, is there any reason we need to support the original behavior for existing applications? Put another way, are we confident that this change is positive change for all developers and their users. Off the top of my head, I can't think of any way in which the previous behavior is preferable, but I want our answer to be explicit and shared by everyone on our team. In core/block.js: > + */ +Blockly.Block.prototype.isInsertionMarker = function() { + return this.isInsertionMarker_; +}; + +/** + * Set whether this block is an insertion marker block or not. + * @param {boolean} insertionMarker True if an insertion marker. + */ +Blockly.Block.prototype.setInsertionMarker = function(insertionMarker) { + if (this.isInsertionMarker_ == insertionMarker) { + return; // No change. + } + this.isInsertionMarker_ = insertionMarker; + if (this.isInsertionMarker_) { + // TODO: Why is this in Block.js instead of block_svg.js? Move this TODO/question to the the setColour method in block.js? In core/block_render_svg.js: > +Blockly.BlockSvg.prototype.positionNewBlock = function(newBlock, newConnection, + existingConnection) { + // We only need to position the new block if it's before the existing one, + // otherwise its position is set by the previous block. + if (newConnection.type == Blockly.NEXT_STATEMENT || + newConnection.type == Blockly.INPUT_VALUE) { + var dx = existingConnection.x_ - newConnection.x_; + var dy = existingConnection.y_ - newConnection.y_; + + newBlock.moveBy(dx, dy); + } +}; + +/** + * Visual effect to show that if the dragging block is dropped, this block will + * be replaced. If a shadow block it will disappear. Otherwise it will bump. Comma after If a shadow block In blockly_uncompressed.js: > @@ -34,82 +34,83 @@ this.BLOCKLY_BOOT = function(root) { dir = this.BLOCKLY_DIR.match(/[^\/]+$/)[0]; } // Execute after Closure has loaded. -goog.addDependency("../../../" + dir + "/core/block.js", ['Blockly.Block'], ['Blockly.Blocks', 'Blockly.Comment', 'Blockly.Connection', 'Blockly.Events.BlockChange', 'Blockly.Events.BlockCreate', 'Blockly.Events.BlockDelete', 'Blockly.Events.BlockMove', 'Blockly.Extensions', 'Blockly.Input', 'Blockly.Mutator', 'Blockly.Warning', 'Blockly.Workspace', 'Blockly.Xml', 'goog.array', 'goog.asserts', 'goog.math.Coordinate', 'goog.string']); I know this is the only compressed file that should be impacted by your changes, but it doesn't seem right to only update one of the files. I would rather see all three updated in a single followup push. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

RoboErikG · 2018-07-27T18:02:48Z

@ewpatton
Regarding 2. That was a trade off done mostly to reduce the cost of creating the insertion marker and re-rendering the stack. The insertion marker is actually a block, so it has to go through the whole rendering pass to figure out the size and shape. If there was a really big stack that you were putting inside another block it took a while to make the shadow of the entire stack.

It was also a little jarring when you dragged a stack down another stack and everything kept jumping off the bottom of the page. We found just using a single block was less jumpy. It may be confusing for some users, though, since at first they might think only one block will be connected.

rachel-fenichel · 2018-07-27T18:11:52Z

This is a preliminary pull request to show what changes are required. I'm not planning to land anything until I'm happy with behaviour and appearance in all cases and we have a good developer story.

Some information to consider:

InsertionMarkerManager and DraggedConnectionManager are fully interchangeable--that was a deliberate design choice.
All of the changes are additive except for the changes to block_dragger.js.
Put another way, you can get back to the old behaviour by reverting the three lines that changed in block_dragger.js.
The eventual goal is to fully switch to insertion markers, rather than maintaining both styles.
The maintenance cost of two paths like this is extremely high, mostly in manual testing on every release.
As a result, I do not want to add an option to the injection struct to choose between insertion markers and small highlights.
We've been working with insertion markers on scratch blocks for ~2 years and have been very happy with how intuitive they look and feel.
Insertion markers can cause performance problems for extremely deep block stacks or slow browsers.
We can continue to fix performance problems as we find them, which usually boosts performance across the board.
Performance fixes are worthwhile in their own right, and are still less work than ongoing maintenance of the dragged_connection_manager.

My current plan is to leave dragged_connection_manager.js in the repository, marked deprecated, for a few releases after we enable insertion markers. Developers who want to switch over more slowly, or keep the old style, can revert the three lines in block_dragger.js.

Developers who want it to be user-configurable would be responsible for including both styles (easy to do) and piping the user's decision to the right place in block_dragger.js. Those developers would also be responsible for keeping the old dragged_connection_manager.js in working form for their project. That's generally a smaller maintenance job than trying to keep it working for all possibly types of blockly projects.

Thoughts?

rachel-fenichel · 2018-07-27T18:15:46Z

@ewpatton for a working example of why the single-block behaviour is the right way to do it, hit the "Spaghetti" button and drag one of the if statements around inside the stack. It'll be slow, but you'll notice that you can move it down one block at a time, allowing you to insert it anywhere.

If the insertion marker contains the entire stack, the next block would jump out of view. The jumpiness isn't just annoying: it makes it impossible to move the insertion marker down by a single block. Instead you have to drag it to the side until the marker disappears, then drag back to the stack at (hopefully) the correct place.

ewpatton · 2018-07-27T19:19:57Z

@RoboErikG @rachel-fenichel Okay, that's sounds like a reasonable argument for the one blockly only approach. Thanks for the info.

…pe block

rachel-fenichel

These changes are now ready for review. I chose to make C-shape blocks always insert instead of wrapping. That choice isn't permanent: we may add more complex behaviour later.

RoboErikG

Needs tests.

RoboErikG · 2018-11-19T20:16:35Z

core/block_dragger.js


+  // // During a drag there may be a lot of rerenders, but not field changes.
+  // // Turn the cache on so we don't do spurious remeasures during the drag.
+  // Blockly.Field.startCache();


Did you forget to uncomment this line?

Whoops, fixed in a commit that I forgot to push.

RoboErikG · 2018-11-19T20:21:05Z

core/connection.js

+ * @return {boolean} true if the connection is not connected or is connected to
+ *    an insertion marker, false otherwise.
+ */
+Blockly.Connection.prototype.isConnectedToNonInsertionMarker = function() {


This doesn't seem to be used anywhere. Also, what about connections that would be connected except there's an insertion marker between blocks?

core/connection.js

RoboErikG · 2018-11-19T20:24:33Z

core/connection.js

+    // marker is in the middle of a stack, it won't work.
+    return !targetBlock.getPreviousBlock();
+  }
+  // ???


!!!

I'm unsure of what cases would end up here. I think it's worth having a log statement here, which will let me debug until all cases are covered. Any objections?

core/connection.js

core/constants.js

rachel-fenichel · 2018-11-19T22:50:36Z

I added tests; PTAL.

RoboErikG

Some references to Scratch specific code in the comments that need to be cleaned up. Once that's done lgtm.

RoboErikG · 2018-11-20T00:18:21Z

core/insertion_marker_manager.js

+
+/**
+ * Get rid of the highlighting marking the block that will be replaced.
+ * Scratch-specific code, where "highlighting" applies to a block rather than


"Scratch-specific code" then why is it here?

RoboErikG · 2018-11-20T00:18:42Z

core/insertion_marker_manager.js

+
+/**
+ * Add highlighting showing which block will be replaced.
+ * Scratch-specific code, where "highlighting" applies to a block rather than


'Scratch specific code'?

RoboErikG · 2018-11-20T00:19:14Z

core/insertion_marker_manager.js

+  /**
+   * The insertion marker that shows up between blocks to show where a block
+   * would go if dropped immediately.
+   * This is the scratch-blocks equivalent of connection highlighting.


remove ref to scratch-blocks.

RoboErikG · 2018-11-20T00:19:28Z

core/insertion_marker_manager.js

+  /**
+   * Connection on the insertion marker block that corresponds to
+   * this.localConnection_ on the currently dragged block.
+   * This is part of the scratch-blocks equivalent of connection highlighting.


remove scratch-blocks ref.

AnmAtAnm approved these changes Jul 27, 2018

View reviewed changes

rachel-fenichel added 9 commits November 14, 2018 11:18

Add insertion markers.

7d77590

Remove setOpacity and getOpacity

df888eb

Mark the dragged connection manager deprecated

dfc1f00

Fix horizontal insertion markers for blockly

565569f

Highlight and unhilight the closest connection

4c133e9

Make replaceable child blocks opaque

9aa54ac

Make insertion markers match the size of the source block

e794b68

Fix problem with connecting a statement into a stack, but not a c-sha…

17823db

…pe block

Prefer inserting to wrapping for c-shape blocks

53d8ac8

rachel-fenichel commented Nov 19, 2018

View reviewed changes

rachel-fenichel requested a review from RoboErikG November 19, 2018 19:32

RoboErikG reviewed Nov 19, 2018

View reviewed changes

rachel-fenichel added 5 commits November 19, 2018 12:44

Minor cleanup

991b704

Cleanup

d70e89d

Add warning for default case in canConnectToPrevious_

a8b0b90

Add private annotation

ef23820

Add tests for canConnectToPrevious

13740f4

RoboErikG approved these changes Nov 20, 2018

View reviewed changes

Comment cleanup; remove logs

ff901a7

rachel-fenichel merged commit c7b5028 into RaspberryPiFoundation:develop Nov 20, 2018

rachel-fenichel mentioned this pull request Nov 20, 2018

Feature/insertion markers #2127

Closed

rachel-fenichel deleted the feature/insertion_markers branch November 20, 2018 00:34

Conversation

rachel-fenichel commented Jul 27, 2018

The basics

The details

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

Uh oh!

AnmAtAnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewpatton commented Jul 27, 2018 via email

Uh oh!

RoboErikG commented Jul 27, 2018

Uh oh!

rachel-fenichel commented Jul 27, 2018

Uh oh!

rachel-fenichel commented Jul 27, 2018

Uh oh!

ewpatton commented Jul 27, 2018

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

RoboErikG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rachel-fenichel commented Nov 19, 2018

Uh oh!

RoboErikG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants