Skip to content

Commit

Permalink
feat!: Invalid Blocks (#7958)
Browse files Browse the repository at this point in the history 
* feat: Invalid Blocks

* Rename the new json property from invalid to invalidReasons.

* Merged isValid into isEnabled.

* Minor fixes.

* More minor fixes.

* Reverting some stuff that didn't need to change.

* Addressing PR feedback.

* Update the BlockInfo interface to match State.

* Make BlockChange.disabledReason private.
  • Loading branch information
@johnnesky
johnnesky authored Apr 18, 2024
1 parent 7d8f88a commit cee7f91
Show file tree
Hide file tree
Showing 26 changed files with 492 additions and 97 deletions.
16 changes: 14 additions & 2 deletions blocks/loops.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -365,7 +370,11 @@ const CONTROL_FLOW_IN_LOOP_CHECK_MIXIN = {
// Don't change state if:
// * It's at the start of a drag.
// * It's not a move event.
if (!ws.isDragging || ws.isDragging() || e.type !== Events.BLOCK_MOVE) {
if (
!ws.isDragging ||
ws.isDragging() ||
(e.type !== Events.BLOCK_MOVE && e.type !== Events.BLOCK_CREATE)
) {
return;
}
const enabled = !!this.getSurroundLoop();
Expand All @@ -376,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.setEnabled(enabled);
this.setDisabledReason(
!enabled,
CONTROL_FLOW_NOT_IN_LOOP_DISABLED_REASON,
);
Events.setGroup(group);
}
},
Expand Down
36 changes: 25 additions & 11 deletions blocks/procedures.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,6 @@ interface CallMixin extends CallMixinType {
defType_: string;
quarkIds_: string[] | null;
quarkConnections_: {[id: string]: Connection};
previousEnabledState_: boolean;
}
type CallMixinType = typeof PROCEDURE_CALL_COMMON;

Expand All @@ -764,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.
Expand Down Expand Up @@ -1124,12 +1130,16 @@ const PROCEDURE_CALL_COMMON = {
);
}
Events.setGroup(event.group);
if (blockChangeEvent.newValue) {
this.previousEnabledState_ = this.isEnabled();
this.setEnabled(false);
} else {
this.setEnabled(this.previousEnabledState_);
}
const valid = def.isEnabled();
this.setDisabledReason(
!valid,
DISABLED_PROCEDURE_DEFINITION_DISABLED_REASON,
);
this.setWarningText(
valid
? null
: Msg['PROCEDURES_CALL_DISABLED_DEF_WARNING'].replace('%1', name),
);
Events.setGroup(oldGroup);
}
}
Expand Down Expand Up @@ -1181,7 +1191,6 @@ blocks['procedures_callnoreturn'] = {
this.argumentVarModels_ = [];
this.quarkConnections_ = {};
this.quarkIds_ = null;
this.previousEnabledState_ = true;
},

defType_: 'procedures_defnoreturn',
Expand All @@ -1202,7 +1211,6 @@ blocks['procedures_callreturn'] = {
this.argumentVarModels_ = [];
this.quarkConnections_ = {};
this.quarkIds_ = null;
this.previousEnabledState_ = true;
},

defType_: 'procedures_defreturn',
Expand All @@ -1219,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.
Expand Down Expand Up @@ -1279,7 +1293,7 @@ const PROCEDURES_IFRETURN = {
if (
((this.workspace as WorkspaceSvg).isDragging &&
(this.workspace as WorkspaceSvg).isDragging()) ||
e.type !== Events.BLOCK_MOVE
(e.type !== Events.BLOCK_MOVE && e.type !== Events.BLOCK_CREATE)
) {
return; // Don't change state at the start of a drag.
}
Expand Down Expand Up @@ -1319,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.setEnabled(legal);
this.setDisabledReason(!legal, UNPARENTED_IFRETURN_DISABLED_REASON);
Events.setGroup(group);
}
},
Expand Down
114 changes: 97 additions & 17 deletions core/block.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import {ConnectionType} from './connection_type.js';
import * as constants from './constants.js';
import {DuplicateIconType} from './icons/exceptions.js';
import type {Abstract} from './events/events_abstract.js';
import type {BlockChange} from './events/events_block_change.js';
import type {BlockMove} from './events/events_block_move.js';
import * as deprecation from './utils/deprecation.js';
import * as eventUtils from './events/utils.js';
import * as Extensions from './extensions.js';
import type {Field} from './field.js';
Expand Down Expand Up @@ -166,7 +168,7 @@ export class Block implements IASTNodeLocation {
inputList: Input[] = [];
inputsInline?: boolean;
icons: IIcon[] = [];
private disabled = false;
private disabledReasons = new Set<string>();
tooltip: Tooltip.TipInfo = '';
contextMenu = true;

Expand Down Expand Up @@ -1390,32 +1392,89 @@ export class Block implements IASTNodeLocation {
}

/**
* Get whether this block is enabled or not.
* Get whether this block is enabled or not. A block is considered enabled
* if there aren't any reasons why it would be disabled. A block may still
* be disabled for other reasons even if the user attempts to manually
* enable it, such as when the block is in an invalid location.
*
* @returns True if enabled.
*/
isEnabled(): boolean {
return !this.disabled;
return this.disabledReasons.size === 0;
}

/** @deprecated v11 - Get whether the block is manually disabled. */
private get disabled(): boolean {
deprecation.warn(
'disabled',
'v11',
'v12',
'the isEnabled or hasDisabledReason methods of Block',
);
return this.hasDisabledReason(constants.MANUALLY_DISABLED);
}

/** @deprecated v11 - Set whether the block is manually disabled. */
private set disabled(value: boolean) {
deprecation.warn(
'disabled',
'v11',
'v12',
'the setDisabledReason method of Block',
);
this.setDisabledReason(value, constants.MANUALLY_DISABLED);
}

/**
* Set whether the block is enabled or not.
* @deprecated v11 - Set whether the block is manually enabled or disabled.
* The user can toggle whether a block is disabled from a context menu
* option. A block may still be disabled for other reasons even if the user
* attempts to manually enable it, such as when the block is in an invalid
* location. This method is deprecated and setDisabledReason should be used
* instead.
*
* @param enabled True if enabled.
*/
setEnabled(enabled: boolean) {
if (this.isEnabled() !== enabled) {
const oldValue = this.disabled;
this.disabled = !enabled;
eventUtils.fire(
new (eventUtils.get(eventUtils.BLOCK_CHANGE))(
this,
'disabled',
null,
oldValue,
!enabled,
),
);
deprecation.warn(
'setEnabled',
'v11',
'v12',
'the setDisabledReason method of Block',
);
this.setDisabledReason(!enabled, constants.MANUALLY_DISABLED);
}

/**
* Add or remove a reason why the block might be disabled. If a block has
* any reasons to be disabled, then the block itself will be considered
* disabled. A block could be disabled for multiple independent reasons
* simultaneously, such as when the user manually disables it, or the block
* is invalid.
*
* @param disabled If true, then the block should be considered disabled for
* at least the provided reason, otherwise the block is no longer disabled
* for that reason.
* @param reason A language-neutral identifier for a reason why the block
* could be disabled. Call this method again with the same identifier to
* update whether the block is currently disabled for this reason.
*/
setDisabledReason(disabled: boolean, reason: string): void {
if (this.disabledReasons.has(reason) !== disabled) {
if (disabled) {
this.disabledReasons.add(reason);
} else {
this.disabledReasons.delete(reason);
}
const blockChangeEvent = new (eventUtils.get(eventUtils.BLOCK_CHANGE))(
this,
'disabled',
/* name= */ null,
/* oldValue= */ !disabled,
/* newValue= */ disabled,
) as BlockChange;
blockChangeEvent.setDisabledReason(reason);
eventUtils.fire(blockChangeEvent);
}
}

Expand All @@ -1428,7 +1487,7 @@ export class Block implements IASTNodeLocation {
getInheritedDisabled(): boolean {
let ancestor = this.getSurroundParent();
while (ancestor) {
if (ancestor.disabled) {
if (!ancestor.isEnabled()) {
return true;
}
ancestor = ancestor.getSurroundParent();
Expand All @@ -1437,6 +1496,27 @@ export class Block implements IASTNodeLocation {
return false;
}

/**
* Get whether the block is currently disabled for the provided reason.
*
* @param reason A language-neutral identifier for a reason why the block
* could be disabled.
* @returns Whether the block is disabled for the provided reason.
*/
hasDisabledReason(reason: string): boolean {
return this.disabledReasons.has(reason);
}

/**
* Get a set of reasons why the block is currently disabled, if any. If the
* block is enabled, this set will be empty.
*
* @returns The set of reasons why the block is disabled, if any.
*/
getDisabledReasons(): ReadonlySet<string> {
return this.disabledReasons;
}

/**
* Get whether the block is collapsed or not.
*
Expand Down
45 changes: 39 additions & 6 deletions core/block_svg.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
LegacyContextMenuOption,
} from './contextmenu_registry.js';
import type {BlockMove} from './events/events_block_move.js';
import * as deprecation from './utils/deprecation.js';
import * as eventUtils from './events/utils.js';
import type {Field} from './field.js';
import {FieldLabel} from './field_label.js';
Expand Down Expand Up @@ -985,16 +986,48 @@ export class BlockSvg
}

/**
* Set whether the block is enabled or not.
* @deprecated v11 - Set whether the block is manually enabled or disabled.
* The user can toggle whether a block is disabled from a context menu
* option. A block may still be disabled for other reasons even if the user
* attempts to manually enable it, such as when the block is in an invalid
* location. This method is deprecated and setDisabledReason should be used
* instead.
*
* @param enabled True if enabled.
*/
override setEnabled(enabled: boolean) {
if (this.isEnabled() !== enabled) {
super.setEnabled(enabled);
if (!this.getInheritedDisabled()) {
this.updateDisabled();
}
deprecation.warn(
'setEnabled',
'v11',
'v12',
'the setDisabledReason method of BlockSvg',
);
const wasEnabled = this.isEnabled();
super.setEnabled(enabled);
if (this.isEnabled() !== wasEnabled && !this.getInheritedDisabled()) {
this.updateDisabled();
}
}

/**
* Add or remove a reason why the block might be disabled. If a block has
* any reasons to be disabled, then the block itself will be considered
* disabled. A block could be disabled for multiple independent reasons
* simultaneously, such as when the user manually disables it, or the block
* is invalid.
*
* @param disabled If true, then the block should be considered disabled for
* at least the provided reason, otherwise the block is no longer disabled
* for that reason.
* @param reason A language-neutral identifier for a reason why the block
* could be disabled. Call this method again with the same identifier to
* update whether the block is currently disabled for this reason.
*/
override setDisabledReason(disabled: boolean, reason: string): void {
const wasEnabled = this.isEnabled();
super.setDisabledReason(disabled, reason);
if (this.isEnabled() !== wasEnabled && !this.getInheritedDisabled()) {
this.updateDisabled();
}
}

Expand Down
6 changes: 6 additions & 0 deletions core/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,9 @@ export const COLLAPSED_INPUT_NAME = '_TEMP_COLLAPSED_INPUT';
* The language-neutral ID given to the collapsed field.
*/
export const COLLAPSED_FIELD_NAME = '_TEMP_COLLAPSED_FIELD';

/**
* The language-neutral ID for when the reason why a block is disabled is
* because the user manually disabled it, such as via the context menu.
*/
export const MANUALLY_DISABLED = 'MANUALLY_DISABLED';
Loading

0 comments on commit cee7f91

Please sign in to comment.