#9394 - Add arrow resizing and redirection on Macromolecules canvas#10144
#9394 - Add arrow resizing and redirection on Macromolecules canvas#10144aslmbk wants to merge 1 commit into
Conversation
aslmbk
force-pushed
the
9394-add-possibility-to-change-arrow-length-and-direction-on-macromolecules-mode
branch
from
1fa39e7 to
0ae335b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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); |
There was a problem hiding this comment.
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 whenisDraggingis not set, because the> 0.01check only gatesisDragging— arrow creation at line 49 is unconditional oncep0exists.mouseupthen matches neither branch:this.arrow && this.isDragging→ false (isDraggingis stillfalse)!this.arrow→ false (arrow was created inmousemove)
- Both
p0andarroware nulled out, but the previously-added arrow stays indrawingEntitiesManager.rxnArrowsand 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.
| 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); | ||
| } |
There was a problem hiding this comment.
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.
| public resizeRxnArrow( | ||
| arrow: RxnArrow, | ||
| endIndex: 0 | 1, | ||
| newPosition: Vec2, | ||
| isSnappingEnabled = true, | ||
| ) { | ||
| const snappedPosition = this.getSnappedArrowEndPosition( | ||
| arrow, | ||
| endIndex, | ||
| newPosition, | ||
| isSnappingEnabled, | ||
| ); | ||
| const previousPosition = new Vec2(arrow.startEndPosition[endIndex]); | ||
| const command = new Command(); | ||
| const operation = new RxnArrowResizeOperation( | ||
| arrow, | ||
| endIndex, | ||
| snappedPosition, | ||
| previousPosition, | ||
| ); | ||
|
|
||
| command.addOperation(operation); | ||
|
|
||
| return command; | ||
| } | ||
|
|
||
| public createRxnArrowResizeHistoryCommand( | ||
| arrow: RxnArrow, | ||
| endIndex: 0 | 1, | ||
| previousPosition: Vec2, | ||
| newPosition: Vec2, | ||
| ) { | ||
| const command = new Command(); | ||
| const operation = new RxnArrowResizeOperation( | ||
| arrow, | ||
| endIndex, | ||
| new Vec2(newPosition), | ||
| new Vec2(previousPosition), | ||
| ); | ||
|
|
||
| command.addOperation(operation); | ||
|
|
||
| return command; | ||
| } |
There was a problem hiding this comment.
API confusion: two near-identical public methods produce the same Command.
resizeRxnArrow and createRxnArrowResizeHistoryCommand both wrap a single RxnArrowResizeOperation. The actual semantic differences are:
resizeRxnArrow: applies snapping; readspreviousPositionfrom the live arrow.createRxnArrowResizeHistoryCommand: takes both positions explicitly; no snapping.
The "history" in the second name is misleading — neither method touches history; callers in SelectBase and ReactionArrow.ts decide whether to push to history.update(...). The real contract is "no snapping + caller-supplied previousPosition", which the name doesn't convey, and the next maintainer is likely to pick the wrong one.
Suggest one of:
- Collapse to a single method with an options arg:
resizeRxnArrow(arrow, endIndex, newPosition, { snap?: boolean; previousPosition?: Vec2 }). - Rename to clearer pair, e.g.
resizeRxnArrowSnapped/resizeRxnArrowExplicit.
A short JSDoc on each (including that endIndex: 0 = start, 1 = end — currently undocumented anywhere) would also help.
| public onSelectTool( | ||
| tool: ToolName | string, | ||
| options?: { toolName?: ToolName }, | ||
| ) { | ||
| const actualTool = (options?.toolName ?? tool) as ToolName; | ||
| this.selectTool(actualTool, options); |
There was a problem hiding this comment.
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.
| {REACTION_ARROW_MENU_ITEMS.map(({ itemId }) => ( | ||
| <Menu.Item | ||
| key={itemId} | ||
| itemId={itemId as IconName} | ||
| title={itemId | ||
| .replace('reaction-arrow-', '') | ||
| .replace(/-/g, ' ') | ||
| .replace(/\b\w/g, (char) => char.toUpperCase())} | ||
| testId={itemId} | ||
| disabled={isSequenceMode} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Every other Menu.Item in this file has a hand-authored title with a hotkey suffix (e.g. Erase (${hotkeysShortcuts.erase})). The arrow items derive title from the kebab-case id via chained .replace calls, which:
- Implicitly couples the user-visible label to the internal id (renaming the id silently changes UI text).
- Produces labels like
Unbalanced Equilibrium Filled Half Bow— readable, but not consistent with the labels surrounding it. - Bakes in English casing, so it won't fit through an i18n layer cleanly if one is added later.
Suggest adding an explicit title next to itemId/mode in REACTION_ARROW_ITEM_ID_TO_MODE (or a parallel record) and reading it here.
|
Reviewed the diff across code quality, performance, documentation, and security. Security: nothing noteworthy — SVG is built via D3 Positives
Prioritized items (all left as inline comments)
Also worth a quick pass: the |
2 participants
Arrows on the Macromolecules canvas previously could only be displayed (loaded
from KET / converted from Molecules) — they could not be created, resized or
redirected. This PR brings arrow editing to the Macromolecules canvas in the same
manner as on the Molecules canvas.
Changes
types (
reaction-arrow-*), wired throughToolName.reactionArrow+mode.visible while the arrow is selected.
snapping enabled by default (hold Ctrl to disable).
RxnArrow.resize,RxnArrowResizeOperation,DrawingEntitiesManager.resizeRxnArrowwith full undo/redo support.Out of scope
ends are resized for all arrow types, per the issue ("length and direction").
How to test
click (default length) or drag (custom length/direction) on the canvas.
direction; hold Ctrl to disable angle snapping.
Tests
RxnArrowResizeOperation(execute/invert), snapping behaviour,and the
ReactionArrowcreation tool (click / drag / cancel).Check list
#1234 – issue name