Addressing PR feedback.

blocks/loops.ts

@@ -334,6 +334,11 @@ export type ControlFlowInLoopBlock = Block & ControlFlowInLoopMixin;
 interface ControlFlowInLoopMixin extends ControlFlowInLoopMixinType {}
 type ControlFlowInLoopMixinType = typeof CONTROL_FLOW_IN_LOOP_CHECK_MIXIN;
 
+/**
+ * The language-neutral ID for when the reason why a block is disabled is
+ * because the block is only valid inside of a loop.
+ */
+const CONTROL_FLOW_NOT_IN_LOOP_DISABLED_REASON = 'CONTROL_FLOW_NOT_IN_LOOP';
 /**
  * This mixin adds a check to make sure the 'controls_flow_statements' block
  * is contained in a loop. Otherwise a warning is added to the block.
@@ -380,7 +385,10 @@ const CONTROL_FLOW_IN_LOOP_CHECK_MIXIN = {
       const group = Events.getGroup();
       // Makes it so the move and the disable event get undone together.
       Events.setGroup(e.group);
-      this.setDisabledReason(!enabled, 'CONTROL_FLOW_NOT_IN_LOOP');
+      this.setDisabledReason(
+        !enabled,
+        CONTROL_FLOW_NOT_IN_LOOP_DISABLED_REASON,
+      );
       Events.setGroup(group);
     }
   },

blocks/procedures.ts

@@ -763,6 +763,13 @@ type CallExtraState = {
   params?: string[];
 };
 
+/**
+ * The language-neutral ID for when the reason why a block is disabled is
+ * because the block's corresponding procedure definition is disabled.
+ */
+const DISABLED_PROCEDURE_DEFINITION_DISABLED_REASON =
+  'DISABLED_PROCEDURE_DEFINITION';
+
 /**
  * Common properties for the procedure_callnoreturn and
  * procedure_callreturn blocks.
@@ -1124,7 +1131,10 @@ const PROCEDURE_CALL_COMMON = {
         }
         Events.setGroup(event.group);
         const valid = def.isEnabled();
-        this.setDisabledReason(!valid, 'DISABLED_PROCEDURE_DEFINITION');
+        this.setDisabledReason(
+          !valid,
+          DISABLED_PROCEDURE_DEFINITION_DISABLED_REASON,
+        );
         this.setWarningText(
           valid
             ? null
@@ -1217,6 +1227,12 @@ interface IfReturnMixin extends IfReturnMixinType {
 }
 type IfReturnMixinType = typeof PROCEDURES_IFRETURN;
 
+/**
+ * The language-neutral ID for when the reason why a block is disabled is
+ * because the block is only valid inside of a procedure body.
+ */
+const UNPARENTED_IFRETURN_DISABLED_REASON = 'UNPARENTED_IFRETURN';
+
 const PROCEDURES_IFRETURN = {
   /**
    * Block for conditionally returning a value from a procedure.
@@ -1317,7 +1333,7 @@ const PROCEDURES_IFRETURN = {
       const group = Events.getGroup();
       // Makes it so the move and the disable event get undone together.
       Events.setGroup(e.group);
-      this.setDisabledReason(!legal, 'UNPARENTED_IFRETURN');
+      this.setDisabledReason(!legal, UNPARENTED_IFRETURN_DISABLED_REASON);
       Events.setGroup(group);
     }
   },

core/contextmenu_items.ts

@@ -467,7 +467,14 @@ export function registerDisable() {
         block!.workspace.options.disable &&
         block!.isEditable()
       ) {
-        if (block!.getInheritedDisabled()) {
+        // Determine whether this block is currently disabled for any reason
+        // other than the manual reason that this context menu item controls.
+        const disabledReasons = block!.getDisabledReasons();
+        const isDisabledForOtherReason =
+          disabledReasons.size >
+          (disabledReasons.has(constants.MANUALLY_DISABLED) ? 1 : 0);
+
+        if (block!.getInheritedDisabled() || isDisabledForOtherReason) {
           return 'disabled';
         }
         return 'enabled';

core/events/utils.ts

@@ -188,6 +188,12 @@ export const COMMENT_COLLAPSE = 'comment_collapse';
  */
 export const FINISHED_LOADING = 'finished_loading';
 
+/**
+ * The language-neutral ID for when the reason why a block is disabled is
+ * because the block is not descended from a root block.
+ */
+const ORPHANED_BLOCK_DISABLED_REASON = 'ORPHANED_BLOCK';
+
 /**
  * Type of events that cause objects to be bumped back into the visible
  * portion of the workspace.
@@ -522,7 +528,6 @@ export function get(
  * @param event Custom data for event.
  */
 export function disableOrphans(event: Abstract) {
-  const disabledReason = 'ORPHANED_BLOCK';
   if (event.type === MOVE || event.type === CREATE) {
     const blockEvent = event as BlockMove | BlockCreate;
     if (!blockEvent.workspaceId) {
@@ -541,17 +546,20 @@ export function disableOrphans(event: Abstract) {
       try {
         recordUndo = false;
         const parent = block.getParent();
-        if (parent && !parent.hasDisabledReason(disabledReason)) {
+        if (
+          parent &&
+          !parent.hasDisabledReason(ORPHANED_BLOCK_DISABLED_REASON)
+        ) {
           const children = block.getDescendants(false);
           for (let i = 0, child; (child = children[i]); i++) {
-            child.setDisabledReason(false, disabledReason);
+            child.setDisabledReason(false, ORPHANED_BLOCK_DISABLED_REASON);
           }
         } else if (
           (block.outputConnection || block.previousConnection) &&
           !eventWorkspace.isDragging()
         ) {
           do {
-            block.setDisabledReason(true, disabledReason);
+            block.setDisabledReason(true, ORPHANED_BLOCK_DISABLED_REASON);
             block = block.getNextBlock();
           } while (block);
         }

core/flyout_base.ts

@@ -43,6 +43,13 @@ enum FlyoutItemType {
   BUTTON = 'button',
 }
 
+/**
+ * The language-neutral ID for when the reason why a block is disabled is
+ * because the workspace is at block capacity.
+ */
+const WORKSPACE_AT_BLOCK_CAPACITY_DISABLED_REASON =
+  'WORKSPACE_AT_BLOCK_CAPACITY';
+
 /**
  * Class for a flyout.
  */
@@ -1239,7 +1246,10 @@ export abstract class Flyout
           common.getBlockTypeCounts(block),
         );
         while (block) {
-          block.setDisabledReason(!enable, 'WORKSPACE_AT_BLOCK_CAPACITY');
+          block.setDisabledReason(
+            !enable,
+            WORKSPACE_AT_BLOCK_CAPACITY_DISABLED_REASON,
+          );
           block = block.getNextBlock();
         }
       }

core/serialization/blocks.ts

@@ -527,7 +527,7 @@ function loadAttributes(block: Block, state: State) {
       'enabled',
       'v11',
       'v12',
-      'disabledReasons to ["MANUALLY_DISABLED"]',
+      'disabledReasons with the value ["' + constants.MANUALLY_DISABLED + '"]',
     );
     block.setDisabledReason(true, constants.MANUALLY_DISABLED);
   }

core/xml.ts

@@ -1027,7 +1027,7 @@ function domToBlockHeadless(
       'disabled',
       'v11',
       'v12',
-      'disabled-reasons to "MANUALLY_DISABLED"',
+      'disabled-reasons with the value "' + constants.MANUALLY_DISABLED + '"',
     );
     block.setDisabledReason(
       disabled === 'true' || disabled === 'disabled',

tests/mocha/jso_serialization_test.js

@@ -86,20 +86,31 @@ suite('JSO Serialization', function () {
         });
       });
 
-      suite('Enabled', function () {
-        test('False', function () {
+      suite('DisabledReasons', function () {
+        test('One reason', function () {
           const block = this.workspace.newBlock('row_block');
           block.setDisabledReason(true, 'test reason');
           const jso = Blockly.serialization.blocks.save(block);
           assertProperty(jso, 'disabledReasons', ['test reason']);
         });
 
-        test('True', function () {
+        test('Zero reasons', function () {
           const block = this.workspace.newBlock('row_block');
           block.setDisabledReason(false, 'test reason');
           const jso = Blockly.serialization.blocks.save(block);
           assertNoProperty(jso, 'disabledReasons');
         });
+
+        test('Multiple reasons', function () {
+          const block = this.workspace.newBlock('row_block');
+          block.setDisabledReason(true, 'test reason 1');
+          block.setDisabledReason(true, 'test reason 2');
+          const jso = Blockly.serialization.blocks.save(block);
+          assertProperty(jso, 'disabledReasons', [
+            'test reason 1',
+            'test reason 2',
+          ]);
+        });
       });
 
       suite('Inline', function () {

tests/mocha/serializer_test.js

@@ -84,6 +84,12 @@ Serializer.Attributes.Disabled = new SerializerTestCase(
     '<block type="logic_negate" id="id******************" disabled-reasons="test%20reason,another%20reason" x="42" y="42"></block>' +
     '</xml>',
 );
+Serializer.Attributes.DisabledWithEncodedComma = new SerializerTestCase(
+  'DisabledWithEncodedComma',
+  '<xml xmlns="https://developers.google.com/blockly/xml">' +
+    '<block type="logic_negate" id="id******************" disabled-reasons="test%2Creason" x="42" y="42"></block>' +
+    '</xml>',
+);
 Serializer.Attributes.NotDeletable = new SerializerTestCase(
   'Deletable',
   '<xml xmlns="https://developers.google.com/blockly/xml">' +
@@ -106,6 +112,7 @@ Serializer.Attributes.testCases = [
   Serializer.Attributes.Basic,
   Serializer.Attributes.Collapsed,
   Serializer.Attributes.Disabled,
+  Serializer.Attributes.DisabledWithEncodedComma,
   Serializer.Attributes.NotDeletable,
   Serializer.Attributes.NotMovable,
   Serializer.Attributes.NotEditable,