Skip to content

fix: remove marked node field #273

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.

Sign up for GitHub

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

Closed
microbit-matt-hillsdon wants to merge 2 commits into from
Closed

fix: remove marked node field #273

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d10380
fix: remove marked node field
microbit-matt-hillsdon Mar 4, 2025
fe7c6a6
Merge branch 'main' into simplify-marked-node
microbit-matt-hillsdon Mar 5, 2025
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
84 changes: 16 additions & 68 deletions src/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,6 @@ export class Navigation {
*/
protected passiveFocusIndicator: PassiveFocus = new PassiveFocus();

/**
* The node that has passive focus when the cursor has moved to the flyout
* or toolbox; null if the cursor is moving around the main workspace.
*/
protected markedNode: Blockly.ASTNode | null = null;

/**
* Constructor for keyboard navigation.
*/
Expand Down Expand Up @@ -418,9 +412,12 @@ export class Navigation {
if (!toolbox) {
return;
}
const cursor = workspace.getCursor();
if (cursor) {
this.passiveFocusIndicator.show(cursor.getCurNode());
cursor.hide();
Copy link
Collaborator

@cpcallen cpcallen Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that (contrary to expectations) Marker.prototype.hide does not cause the cursor to be persistently hidden, but only hides the marker SVG, and only until the next time a render occurs.

Marker SVG is used for denoting position of cursor when it is on a connection or on the workspace (or on a whole-stack node, though these are only currently accessible via the context-out shortcut which will probably go away very soon).

So AFAICT this won't hide the cursor if it's on a block, and if it isn't on a block then it will be hidden but may reappear unexpectedly if something causes a render to occur.

Probably not a big deal, given that the existing code does exactly the same thing, though I do wonder if it might be better to hide the cursor before showing the passive focus indicator (here and in focusFlyout).

}

this.markAtCursor(workspace);
workspace.getCursor()?.hide();
this.setState(workspace, Constants.STATE.TOOLBOX);
this.resetFlyout(workspace, false /* shouldHide */);

Expand All @@ -443,8 +440,11 @@ export class Navigation {
* @param workspace The workspace the flyout is on.
*/
focusFlyout(workspace: Blockly.WorkspaceSvg) {
workspace.getCursor()?.hide();
this.markAtCursor(workspace);
const cursor = workspace.getCursor();
if (cursor) {
this.passiveFocusIndicator.show(cursor.getCurNode());
cursor.hide();
Comment on lines +445 to +446
Copy link
Collaborator

@cpcallen cpcallen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still work if you call cursor.hide() before passiveFocusIndicator.show? It would seem more tidy to do it in that order.

}

const flyout = workspace.getFlyout();
this.setState(workspace, Constants.STATE.FLYOUT);
Expand Down Expand Up @@ -482,6 +482,9 @@ export class Navigation {
this.resetFlyout(workspace, reset);
this.setState(workspace, Constants.STATE.WORKSPACE);
this.setCursorOnWorkspaceFocus(workspace, keepCursorPosition);

this.passiveFocusIndicator.hide();
workspace.getCursor()?.draw();
}

/**
Expand All @@ -505,12 +508,6 @@ export class Navigation {
return;
}

if (this.markedNode) {
cursor.setCurNode(this.markedNode);
this.removeMark(workspace);
return;
}

const disposed = cursor.getCurNode()?.getSourceBlock()?.disposed;
if (cursor.getCurNode() && !disposed && keepPosition) {
// Retain the cursor's previous position since it's set, but only if not
Expand Down Expand Up @@ -555,13 +552,14 @@ export class Navigation {
* the block will be placed on.
*/
insertFromFlyout(workspace: Blockly.WorkspaceSvg) {
const stationaryNode = workspace.getCursor()?.getCurNode();
const newBlock = this.createNewBlock(workspace);
if (!newBlock) return;
if (this.markedNode) {
if (stationaryNode) {
if (
!this.tryToConnectNodes(
workspace,
this.markedNode,
stationaryNode,
Blockly.ASTNode.createBlockNode(newBlock)!,
)
) {
Expand All @@ -575,7 +573,6 @@ export class Navigation {
workspace
.getCursor()!
.setCurNode(Blockly.ASTNode.createBlockNode(newBlock)!);
this.removeMark(workspace);
}

/**
Expand Down Expand Up @@ -628,26 +625,6 @@ export class Navigation {
}
}

/**
* Connects the location of the marked node and the location of the cursor.
* No-op if the marked node or cursor node are null.
*
* @param workspace The main workspace.
* @returns True if the cursor and marker locations were connected,
* false otherwise.
*/
connectMarkerAndCursor(workspace: Blockly.WorkspaceSvg): boolean {
const cursorNode = workspace.getCursor()!.getCurNode();

if (this.markedNode && cursorNode) {
if (this.tryToConnectNodes(workspace, this.markedNode, cursorNode)) {
this.removeMark(workspace);
return true;
}
}
return false;
}

/**
* Tries to intelligently connect the blocks or connections
* represented by the given nodes, based on node types and locations.
Expand Down Expand Up @@ -1082,32 +1059,6 @@ export class Navigation {
}
}

/**
* Moves the passive focus indicator to the cursor's current location.
*
* @param workspace The workspace.
*/
markAtCursor(workspace: Blockly.WorkspaceSvg) {
const cursor = workspace.getCursor()!;
this.markedNode = cursor.getCurNode();

// Although it seems like this should never happen, the typings are wrong
// in the base Marker class and this can therefore be null.
if (this.markedNode) {
this.passiveFocusIndicator.show(this.markedNode);
}
}

/**
* Removes the passive focus indicator from its current location and hides it.
*
* @param workspace The workspace.
*/
removeMark(workspace: Blockly.WorkspaceSvg) {
this.passiveFocusIndicator.hide();
this.markedNode = null;
}

/**
* Enables accessibility mode.
*
Expand Down Expand Up @@ -1135,7 +1086,6 @@ export class Navigation {
workspace.keyboardAccessibilityMode
) {
workspace.keyboardAccessibilityMode = false;
this.markAtCursor(workspace);
workspace.getCursor()!.hide();
if (this.getFlyoutCursor(workspace)) {
this.getFlyoutCursor(workspace)!.hide();
Expand Down Expand Up @@ -1250,7 +1200,6 @@ export class Navigation {
* @param workspace The active workspace.
*/
openToolboxOrFlyout(workspace: Blockly.WorkspaceSvg) {
this.markAtCursor(workspace);
if (workspace.getToolbox()) {
this.focusToolbox(workspace);
} else {
Expand Down Expand Up @@ -1390,7 +1339,6 @@ export class Navigation {
Blockly.ASTNode.createBlockNode(block)!,
);
}
this.removeMark(workspace);
return true;
}
Blockly.Events.setGroup(false);
Expand Down