Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
import { CoreEditor, ToolName } from 'application/editor';
import { ReactionArrow } from 'application/editor/tools/ReactionArrow';
import { RxnArrowResizeOperation } from 'application/editor/operations/coreRxn/rxnArrow';
import { RxnArrow } from 'domain/entities/CoreRxnArrow';
import { RxnArrowMode } from 'domain/entities/rxnArrow';
import { Vec2 } from 'domain/entities/vec2';
import type { DrawingEntitiesManager } from 'domain/entities/DrawingEntitiesManager';
import {
createPolymerEditorCanvas,
createRenderersManager,
} from '../../../../helpers/dom';

describe('RxnArrowResizeOperation', () => {
let renderersManager: ReturnType<typeof createRenderersManager>;

beforeEach(() => {
createPolymerEditorCanvas();
renderersManager = createRenderersManager();
});

it('executes and inverts end resize', () => {
const arrow = new RxnArrow(RxnArrowMode.OpenAngle, [
new Vec2(0, 0),
new Vec2(1, 0),
]);
const previousPosition = new Vec2(1, 0);
const newPosition = new Vec2(2, 1);
const operation = new RxnArrowResizeOperation(
arrow,
1,
newPosition,
previousPosition,
);

operation.execute(renderersManager);
expect(arrow.endPosition).toEqual(newPosition);

operation.invert(renderersManager);
expect(arrow.endPosition).toEqual(previousPosition);
});
});

describe('DrawingEntitiesManager.resizeRxnArrow', () => {
let editor: CoreEditor;
let drawingEntitiesManager: DrawingEntitiesManager;

beforeEach(() => {
const canvas = createPolymerEditorCanvas();
editor = new CoreEditor({
theme: {},
canvas,
renderersContainer: createRenderersManager(),
});
drawingEntitiesManager = editor.drawingEntitiesManager;
});

afterEach(() => {
editor.destroy();
});

it('snaps arrow end to horizontal axis', () => {
const addCommand = drawingEntitiesManager.addRxnArrow(
RxnArrowMode.OpenAngle,
[new Vec2(0, 0), new Vec2(1, 0)],
);
addCommand.execute(editor.renderersContainer);
const arrow = drawingEntitiesManager.rxnArrows.values().next().value;
const almostHorizontalEnd = new Vec2(2, 0.05);

const resizeCommand = drawingEntitiesManager.resizeRxnArrow(
arrow,
1,
almostHorizontalEnd,
true,
);
resizeCommand.execute(editor.renderersContainer);

expect(arrow.endPosition.y).toBe(0);
expect(arrow.endPosition.x).toBeGreaterThan(1.99);
});

it('does not snap when snapping is disabled', () => {
const addCommand = drawingEntitiesManager.addRxnArrow(
RxnArrowMode.OpenAngle,
[new Vec2(0, 0), new Vec2(1, 0)],
);
addCommand.execute(editor.renderersContainer);
const arrow = drawingEntitiesManager.rxnArrows.values().next().value;
const unsnappedEnd = new Vec2(2, 0.05);

const resizeCommand = drawingEntitiesManager.resizeRxnArrow(
arrow,
1,
unsnappedEnd,
false,
);
resizeCommand.execute(editor.renderersContainer);

expect(arrow.endPosition).toEqual(unsnappedEnd);
});
});

describe('ReactionArrow creation tool', () => {
let editor: CoreEditor;
let drawingEntitiesManager: DrawingEntitiesManager;

beforeEach(() => {
const canvas = createPolymerEditorCanvas();
editor = new CoreEditor({
theme: {},
canvas,
renderersContainer: createRenderersManager(),
});
drawingEntitiesManager = editor.drawingEntitiesManager;
});

afterEach(() => {
editor.destroy();
});

const createTool = () =>
new ReactionArrow(editor, {
toolName: ToolName.reactionArrow,
mode: RxnArrowMode.OpenAngle,
});

const getArrows = () => [...drawingEntitiesManager.rxnArrows.values()];

it('creates a single horizontal arrow of default length on click', () => {
editor.lastCursorPositionOfCanvas = new Vec2(100, 100);
const tool = createTool();

tool.mousedown();
tool.mouseup();

const arrows = getArrows();
expect(arrows).toHaveLength(1);

const arrow = arrows[0];
expect(arrow.endPosition.x - arrow.startPosition.x).toBeCloseTo(
ReactionArrow.DEFAULT_LENGTH,
);
expect(arrow.endPosition.y).toBeCloseTo(arrow.startPosition.y);
});

it('creates an arrow following the drag length and direction', () => {
editor.lastCursorPositionOfCanvas = new Vec2(100, 100);
const tool = createTool();

tool.mousedown();
editor.lastCursorPositionOfCanvas = new Vec2(300, 100);
tool.mousemove({ ctrlKey: false } as MouseEvent);
tool.mouseup();

const arrows = getArrows();
expect(arrows).toHaveLength(1);

const arrow = arrows[0];
expect(arrow.endPosition.x).toBeGreaterThan(arrow.startPosition.x);
expect(Vec2.dist(arrow.startPosition, arrow.endPosition)).toBeGreaterThan(
ReactionArrow.MIN_LENGTH,
);
});

it('removes the in-progress arrow when the tool is destroyed mid-drag', () => {
editor.lastCursorPositionOfCanvas = new Vec2(100, 100);
const tool = createTool();

tool.mousedown();
editor.lastCursorPositionOfCanvas = new Vec2(300, 100);
tool.mousemove({ ctrlKey: false } as MouseEvent);

expect(getArrows()).toHaveLength(1);

tool.destroy();

expect(getArrows()).toHaveLength(0);
});
});
8 changes: 6 additions & 2 deletions packages/ketcher-core/src/application/editor/Editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1665,8 +1665,12 @@ export class CoreEditor {
}
}

public onSelectTool(tool: ToolName, options?: object) {
this.selectTool(tool, options);
public onSelectTool(
tool: ToolName | string,
options?: { toolName?: ToolName },
) {
const actualTool = (options?.toolName ?? tool) as ToolName;
this.selectTool(actualTool, options);
Comment on lines +1668 to +1673
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Widening tool to ToolName | string plus the as ToolName cast removes the only compile-time check that callers pass a valid tool key. Existing call sites in EditorEvents.tsx and TopMenuComponent.tsx already pass plain string literals as name, which previously had to match ToolName to typecheck — now any typo silently passes through and lands as toolsMap[undefined] at runtime.

If the intent is "first arg is the menu item id, options.toolName is the actual tool", consider keeping the type signature tight: onSelectTool(menuItemId: string, options?: { toolName: ToolName }): void with an explicit runtime guard (e.g. if (!(actualTool in toolsMap)) return;) before selectTool. That makes the indirection visible at the type level and avoids the cast.

}

private onCreateBond(payload: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import type { RenderersManager } from 'application/render/renderers/RenderersManager';
import type { Operation } from 'domain/entities/Operation';
import type { RxnArrow } from 'domain/entities/CoreRxnArrow';
import type { Vec2 } from 'domain/entities/vec2';

export class RxnArrowAddOperation implements Operation {
public rxnArrow: RxnArrow;
Expand Down Expand Up @@ -61,3 +62,26 @@ export class RxnArrowDeleteOperation implements Operation {
renderersManager.addRxnArrow(this.rxnArrow);
}
}

export class RxnArrowResizeOperation implements Operation {
public priority = 2;

constructor(
public rxnArrow: RxnArrow,
public endIndex: 0 | 1,
public newPosition: Vec2,
public previousPosition: Vec2,
) {}

public execute(renderersManager: RenderersManager) {
this.rxnArrow.resize(this.endIndex, this.newPosition);
renderersManager.deleteRxnArrow(this.rxnArrow);
renderersManager.addRxnArrow(this.rxnArrow);
}

public invert(renderersManager: RenderersManager) {
this.rxnArrow.resize(this.endIndex, this.previousPosition);
renderersManager.deleteRxnArrow(this.rxnArrow);
renderersManager.addRxnArrow(this.rxnArrow);
}
Comment on lines +76 to +86
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perf: this fires on every drag mousemove and fully destroys/recreates the renderer.

deleteRxnArrow + addRxnArrow tears down the root <g>, hover, selection, and the end-handle groups, then runs the full show() pipeline (generateArrowPath, up to 4 <path> inserts, appendHoverAreaElement with svgpath rotation, drawSelection, drawEndHandles). At ~60 mousemoves/s during a resize drag this is dozens of DOM ops per frame.

A targeted update path — updating d on the existing paths and the root transform in place — would be roughly 5× cheaper and would also fix the end-handle flicker on every move. The existing RxnArrowRenderer.move() is closer to what's needed but still does remove(); show();.

Not blocking, but worth a follow-up given how visible this is on busy canvases.

}
136 changes: 136 additions & 0 deletions packages/ketcher-core/src/application/editor/tools/ReactionArrow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import type { CoreEditor } from 'application/editor/Editor';
import { EditorHistory } from 'application/editor/internal';
import type { BaseTool } from 'application/editor/tools/Tool';
import { Coordinates } from 'application/editor/shared/coordinates';
import type { RxnArrow } from 'domain/entities/CoreRxnArrow';
import type { RxnArrowMode } from 'domain/entities/rxnArrow';
import { Vec2 } from 'domain/entities/vec2';
import type { ToolName } from 'application/editor/tools/types';
import assert from 'assert';

class ReactionArrow implements BaseTool {
static readonly MIN_LENGTH = 0.5;
static readonly DEFAULT_LENGTH = 1;

private readonly history: EditorHistory;
private readonly mode: RxnArrowMode;
private p0: Vec2 | null = null;
private arrow?: RxnArrow;
private isDragging = false;

constructor(
private readonly editor: CoreEditor,
options: { toolName: ToolName; mode: RxnArrowMode },
) {
this.history = EditorHistory.getInstance(this.editor);
this.mode = options.mode;
}

public mousedown() {
this.p0 = Coordinates.canvasToModel(this.editor.lastCursorPositionOfCanvas);
this.arrow = undefined;
this.isDragging = false;
}

public mousemove(event: MouseEvent) {
if (!this.p0) {
return;
}

const current = Coordinates.canvasToModel(
this.editor.lastCursorPositionOfCanvas,
);
const diff = Vec2.diff(current, this.p0);

if (diff.length() > 0.01) {
this.isDragging = true;
}

if (!this.arrow) {
const addCommand = this.editor.drawingEntitiesManager.addRxnArrow(
this.mode,
[this.p0, this.p0],
);
addCommand.execute(this.editor.renderersContainer);
this.arrow = addCommand.operations[0].rxnArrow;
}

assert(this.arrow);

const resizeCommand = this.editor.drawingEntitiesManager.resizeRxnArrow(
this.arrow,
1,
current,
!event.ctrlKey,
);
this.editor.renderersContainer.update(resizeCommand);
}

public mouseup() {
if (!this.p0) {
return;
}

if (this.arrow && this.isDragging) {
const end = this.getArrowWithMinimalLengthEnd(
this.p0,
this.arrow.endPosition,
);
const deleteCommand = this.editor.drawingEntitiesManager.deleteRxnArrow(
this.arrow,
);
deleteCommand.execute(this.editor.renderersContainer);

const addCommand = this.editor.drawingEntitiesManager.addRxnArrow(
this.mode,
[this.p0, end],
);
this.history.update(addCommand);
addCommand.execute(this.editor.renderersContainer);
Comment on lines +69 to +89
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: a sub-threshold mousemove leaves a ghost arrow on the canvas with no history entry.

Tracing mousedown → mousemove (diff.length() ≤ 0.01) → mouseup:

  • mousemove (line 35) creates the arrow even when isDragging is not set, because the > 0.01 check only gates isDragging — arrow creation at line 49 is unconditional once p0 exists.
  • mouseup then matches neither branch:
    • this.arrow && this.isDragging → false (isDragging is still false)
    • !this.arrow → false (arrow was created in mousemove)
  • Both p0 and arrow are nulled out, but the previously-added arrow stays in drawingEntitiesManager.rxnArrows and remains rendered — with no entry in history, so it can't be undone.

This is reachable with any small pointer jitter between mousedown and mouseup (high-DPI mouse, pen, trackpad).

Suggest either (a) gating arrow creation in mousemove behind the same > 0.01 threshold as isDragging, or (b) adding a this.arrow && !this.isDragging branch in mouseup that deletes the in-progress arrow and falls through to the click-only path.

} else if (!this.arrow) {
const end = this.getArrowWithMinimalLengthEnd(this.p0, null);
const addCommand = this.editor.drawingEntitiesManager.addRxnArrow(
this.mode,
[this.p0, end],
);
this.history.update(addCommand);
addCommand.execute(this.editor.renderersContainer);
}

this.p0 = null;
this.arrow = undefined;
this.isDragging = false;
}

public destroy() {
if (this.arrow) {
const deleteCommand = this.editor.drawingEntitiesManager.deleteRxnArrow(
this.arrow,
);
this.editor.renderersContainer.update(deleteCommand);
}
this.p0 = null;
this.arrow = undefined;
this.isDragging = false;
}

private getArrowWithMinimalLengthEnd(start: Vec2, end: Vec2 | null): Vec2 {
if (!end) {
return new Vec2(start.x + ReactionArrow.DEFAULT_LENGTH, start.y);
}

const length = Vec2.dist(start, end);

return length < ReactionArrow.MIN_LENGTH
? new Vec2(
Vec2.findSecondPoint(
start,
ReactionArrow.DEFAULT_LENGTH,
Vec2.oxAngleForVector(start, end),
),
)
: end;
}
}

export { ReactionArrow };
3 changes: 3 additions & 0 deletions packages/ketcher-core/src/application/editor/tools/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { HandTool } from 'application/editor/tools/Hand';
import { ToolName } from 'application/editor/tools/types';
import { SelectLasso } from 'application/editor/tools/select/SelectLasso';
import { SelectFragment } from 'application/editor/tools/select/SelectFragment';
import { ReactionArrow } from 'application/editor/tools/ReactionArrow';

export const toolsMap: Record<ToolName, ToolConstructorInterface> = {
[ToolName.monomer]: MonomerTool,
Expand All @@ -36,9 +37,11 @@ export const toolsMap: Record<ToolName, ToolConstructorInterface> = {
[ToolName.erase]: EraserTool,
[ToolName.clear]: ClearTool,
[ToolName.hand]: HandTool,
[ToolName.reactionArrow]: ReactionArrow,
};

export * from './Tool';
export * from './Zoom';
export * from './select';
export * from './rnaPresetConnections';
export * from './reactionArrowConstants';
Loading
Loading