Skip to content

fix!: remove MarkerSvg and uses #8991

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

Merged
rachel-fenichel merged 4 commits into from
May 7, 2025
Merged

fix!: remove MarkerSvg and uses #8991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0d70a9
fix: delete MarkerSvg (marker drawer)
rachel-fenichel May 5, 2025
834a59d
fix: delete marker and cursor SVG elements
rachel-fenichel May 5, 2025
b449343
chore: format
rachel-fenichel May 5, 2025
c8ec2f2
chore: lint
rachel-fenichel May 7, 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
40 changes: 0 additions & 40 deletions core/block_svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ import type {IFocusableTree} from './interfaces/i_focusable_tree.js';
import {IIcon} from './interfaces/i_icon.js';
import type {INavigable} from './interfaces/i_navigable.js';
import * as internalConstants from './internal_constants.js';
import {MarkerManager} from './marker_manager.js';
import {Msg} from './msg.js';
import * as renderManagement from './render_management.js';
import {RenderedConnection} from './rendered_connection.js';
Expand Down Expand Up @@ -1679,7 +1678,6 @@ export class BlockSvg
this.tightenChildrenEfficiently();

dom.stopTextWidthCache();
this.updateMarkers_();
}

/**
Expand All @@ -1699,44 +1697,6 @@ export class BlockSvg
if (this.nextConnection) this.nextConnection.tightenEfficiently();
}

/** Redraw any attached marker or cursor svgs if needed. */
protected updateMarkers_() {
if (this.workspace.keyboardAccessibilityMode && this.pathObject.cursorSvg) {
this.workspace.getCursor()!.draw();
}
if (this.workspace.keyboardAccessibilityMode && this.pathObject.markerSvg) {
// TODO(#4592): Update all markers on the block.
this.workspace.getMarker(MarkerManager.LOCAL_MARKER)!.draw();
}
for (const input of this.inputList) {
for (const field of input.fieldRow) {
field.updateMarkers_();
}
}
}

/**
* Add the cursor SVG to this block's SVG group.
*
* @param cursorSvg The SVG root of the cursor to be added to the block SVG
* group.
* @internal
*/
setCursorSvg(cursorSvg: SVGElement) {
this.pathObject.setCursorSvg(cursorSvg);
}

/**
* Add the marker SVG to this block's SVG group.
*
* @param markerSvg The SVG root of the marker to be added to the block SVG
* group.
* @internal
*/
setMarkerSvg(markerSvg: SVGElement) {
this.pathObject.setMarkerSvg(markerSvg);
}

/**
* Returns a bounding box describing the dimensions of this block
* and any blocks stacked below it.
Expand Down
71 changes: 0 additions & 71 deletions core/field.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import type {IKeyboardAccessible} from './interfaces/i_keyboard_accessible.js';
import type {INavigable} from './interfaces/i_navigable.js';
import type {IRegistrable} from './interfaces/i_registrable.js';
import {ISerializable} from './interfaces/i_serializable.js';
import {MarkerManager} from './marker_manager.js';
import type {ConstantProvider} from './renderers/common/constants.js';
import type {KeyboardShortcut} from './shortcut_registry.js';
import * as Tooltip from './tooltip.js';
Expand Down Expand Up @@ -113,18 +112,6 @@ export abstract class Field<T = any>
private tooltip: Tooltip.TipInfo | null = null;
protected size_: Size;

/**
* Holds the cursors svg element when the cursor is attached to the field.
* This is null if there is no cursor on the field.
*/
private cursorSvg: SVGElement | null = null;

/**
* Holds the markers svg element when the marker is attached to the field.
* This is null if there is no marker on the field.
*/
private markerSvg: SVGElement | null = null;

/** The rendered field's SVG group element. */
protected fieldGroup_: SVGGElement | null = null;

Expand Down Expand Up @@ -1358,64 +1345,6 @@ export abstract class Field<T = any>
return false;
}

/**
* Add the cursor SVG to this fields SVG group.
*
* @param cursorSvg The SVG root of the cursor to be added to the field group.
* @internal
*/
setCursorSvg(cursorSvg: SVGElement) {
if (!cursorSvg) {
this.cursorSvg = null;
return;
}

if (!this.fieldGroup_) {
throw new Error(`The field group is ${this.fieldGroup_}.`);
}
this.fieldGroup_.appendChild(cursorSvg);
this.cursorSvg = cursorSvg;
}

/**
* Add the marker SVG to this fields SVG group.
*
* @param markerSvg The SVG root of the marker to be added to the field group.
* @internal
*/
setMarkerSvg(markerSvg: SVGElement) {
if (!markerSvg) {
this.markerSvg = null;
return;
}

if (!this.fieldGroup_) {
throw new Error(`The field group is ${this.fieldGroup_}.`);
}
this.fieldGroup_.appendChild(markerSvg);
this.markerSvg = markerSvg;
}

/**
* Redraw any attached marker or cursor svgs if needed.
*
* @internal
*/
updateMarkers_() {
const block = this.getSourceBlock();
if (!block) {
throw new UnattachedFieldError();
}
const workspace = block.workspace as WorkspaceSvg;
if (workspace.keyboardAccessibilityMode && this.cursorSvg) {
workspace.getCursor()!.draw();
}
if (workspace.keyboardAccessibilityMode && this.markerSvg) {
// TODO(#4592): Update all markers on the field.
workspace.getMarker(MarkerManager.LOCAL_MARKER)!.draw();
}
}

/** See IFocusableNode.getFocusableElement. */
getFocusableElement(): HTMLElement | SVGElement {
if (!this.fieldGroup_) {
Expand Down
9 changes: 0 additions & 9 deletions core/flyout_button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,15 +348,6 @@ export class FlyoutButton
}
}

/**
* Required by IASTNodeLocationSvg, but not used. A marker cannot be set on a
* button. If the 'mark' shortcut is used on a button, its associated callback
* function is triggered.
*/
setMarkerSvg() {
throw new Error('Attempted to set a marker on a button.');
}

/**
* Do something when the button is clicked.
*
Expand Down
16 changes: 1 addition & 15 deletions core/interfaces/i_ast_node_location_svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,4 @@ import type {IASTNodeLocation} from './i_ast_node_location.js';
/**
* An AST node location SVG interface.
*/
export interface IASTNodeLocationSvg extends IASTNodeLocation {
/**
* Add the marker SVG to this node's SVG group.
*
* @param markerSvg The SVG root of the marker to be added to the SVG group.
*/
setMarkerSvg(markerSvg: SVGElement | null): void;

/**
* Add the cursor SVG to this node's SVG group.
*
* @param cursorSvg The SVG root of the cursor to be added to the SVG group.
*/
setCursorSvg(cursorSvg: SVGElement | null): void;
}
export interface IASTNodeLocationSvg extends IASTNodeLocation {}
Copy link
Collaborator

@cpcallen cpcallen May 7, 2025

Choose a reason for hiding this comment

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

Could this interface not be deleted entirely, now that it is empty?

I suppose we could formally deprecate it first, but since anyone who was calling setFooSvg on an IASTNodeLocationSvg instance already has broken code I'm not sure it's worth it.

Copy link
Collaborator Author

@rachel-fenichel rachel-fenichel May 7, 2025

Choose a reason for hiding this comment

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

Filed as #9006

156 changes: 0 additions & 156 deletions core/keyboard_nav/line_cursor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,8 @@ import {isFocusableNode} from '../interfaces/i_focusable_node.js';
import type {INavigable} from '../interfaces/i_navigable.js';
import * as registry from '../registry.js';
import {RenderedConnection} from '../rendered_connection.js';
import type {MarkerSvg} from '../renderers/common/marker_svg.js';
import type {PathObject} from '../renderers/zelos/path_object.js';
import {Renderer} from '../renderers/zelos/renderer.js';
import * as dom from '../utils/dom.js';
import {WorkspaceSvg} from '../workspace_svg.js';
import {ASTNode} from './ast_node.js';
import {BlockNavigationPolicy} from './block_navigation_policy.js';
import {ConnectionNavigationPolicy} from './connection_navigation_policy.js';
import {FieldNavigationPolicy} from './field_navigation_policy.js';
Expand Down Expand Up @@ -574,40 +570,6 @@ export class LineCursor extends Marker {
return super.getCurNode();
}

/**
* Sets the object in charge of drawing the marker.
*
* We want to customize drawing, so rather than directly setting the given
* object, we instead set a wrapper proxy object that passes through all
* method calls and property accesses except for draw(), which it delegates
* to the drawMarker() method in this class.
*
* @param drawer The object ~in charge of drawing the marker.
*/
override setDrawer(drawer: MarkerSvg) {
const altDraw = function (
this: LineCursor,
oldNode: ASTNode | null,
curNode: ASTNode | null,
) {
// Pass the unproxied, raw drawer object so that drawMarker can call its
// `draw()` method without triggering infinite recursion.
this.drawMarker(oldNode, curNode, drawer);
}.bind(this);

super.setDrawer(
new Proxy(drawer, {
get(target: typeof drawer, prop: keyof typeof drawer) {
if (prop === 'draw') {
return altDraw;
}

return target[prop];
},
}),
);
}

/**
* Set the location of the cursor and draw it.
*
Expand All @@ -631,124 +593,6 @@ export class LineCursor extends Marker {
}
}

/**
* Draw this cursor's marker.
*
* This is a wrapper around this.drawer.draw (usually implemented by
* MarkerSvg.prototype.draw) that will, if newNode is a BLOCK node,
* instead call `setSelected` to select it (if it's a regular block)
* or `addSelect` (if it's a shadow block, since shadow blocks can't
* be selected) instead of using the normal drawer logic.
*
* TODO(#142): The selection and fake-selection code was originally
* a hack added for testing on October 28 2024, because the default
* drawer (MarkerSvg) behaviour in Zelos was to draw a box around
* the block and all attached child blocks, which was confusing when
* navigating stacks.
*
* Since then we have decided that we probably _do_ in most cases
* want navigating to a block to select the block, but more
* particularly that we want navigation to move _focus_. Replace
* this selection hack with non-hacky changing of focus once that's
* possible.
*
* @param oldNode The previous node.
* @param curNode The current node.
* @param realDrawer The object ~in charge of drawing the marker.
*/
private drawMarker(
oldNode: ASTNode | null,
curNode: ASTNode | null,
realDrawer: MarkerSvg,
) {
// If old node was a block, unselect it or remove fake selection.
if (oldNode?.getType() === ASTNode.types.BLOCK) {
const block = oldNode.getLocation() as BlockSvg;
if (!block.isShadow()) {
// Selection should already be in sync.
} else {
block.removeSelect();
}
}

if (this.isZelos && oldNode && this.isValueInputConnection(oldNode)) {
this.hideAtInput(oldNode);
}

const curNodeType = curNode?.getType();
const isZelosInputConnection =
this.isZelos && curNode && this.isValueInputConnection(curNode);

// If drawing can't be handled locally, just use the drawer.
if (curNodeType !== ASTNode.types.BLOCK && !isZelosInputConnection) {
realDrawer.draw(oldNode, curNode);
return;
}

// Hide any visible marker SVG and instead do some manual rendering.
realDrawer.hide();

if (isZelosInputConnection) {
this.showAtInput(curNode);
} else if (curNode && curNodeType === ASTNode.types.BLOCK) {
const block = curNode.getLocation() as BlockSvg;
if (!block.isShadow()) {
// Selection should already be in sync.
} else {
block.addSelect();
block.getParent()?.removeSelect();
}
}

// Call MarkerSvg.prototype.fireMarkerEvent like
// MarkerSvg.prototype.draw would (even though it's private).
(realDrawer as any)?.fireMarkerEvent?.(oldNode, curNode);
}

/**
* Check whether the node represents a value input connection.
*
* @param node The node to check
* @returns True if the node represents a value input connection.
*/
private isValueInputConnection(node: ASTNode) {
if (node?.getType() !== ASTNode.types.INPUT) return false;
const connection = node.getLocation() as RenderedConnection;
return connection.type === ConnectionType.INPUT_VALUE;
}

/**
* Hide the cursor rendering at the given input node.
*
* @param node The input node to hide.
*/
private hideAtInput(node: ASTNode) {
const inputConnection = node.getLocation() as RenderedConnection;
const sourceBlock = inputConnection.getSourceBlock() as BlockSvg;
const input = inputConnection.getParentInput();
if (input) {
const pathObject = sourceBlock.pathObject as PathObject;
const outlinePath = pathObject.getOutlinePath(input.name);
dom.removeClass(outlinePath, 'inputActiveFocus');
}
}

/**
* Show the cursor rendering at the given input node.
*
* @param node The input node to show.
*/
private showAtInput(node: ASTNode) {
const inputConnection = node.getLocation() as RenderedConnection;
const sourceBlock = inputConnection.getSourceBlock() as BlockSvg;
const input = inputConnection.getParentInput();
if (input) {
const pathObject = sourceBlock.pathObject as PathObject;
const outlinePath = pathObject.getOutlinePath(input.name);
dom.addClass(outlinePath, 'inputActiveFocus');
}
}

/**
* Updates the current node to match the selection.
*
Expand Down
Loading