Skip to content

Select Vue Nodes After Drag #5863

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −17
Open

Select Vue Nodes After Drag #5863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 9 additions & 11 deletions src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle'
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
import { useCanvasInteractions } from '@/renderer/core/canvas/useCanvasInteractions'
import { useNodeZIndex } from '@/renderer/extensions/vueNodes/composables/useNodeZIndex'
import { isLGraphNode } from '@/utils/litegraphUtil'

function useNodeEventHandlersIndividual() {
const canvasStore = useCanvasStore()
Expand All @@ -26,11 +27,7 @@ function useNodeEventHandlersIndividual() {
* Handle node selection events
* Supports single selection and multi-select with Ctrl/Cmd
*/
const handleNodeSelect = (
event: PointerEvent,
nodeData: VueNodeData,
wasDragging: boolean
) => {
const handleNodeSelect = (event: PointerEvent, nodeData: VueNodeData) => {
if (!shouldHandleNodePointerEvents.value) return

if (!canvasStore.canvas || !nodeManager.value) return
Expand All @@ -48,12 +45,13 @@ function useNodeEventHandlersIndividual() {
canvasStore.canvas.select(node)
}
} else {
// If it wasn't a drag: single-select the node
if (!wasDragging) {
const selectedMultipleNodes =
canvasStore.selectedItems.filter((n) => isLGraphNode(n)).length > 1
Comment on lines +48 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can optimize this by just doing a normal for loop and returning early if we hit 2, then move it to existing utils file or into the module scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Long term if we're doing this in multiple places, it might be a candidate for a computed value in the store. Something like canvasStore.selectedNodesCount? (Depending on how expensive the filtering ends up being)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Last time I checked, canvas.selectedItems is not reactive

Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little weird to me. Can we change the selection logic in useNodePointerInteractions instead? I think we'll have to change the interface there anyway since that's where the dragging logic lives.

I would expect the selection to be apparent as I click and drag, not after I release the click.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - I think we should try to match litegraph behavior exactly (unless it's extremely difficult to do).

if (!selectedMultipleNodes) {
// Single-select the node
canvasStore.canvas.deselectAll()
canvasStore.canvas.select(node)
}
// Regular click -> single select
}

// Bring node to front when clicked (similar to LiteGraph behavior)
Expand Down Expand Up @@ -122,7 +120,7 @@ function useNodeEventHandlersIndividual() {
// TODO: add custom double-click behavior here
// For now, ensure node is selected
if (!node.selected) {
handleNodeSelect(event, nodeData, false)
handleNodeSelect(event, nodeData)
}
}

Expand All @@ -143,7 +141,7 @@ function useNodeEventHandlersIndividual() {

// Select the node if not already selected
if (!node.selected) {
handleNodeSelect(event, nodeData, false)
handleNodeSelect(event, nodeData)
}

// Let LiteGraph handle the context menu
Expand All @@ -170,7 +168,7 @@ function useNodeEventHandlersIndividual() {
metaKey: event.metaKey,
bubbles: true
})
handleNodeSelect(syntheticEvent, nodeData, false)
handleNodeSelect(syntheticEvent, nodeData)
}

// Set drag data for potential drop operations
Expand Down
12 changes: 6 additions & 6 deletions tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ describe('useNodeEventHandlers', () => {
metaKey: false
})

handleNodeSelect(event, testNodeData, false)
handleNodeSelect(event, testNodeData)

expect(canvas?.deselectAll).toHaveBeenCalledOnce()
expect(canvas?.select).toHaveBeenCalledWith(mockNode)
Expand All @@ -122,7 +122,7 @@ describe('useNodeEventHandlers', () => {
metaKey: false
})

handleNodeSelect(ctrlClickEvent, testNodeData, false)
handleNodeSelect(ctrlClickEvent, testNodeData)

expect(canvas?.deselectAll).not.toHaveBeenCalled()
expect(canvas?.select).toHaveBeenCalledWith(mockNode)
Expand All @@ -141,7 +141,7 @@ describe('useNodeEventHandlers', () => {
metaKey: false
})

handleNodeSelect(ctrlClickEvent, testNodeData, false)
handleNodeSelect(ctrlClickEvent, testNodeData)

expect(canvas?.deselect).toHaveBeenCalledWith(mockNode)
expect(canvas?.select).not.toHaveBeenCalled()
Expand All @@ -159,7 +159,7 @@ describe('useNodeEventHandlers', () => {
metaKey: true
})

handleNodeSelect(metaClickEvent, testNodeData, false)
handleNodeSelect(metaClickEvent, testNodeData)

expect(canvas?.select).toHaveBeenCalledWith(mockNode)
expect(canvas?.deselectAll).not.toHaveBeenCalled()
Expand All @@ -171,7 +171,7 @@ describe('useNodeEventHandlers', () => {
mockNode!.flags.pinned = false

const event = new PointerEvent('pointerdown')
handleNodeSelect(event, testNodeData, false)
handleNodeSelect(event, testNodeData)

expect(mockLayoutMutations.bringNodeToFront).toHaveBeenCalledWith(
'node-1'
Expand All @@ -184,7 +184,7 @@ describe('useNodeEventHandlers', () => {
mockNode!.flags.pinned = true

const event = new PointerEvent('pointerdown')
handleNodeSelect(event, testNodeData, false)
handleNodeSelect(event, testNodeData)

expect(mockLayoutMutations.bringNodeToFront).not.toHaveBeenCalled()
})
Expand Down
Loading