Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Invalid Blocks #7958

Merged
merged 14 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions blocks/loops.ts
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
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
101 changes: 90 additions & 11 deletions core/block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import * as constants from './constants.js';
import {DuplicateIconType} from './icons/exceptions.js';
import type {Abstract} from './events/events_abstract.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 +167,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,30 +1391,87 @@ 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;
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);
}
eventUtils.fire(
new (eventUtils.get(eventUtils.BLOCK_CHANGE))(
this,
'disabled',
null,
oldValue,
!enabled,
reason,
/* oldValue= */ !disabled,
/* newValue= */ disabled,
),
);
}
Expand All @@ -1428,7 +1486,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 +1495,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
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
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