Added Max Instances Property to Workspace Options #2130

BeksOmega · 2018-11-20T20:38:43Z

The basics

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

The details

Resolves

Also resolves a bug I could not find an issue for (I assume because no one noticed) where you could not duplicate a block, even if there was enough remaining capacity in the workspace to do so, because the context menu was checking against block.getDecendants() which includes blocks that do not get duplicated.

Proposed Changes

Adds a maxInstances map of block types -> max instances number to the workspace options object
Adds an isDuplicatable() function to Blockly.Block.
Disables the block in the toolbox if there are maxInstances number of that block in the workspace.
Disables the Duplicate context menu option if the block is not duplicatable.
Disables pasting the block from the clipboard if the block is not duplicatable.

Reason for Changes

Adding this feature gives additional flexibility to event-driven and multi-threaded type Blockly applications. It also suggests creating Blockly games that force you to work more "efficiently" (with a limit number, and types, of blocks).

Test Coverage

I tested the changes by following this proceedure:

Add a maxInstances property to the "controls_if" block with a value of 3.
Create 3 instances of the block, see if the block is disabled in the toolbox flyout. (Pass)
Create 1 instance of the block. Copy it to the clipboard. Press control-v repeatedly. See if the blocks stop pasting after 3 instances. (Pass)
Create a "controls_if" block with another "controls_if" block inside it. See if the duplicate option is enabled for the inner if, but disabled for the outer if. (Pass)
Remove the maxInstaces property from the "controls_if" block.
Add a maxInstances property to the "logic_boolean" block with a value of 1.
Create a "controls_if" block with another "controls_if" block inside it. Attatch a "logic_boolean" block to the inner if. See if the duplicate option is disabled for both blocks. (Pass)

This proceedure tests all of the features of this PR:

Adds maxInstances property to JsonInit Object - It is being read properly in all steps.
Adds an isDuplicatable() function to Blockly.Block. - This function is working correctly in all applicable steps (3, 4, 7)
Disables the block in the toolbox if there are maxInstances number of that block in the workspace. - This is working correctly in all applicable steps (1)
Disables the Duplicate context menu option if the block is not duplicatable. - This is working correctly in all applicable steps (4, 7)
Disables pasting the block from the clipboard if the block is not duplicatable. - This is working correctly in all applicable steps (7)

Tested on:

Desktop Chrome

Additional Information

googlebot · 2018-11-20T20:38:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

BeksOmega · 2018-11-20T20:46:08Z

I signed it!

googlebot · 2018-11-20T20:46:10Z

CLAs look good, thanks!

RoboErikG

Some high level comments on structure.

core/block.js

…k type to max instances). isDuplicate() changed to correctly handle siblings/branches.

RoboErikG

Structure looks good. Couple more notes on APIs and runtime. Expect one more review to go over nits and style.

Thanks again for making this change!

core/block.js

core/workspace.js

core/block.js

NeilFraser

Can you also create a test block in the 'Basic' category of the playground with the words "limit 3 instances":
tests/playground.html?toolbox=test-blocks
That way this feature may be easily tested.

NeilFraser · 2018-11-26T19:35:09Z

core/block.js

+Blockly.Block.prototype.isDuplicatable = function() {
+  var copyableBlocks = this.getDescendants(true);
+  // Remove all "next statement" blocks because they will not be copied.
+  if (this.getNextBlock()) {


Cache the result of getNextBlock rather than calling it twice.

NeilFraser · 2018-11-26T20:32:29Z

core/block.js

+
+  var checkedTypes = [];
+  for (var i = 0, checkBlock; checkBlock = copyableBlocks[i]; i++) {
+    if (checkedTypes.includes(checkBlock.type)) {


Array.prototype.includes isn't ES5; not supported by IE. Use indexOf != -1 instead.

However, checkedTypes really should be a map for many reasons (including the one Erik states below), so this problem goes away. Note that indexOf and includes are both O(n), whereas a map lookup is essentially O(1).

NeilFraser · 2018-11-26T20:42:10Z

core/workspace.js

+ * @param {!Blockly.Block} block Block to remove.
+ */
+Blockly.Workspace.prototype.removeTypedBlock = function(block) {
+  this.typedBlocksDB_[block.type].splice(this.typedBlocksDB_[block.type].indexOf(block), 1);


Line length.

NeilFraser · 2018-11-26T20:42:54Z

core/workspace.js

+ */
+Blockly.Workspace.prototype.removeTypedBlock = function(block) {
+  this.typedBlocksDB_[block.type].splice(this.typedBlocksDB_[block.type].indexOf(block), 1);
+  if (this.typedBlocksDB_[block.type].length === 0) {


'!' is shorter than '=== 0'.

NeilFraser · 2018-11-26T20:45:06Z

core/workspace.js

+    if (this.RTL) {
+      offset *= -1;
+    }
+    blocks.sort(function(a, b) {


If you pull this function out and make it a static helper function, then it won't need to be redeclared every time getBlocklyByType is called. Better performance.

Suggest:
Blockly.Workspace.prototype.getBlocksByType.sortFunc_ = ...

Sounds good to me! 2 things:

The same sort function is used by Blockly.Workspace.prototype.getTopBlocks, so I figure just declare it a private function of the workspace?

I'm not sure how to pass the offset var into the sort method if I separate it out. My thought was to set a .offset property of the sort function, and then have it use this.offset, but declaring properties of functions seems like a bad idea (not even sure if it would work).

Good idea.

I think a property on the function is also a good idea. It's a seldom used but useful ability in JS.

NeilFraser · 2018-11-26T20:51:55Z

core/workspace.js

+ * @return {!Array.<!Blockly.Block>} The blocks of the given type.
+ */
+Blockly.Workspace.prototype.getBlocksByType = function(type, ordered) {
+  if (this.typedBlocksDB_[type]) {


Using slice(0) is faster than [].concat:
https://jsperf.com/cloning-arrays/22

Also, try reversing this to drop the 'else'.

if (!this.typedBlocksDB_[type]) { return []; } var blocks = this.typedBlocksDB_[type].slice(0);

NeilFraser · 2018-11-26T20:53:27Z

core/block.js

+ * decendents will put this block over the workspace\'s capacity this block is
+ * not duplicatable. If duplicating this block and decendents will put any
+ * type over their maxInstances this block is not duplicatable.
+ * @return {boolean} True if duplicatable


We prefer comments to end with punctuation. (Silliest nit, ever.)

NeilFraser · 2018-11-26T20:54:01Z

core/block.js

@@ -668,6 +670,43 @@ Blockly.Block.prototype.setMovable = function(movable) {
  this.movable_ = movable;
 };

+/**
+ * Get whether is block is duplicatable or not. If duplicating this block and
+ * decendents will put this block over the workspace\'s capacity this block is


Why the backslash before the apostrophe?

NeilFraser · 2018-11-26T20:54:32Z

core/block.js

+/**
+ * Get whether is block is duplicatable or not. If duplicating this block and
+ * decendents will put this block over the workspace\'s capacity this block is
+ * not duplicatable. If duplicating this block and decendents will put any


Did you mean: descendants?

Still one more 'decendents' on this line.

There's a second instance of decendents -> descendants.

NeilFraser · 2018-11-26T20:58:15Z

core/blockly.js

@@ -98,6 +98,13 @@ Blockly.clipboardXml_ = null;
 */
 Blockly.clipboardSource_ = null;

+/**
+ * Copied object.


Add a bit more context here. Something like:
Object copied onto the clipboard.

NeilFraser · 2018-11-26T21:12:00Z

Don't let the avalanche of comments discourage you. This is an awesome change.

RoboErikG · 2018-11-26T23:22:24Z

core/block.js

+/**
+ * Get whether is block is duplicatable or not. If duplicating this block and
+ * decendents will put this block over the workspace\'s capacity this block is
+ * not duplicatable. If duplicating this block and decendents will put any


There's a second instance of decendents -> descendants.

RoboErikG · 2018-11-26T23:25:10Z

core/blockly.js

@@ -273,6 +281,7 @@ Blockly.copy_ = function(toCopy) {
  }
  Blockly.clipboardXml_ = xml;
  Blockly.clipboardSource_ = toCopy.workspace;
+  Blockly.copiedObject_ = toCopy;


Missed this before: What happens if the block on the clipboard gets deleted? I think you may have to optimistically create the copy and then undo it if it causes a count to go over the limit.

Thought about it some more. Instead of putting the block stack to copy here can you put the map of typecounts? That way it'll be quick to check when pasting and you won't need to undo anything.

Sounds like a good solution, but could you give me some tips on implimentation?

Currently the map of typecounts is stored as a local variable inside the isDuplicatable function. It could be moved to a property of the block object, but to get an updated version of it you'd need to call some getCopyableBlocksTypeCounts() function (since isDuplicatable stops once it runs into a type that goes over the remainingCapacityOfType).

So if the above doesn't have any gaps in logic, we have two options:

getCopyableBlocksTypeCounts() & isDuplicatable() share duplicate code.

isDuplicatable() calls getCopyableBlocksTypeCounts() and then iterates over the types (like blockly core would do on paste).

Idk if I should minimize iterations, duplicate code, or if there is a different solution.

Let's go with 2. It'll be easier to maintain and visiting every block once shouldn't be too bad.

The method probably belongs in Blockly.utils.getBlockTypeCounts(block).

NeilFraser

LGTM for JS style. Wait for Erik to also approve.

NeilFraser · 2018-11-26T23:20:51Z

core/block.js

+/**
+ * Get whether is block is duplicatable or not. If duplicating this block and
+ * decendents will put this block over the workspace\'s capacity this block is
+ * not duplicatable. If duplicating this block and decendents will put any


Still one more 'decendents' on this line.

NeilFraser · 2018-11-26T23:23:35Z

core/block.js

+    return false;
+  }
+
+  var copyableBlocksTypeCounts = {};


Object.create(null) would be better than {}, since if there's ever a block type named "constructor", or "toString", the results would be unexpected.

NeilFraser · 2018-11-26T23:26:22Z

core/workspace.js

@@ -189,6 +194,54 @@ Blockly.Workspace.prototype.getTopBlocks = function(ordered) {
  return blocks;
 };

+/** Add a block to the list of blocks keyed by type.


First line of a JSDoc should be on the next line (/** should be on its own). Here and method below.

NeilFraser · 2018-11-26T23:28:50Z

core/workspace.js

+    if (this.RTL) {
+      offset *= -1;
+    }
+    blocks.sort(function(a, b) {


Good idea.

I think a property on the function is also a good idea. It's a seldom used but useful ability in JS.

rachel-fenichel · 2018-11-27T19:16:49Z

.eslintrc

@@ -1,68 +0,0 @@
-{


Undelete this file (from memory I think you should run git checkout develop -- .eslintrc)

Yeah I just noticed, my dad was helping me get set up with a better dev environment last night and must have commited some stuff.

Should I remove the changes to the .gitignore as well? (it added anything beneath idea/ to the ignore list)

Yeah, go ahead and revert that as well.

…. Fixed nits. Undeleted .eslintrc

…orkspace. Changed clipboard to save typeCountsMap rather than object.

RoboErikG · 2018-11-28T00:34:14Z

Everything looks good to me. Thanks again for this PR!

Added Max Instances property to Blocks

b4159b9

BeksOmega added 2 commits November 20, 2018 13:17

eslint cleanup

a723541

eslint cleanup 2

162900f

rachel-fenichel requested a review from NeilFraser November 20, 2018 21:28

RoboErikG reviewed Nov 21, 2018

View reviewed changes

core/block.js Outdated Show resolved Hide resolved

core/block.js Outdated Show resolved Hide resolved

core/block.js Outdated Show resolved Hide resolved

BeksOmega added 2 commits November 21, 2018 16:26

Moved maxInstances property from block to workspace (as a map of bloc…

5ebfd67

…k type to max instances). isDuplicate() changed to correctly handle siblings/branches.

eslint cleanup

a581b5a

BeksOmega changed the title ~~Added Max Instances Property to Blocks~~ Added Max Instances Property to Workspace Options Nov 25, 2018

RoboErikG reviewed Nov 26, 2018

View reviewed changes

core/block.js Outdated Show resolved Hide resolved

core/workspace.js Show resolved Hide resolved

core/block.js Outdated Show resolved Hide resolved

NeilFraser requested changes Nov 26, 2018

View reviewed changes

BeksOmega added 2 commits November 26, 2018 14:27

Changed checking types to map. Added hasBlockLimits. Fixed Nits.

d0a9fc4

Added limit_instances test block. eslint fixes.

7a64832

RoboErikG reviewed Nov 26, 2018

View reviewed changes

NeilFraser approved these changes Nov 26, 2018

View reviewed changes

fixup! Added limit_instances test block. eslint fixes.

110b84a

rachel-fenichel reviewed Nov 27, 2018

View reviewed changes

BeksOmega added 3 commits November 27, 2018 12:51

Changed sorting objects to a private static function of the workspace…

b810bc2

…. Fixed nits. Undeleted .eslintrc

Reverted .gitignore file.

86d0a20

Added getBlockTypeCounts() to utils. Added isCapacityAvailable() to w…

17584ee

…orkspace. Changed clipboard to save typeCountsMap rather than object.

RoboErikG merged commit 1c4ba38 into google:develop Nov 28, 2018

BeksOmega deleted the feature/issue1435 branch November 29, 2018 00:39

rachel-fenichel mentioned this pull request Dec 14, 2018

December 2018 release #2174

Merged

Added Max Instances Property to Workspace Options #2130

Added Max Instances Property to Workspace Options #2130

Conversation

BeksOmega commented Nov 20, 2018 • edited

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

googlebot commented Nov 20, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

BeksOmega commented Nov 20, 2018

googlebot commented Nov 20, 2018

RoboErikG left a comment

Choose a reason for hiding this comment

RoboErikG left a comment

Choose a reason for hiding this comment

NeilFraser 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

BeksOmega Nov 26, 2018 • edited

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

NeilFraser commented Nov 26, 2018

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

NeilFraser 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

RoboErikG commented Nov 28, 2018

BeksOmega commented Nov 20, 2018 •

edited

BeksOmega Nov 26, 2018 •

edited