#4694 - Add Hotkeys in Macro mode similar to Micro mode#8895
Conversation
2a243ef to
3b807e5
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds keyboard hotkeys (Delete and Backspace) for deleting monomers on hover in Macro mode, bringing it in line with similar functionality in Micro mode. The implementation introduces a new event subscription system for deleting hovered structures and modifies the entity deletion logic.
Key Changes:
- Adds global keydown event listener to handle Delete/Backspace keys when hovering over monomers
- Introduces
deleteHoveredStructureevent and corresponding handler - Adds
hoveredEntitiesgetter anddeleteHoveredEntities()method to DrawingEntitiesManager
Reviewed changes
Copilot reviewed 5 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| EditorEvents.tsx | Adds useRef for tracking hovered monomer and useEffect with keydown listener to dispatch deletion events |
| DrawingEntitiesManager.ts | Adds hoveredEntities getter and deleteHoveredEntities method; adds guard in deleteSelectedEntities to prevent deletion when entities are hovered |
| Erase.ts | Adds type cast to deleteSelectedEntities return value |
| editorEvents.ts | Adds deleteHoveredStructure event subscription to IEditorEvents interface |
| Editor.ts | Refactors deleteSelectedStructure handler and adds deleteHoveredStructure handler |
| Test snapshots (PNG files) | Updated test snapshots for sequence mode deletion tests |
| ) { | ||
| const modelChanges = | ||
| this.editor.drawingEntitiesManager.deleteSelectedEntities(); | ||
| this.editor.drawingEntitiesManager.deleteSelectedEntities() as Command; |
There was a problem hiding this comment.
The type cast 'as Command' is redundant because deleteSelectedEntities() already returns a Command type. Consider removing this cast for cleaner code.
|
|
||
| window.addEventListener('keydown', onKeyDown); | ||
| return () => window.removeEventListener('keydown', onKeyDown); | ||
| }, [editor, dispatch]); |
There was a problem hiding this comment.
The keydown event listener is missing the 'editor' and 'dispatch' dependencies in the useEffect dependency array. While 'editor' and 'dispatch' are relatively stable, omitting them could lead to stale closures. Consider adding them to the dependency array or documenting why they are intentionally omitted.
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === '1' && editor?.mode.modeName !== 'sequence-layout-mode') { | ||
| dispatch(selectTool('bond-single')); | ||
| } else { | ||
| if (e.key !== 'Delete' && e.key !== 'Backspace') return; | ||
| if (e.ctrlKey || e.metaKey || e.altKey) return; | ||
| const monomer = hoveredTargetRef.current; | ||
| if (!monomer) return; | ||
| if (editor) { | ||
| editor.events.deleteHoveredStructure.dispatch(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The keydown event listener does not check if the user is typing in an input field or text area. This could cause monomers to be deleted while the user is typing "Delete" or "Backspace" in a form field elsewhere in the application. Consider checking if the event target is an input element before processing the deletion.
| this.events.deleteSelectedStructure.add(() => { | ||
| const command = new Command(); | ||
| const history = new EditorHistory(this); | ||
| const commandData = | ||
| this.drawingEntitiesManager.deleteSelectedEntities() as Command; | ||
| command.merge(commandData); | ||
| history.update(command); | ||
| this.renderersContainer.update(command); | ||
| }); |
There was a problem hiding this comment.
The deleteSelectedStructure event handler was completely rewritten and no longer handles SequenceMode separately. The previous implementation included logic to check for SequenceMode and call mode.deleteSelection(). This change may break deletion functionality in Sequence mode. Verify that SequenceMode deletion still works correctly or restore the mode-specific handling.
| const command = new Command(); | ||
| const history = new EditorHistory(this); | ||
| const commandData = | ||
| this.drawingEntitiesManager.deleteSelectedEntities() as Command; |
There was a problem hiding this comment.
The type cast 'as Command' is unnecessary because deleteSelectedEntities() already returns a Command type. This cast can be removed to improve code clarity.
| this.drawingEntitiesManager.deleteSelectedEntities() as Command; | |
| this.drawingEntitiesManager.deleteSelectedEntities(); |
| public deleteHoveredEntities() { | ||
| const mergedCommand = new Command(); | ||
| this.hoveredEntities.forEach(([, drawingEntity]) => { | ||
| const command = this.deleteDrawingEntity(drawingEntity); | ||
| mergedCommand.merge(command); | ||
| }); | ||
| return mergedCommand; | ||
| } |
There was a problem hiding this comment.
The new deleteHoveredEntities method lacks documentation. Consider adding a JSDoc comment explaining its purpose, behavior, and how it differs from deleteSelectedEntities, especially regarding the hover state interactions.
| public get hoveredEntities() { | ||
| return this.allEntities.filter( | ||
| ([, drawingEntity]) => drawingEntity.hovered, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new hoveredEntities getter lacks documentation. Consider adding a JSDoc comment explaining what constitutes a "hovered" entity and how this state is managed, to help future maintainers understand the hover state tracking.
| hoveredTargetRef.current = | ||
| e.target?.__data__?.monomer || | ||
| e.target?.__data__?.node?.monomer || | ||
| null; |
There was a problem hiding this comment.
The hoveredTargetRef is updated on every mouse move event. While using a ref avoids re-renders, consider whether this granular tracking is necessary or if there's a more efficient approach. The current implementation may lead to frequent ref updates during mouse movements across the canvas.
| hoveredTargetRef.current = | |
| e.target?.__data__?.monomer || | |
| e.target?.__data__?.node?.monomer || | |
| null; | |
| const newHoveredTarget = | |
| e.target?.__data__?.monomer || | |
| e.target?.__data__?.node?.monomer || | |
| null; | |
| if (hoveredTargetRef.current !== newHoveredTarget) { | |
| hoveredTargetRef.current = newHoveredTarget; | |
| } |
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === '1' && editor?.mode.modeName !== 'sequence-layout-mode') { | ||
| dispatch(selectTool('bond-single')); | ||
| } else { | ||
| if (e.key !== 'Delete' && e.key !== 'Backspace') return; | ||
| if (e.ctrlKey || e.metaKey || e.altKey) return; | ||
| const monomer = hoveredTargetRef.current; | ||
| if (!monomer) return; | ||
| if (editor) { | ||
| editor.events.deleteHoveredStructure.dispatch(); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('keydown', onKeyDown); | ||
| return () => window.removeEventListener('keydown', onKeyDown); | ||
| }, [editor, dispatch]); | ||
|
|
There was a problem hiding this comment.
The key '1' handling for bond-single tool selection is mixed with the Delete/Backspace handling in the same event listener. Consider separating these concerns into different useEffect hooks for better code organization and maintainability.
| const onKeyDown = (e: KeyboardEvent) => { | |
| if (e.key === '1' && editor?.mode.modeName !== 'sequence-layout-mode') { | |
| dispatch(selectTool('bond-single')); | |
| } else { | |
| if (e.key !== 'Delete' && e.key !== 'Backspace') return; | |
| if (e.ctrlKey || e.metaKey || e.altKey) return; | |
| const monomer = hoveredTargetRef.current; | |
| if (!monomer) return; | |
| if (editor) { | |
| editor.events.deleteHoveredStructure.dispatch(); | |
| } | |
| } | |
| }; | |
| window.addEventListener('keydown', onKeyDown); | |
| return () => window.removeEventListener('keydown', onKeyDown); | |
| }, [editor, dispatch]); | |
| const onKeyDownForBondSingle = (e: KeyboardEvent) => { | |
| if (e.key === '1' && editor?.mode.modeName !== 'sequence-layout-mode') { | |
| dispatch(selectTool('bond-single')); | |
| } | |
| }; | |
| window.addEventListener('keydown', onKeyDownForBondSingle); | |
| return () => window.removeEventListener('keydown', onKeyDownForBondSingle); | |
| }, [editor, dispatch]); | |
| useEffect(() => { | |
| const onKeyDownForDelete = (e: KeyboardEvent) => { | |
| if (e.key !== 'Delete' && e.key !== 'Backspace') return; | |
| if (e.ctrlKey || e.metaKey || e.altKey) return; | |
| const monomer = hoveredTargetRef.current; | |
| if (!monomer) return; | |
| if (editor) { | |
| editor.events.deleteHoveredStructure.dispatch(); | |
| } | |
| }; | |
| window.addEventListener('keydown', onKeyDownForDelete); | |
| return () => window.removeEventListener('keydown', onKeyDownForDelete); | |
| }, [editor]); |
| } | ||
|
|
||
| public deleteSelectedEntities() { | ||
| if (this.hoveredEntities.length !== 0) return; |
There was a problem hiding this comment.
The deleteSelectedEntities method now returns early when there are hovered entities. This creates a logic conflict where selected entities cannot be deleted if any entity happens to be hovered. This is inconsistent with the typical expectation that Delete/Backspace should delete selected items. Consider removing this guard or clarifying the intended behavior when both selection and hover states are present.
| if (this.hoveredEntities.length !== 0) return; |
Erase Operation with (Del) when hovering on monomer Erase Operation with (Backspace) when hovering on monomer
f7a5f88 to
dca01f1
Compare
dca01f1 to
f54fc87
Compare
Erase Operation with (Del) when hovering on monomer Erase Operation with (Backspace) when hovering on monomer
How the feature works? / How did you fix the issue?
(Screenshots, videos, or GIFs, if applicable)
Check list
#1234 – issue name