web: Graphology node drag'n'drop #1236
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @vorburger, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces drag-and-drop functionality for nodes in the graphology visualization. It updates the bun version in .tool-versions from 1.2.4 to 1.2.6, removes the 'Let users drag nodes around' item from ToDo.md, and implements the drag-and-drop feature in web/src/browser/index.ts. The implementation involves event listeners for 'downNode', 'moveBody', 'upNode', and 'upStage' events on the Sigma.js renderer to handle the dragging state and update node positions.
Highlights
- Drag and Drop Implementation: Implements node drag-and-drop functionality using Sigma.js events.
- State Management: Introduces state variables
draggedNodeandisDraggingto manage the drag-and-drop interaction. - Event Handling: Adds event listeners to handle mouse down, move, and up events for node dragging.
- Layout Management: Stops the ForceAtlas2 layout during dragging to prevent conflicts between manual and automatic repositioning.
Changelog
- .tool-versions
- Updated bun version from 1.2.4 to 1.2.6.
- web/ToDo.md
- Removed the 'Let users drag nodes around' item from the ToDo list.
- web/src/browser/index.ts
- Added node size initialization.
- Implemented drag-and-drop functionality for nodes.
- Added event listeners for 'downNode', 'moveBody', 'upNode', and 'upStage' events.
- Introduced state variables
draggedNodeandisDragging. - Stopped ForceAtlas2 layout during dragging.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The first graphical computer game was 'Tennis for Two', created in 1958 by William Higinbotham. It was displayed on an oscilloscope.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces node dragging functionality to the graphology visualization, which is a useful interactive feature. The implementation appears well-structured, using event listeners to manage the dragging state and update node positions. However, there are a few areas that could be improved for clarity and robustness.
Summary of Findings
- Potential memory leak with event listeners: The event listeners added to the renderer might not be properly removed when the component is unmounted or the graph is destroyed, potentially leading to memory leaks. Consider adding a mechanism to remove these listeners when they are no longer needed.
- Custom BBox handling: The logic for setting the custom BBox seems a bit unclear. It's only set if it's not already set, but it's not clear when it would be cleared. This could lead to unexpected behavior if the graph's dimensions change.
Merge Readiness
The pull request introduces a valuable feature and is generally well-implemented. However, addressing the potential memory leak with event listeners and clarifying the custom BBox handling would improve the code's robustness and maintainability. I recommend addressing these points before merging. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
web/src/browser/index.ts
Outdated
| renderer.on("downNode", e => { | ||
| // On mouse down on a node: | ||
| // - Stop layout-ing (required; otherwise manual and automatic re-positioning "compete") | ||
| // - Enable the drag mode | ||
| // - Save in the dragged node in the state | ||
| // - Highlight the node | ||
| // - Disable the camera so its state is not updated | ||
| fa2Layout.stop() | ||
| isDragging = true | ||
| draggedNode = e.node | ||
| graph.setNodeAttribute(draggedNode, "highlighted", true) | ||
| if (!renderer.getCustomBBox()) renderer.setCustomBBox(renderer.getBBox()) | ||
| }) | ||
| renderer.on("moveBody", ({ event }) => { | ||
| // On mouse move, if the drag mode is enabled, we change the position of the draggedNode | ||
| if (!isDragging || !draggedNode) return | ||
|
|
||
| // Get new position of node | ||
| const pos = renderer.viewportToGraph(event) | ||
|
|
||
| graph.setNodeAttribute(draggedNode, "x", pos.x) | ||
| graph.setNodeAttribute(draggedNode, "y", pos.y) | ||
|
|
||
| // Prevent sigma to move camera: | ||
| event.preventSigmaDefault() | ||
| event.original.preventDefault() | ||
| event.original.stopPropagation() | ||
| }) | ||
| const handleUp = () => { | ||
| // On mouse up, we reset the dragging mode | ||
| if (draggedNode) { | ||
| graph.removeNodeAttribute(draggedNode, "highlighted") | ||
| } | ||
| isDragging = false | ||
| draggedNode = null | ||
| } | ||
| renderer.on("upNode", handleUp) | ||
| renderer.on("upStage", handleUp) |
There was a problem hiding this comment.
These event listeners are added to the renderer instance, but it's not clear if they are ever removed. If the component that contains this code is unmounted or the graph is destroyed, these listeners will continue to exist, potentially leading to memory leaks. Consider adding a mechanism to remove these listeners when they are no longer needed, such as using renderer.off() in a cleanup function.
| isDragging = true | ||
| draggedNode = e.node | ||
| graph.setNodeAttribute(draggedNode, "highlighted", true) | ||
| if (!renderer.getCustomBBox()) renderer.setCustomBBox(renderer.getBBox()) |
There was a problem hiding this comment.
It's unclear when renderer.getCustomBBox() would return a value, and thus when the custom BBox would not be set. Consider adding a comment explaining the logic behind this condition, or refactoring to make it more explicit when the custom BBox should be updated.
There was a problem hiding this comment.
I copy/pasted this and don't know either.
web/src/browser/index.ts
Outdated
| const handleUp = () => { | ||
| // On mouse up, we reset the dragging mode | ||
| if (draggedNode) { | ||
| graph.removeNodeAttribute(draggedNode, "highlighted") | ||
| } | ||
| isDragging = false | ||
| draggedNode = null | ||
| } | ||
| renderer.on("upNode", handleUp) | ||
| renderer.on("upStage", handleUp) |
There was a problem hiding this comment.
Consider adding a finally block to the handleUp function to ensure that isDragging and draggedNode are always reset, even if an error occurs within the if (draggedNode) block. This will prevent the dragging state from getting stuck in an inconsistent state.
| const handleUp = () => { | |
| // On mouse up, we reset the dragging mode | |
| if (draggedNode) { | |
| graph.removeNodeAttribute(draggedNode, "highlighted") | |
| } | |
| isDragging = false | |
| draggedNode = null | |
| } | |
| renderer.on("upNode", handleUp) | |
| renderer.on("upStage", handleUp) | |
| const handleUp = () => { | |
| try { | |
| // On mouse up, we reset the dragging mode | |
| if (draggedNode) { | |
| graph.removeNodeAttribute(draggedNode, "highlighted") | |
| } | |
| } finally { | |
| isDragging = false | |
| draggedNode = null | |
| } | |
| } |
I'll document it when I like; Thank You very much.
🖱️ 🐭 🐁 inspired by https://www.sigmajs.org/storybook/?path=/story/mouse-manipulations--story
@lbovet