From baea47c49355fca241233e3c157fea6adebeae9c Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Thu, 31 Jul 2025 18:02:08 -0700 Subject: [PATCH] Extract selection filtering logic to useSelectedLiteGraphItems composable and don't show toolbox when selecting Reroutes (#4634) --- src/components/graph/SelectionOverlay.vue | 11 +- .../canvas/useSelectedLiteGraphItems.ts | 71 ++++++ .../canvas/useSelectedLiteGraphItems.test.ts | 216 ++++++++++++++++++ 3 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 src/composables/canvas/useSelectedLiteGraphItems.ts create mode 100644 tests-ui/tests/composables/canvas/useSelectedLiteGraphItems.test.ts diff --git a/src/components/graph/SelectionOverlay.vue b/src/components/graph/SelectionOverlay.vue index aa1f11c76..d998dad06 100644 --- a/src/components/graph/SelectionOverlay.vue +++ b/src/components/graph/SelectionOverlay.vue @@ -17,26 +17,28 @@ import { createBounds } from '@comfyorg/litegraph' import { whenever } from '@vueuse/core' import { ref, watch } from 'vue' +import { useSelectedLiteGraphItems } from '@/composables/canvas/useSelectedLiteGraphItems' import { useAbsolutePosition } from '@/composables/element/useAbsolutePosition' import { useCanvasStore } from '@/stores/graphStore' const canvasStore = useCanvasStore() const { style, updatePosition } = useAbsolutePosition() +const { getSelectableItems } = useSelectedLiteGraphItems() const visible = ref(false) const showBorder = ref(false) const positionSelectionOverlay = () => { - const { selectedItems } = canvasStore.getCanvas() - showBorder.value = selectedItems.size > 1 + const selectableItems = getSelectableItems() + showBorder.value = selectableItems.size > 1 - if (!selectedItems.size) { + if (!selectableItems.size) { visible.value = false return } visible.value = true - const bounds = createBounds(selectedItems) + const bounds = createBounds(selectableItems) if (bounds) { updatePosition({ pos: [bounds[0], bounds[1]], @@ -45,7 +47,6 @@ const positionSelectionOverlay = () => { } } -// Register listener on canvas creation. whenever( () => canvasStore.getCanvas().state.selectionChanged, () => { diff --git a/src/composables/canvas/useSelectedLiteGraphItems.ts b/src/composables/canvas/useSelectedLiteGraphItems.ts new file mode 100644 index 000000000..3d44fe089 --- /dev/null +++ b/src/composables/canvas/useSelectedLiteGraphItems.ts @@ -0,0 +1,71 @@ +import { Positionable, Reroute } from '@comfyorg/litegraph' + +import { useCanvasStore } from '@/stores/graphStore' + +/** + * Composable for handling selected LiteGraph items filtering and operations. + * This provides utilities for working with selected items on the canvas, + * including filtering out items that should not be included in selection operations. + */ +export function useSelectedLiteGraphItems() { + const canvasStore = useCanvasStore() + + /** + * Items that should not show in the selection overlay are ignored. + * @param item - The item to check. + * @returns True if the item should be ignored, false otherwise. + */ + const isIgnoredItem = (item: Positionable): boolean => { + return item instanceof Reroute + } + + /** + * Filter out items that should not show in the selection overlay. + * @param items - The Set of items to filter. + * @returns The filtered Set of items. + */ + const filterSelectableItems = ( + items: Set + ): Set => { + const result = new Set() + for (const item of items) { + if (!isIgnoredItem(item)) { + result.add(item) + } + } + return result + } + + /** + * Get the filtered selected items from the canvas. + * @returns The filtered Set of selected items. + */ + const getSelectableItems = (): Set => { + const { selectedItems } = canvasStore.getCanvas() + return filterSelectableItems(selectedItems) + } + + /** + * Check if there are any selectable items. + * @returns True if there are selectable items, false otherwise. + */ + const hasSelectableItems = (): boolean => { + return getSelectableItems().size > 0 + } + + /** + * Check if there are multiple selectable items. + * @returns True if there are multiple selectable items, false otherwise. + */ + const hasMultipleSelectableItems = (): boolean => { + return getSelectableItems().size > 1 + } + + return { + isIgnoredItem, + filterSelectableItems, + getSelectableItems, + hasSelectableItems, + hasMultipleSelectableItems + } +} diff --git a/tests-ui/tests/composables/canvas/useSelectedLiteGraphItems.test.ts b/tests-ui/tests/composables/canvas/useSelectedLiteGraphItems.test.ts new file mode 100644 index 000000000..4bbbc906b --- /dev/null +++ b/tests-ui/tests/composables/canvas/useSelectedLiteGraphItems.test.ts @@ -0,0 +1,216 @@ +import { Positionable, Reroute } from '@comfyorg/litegraph' +import { createPinia, setActivePinia } from 'pinia' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { useSelectedLiteGraphItems } from '@/composables/canvas/useSelectedLiteGraphItems' +import { useCanvasStore } from '@/stores/graphStore' + +// Mock the litegraph module +vi.mock('@comfyorg/litegraph', () => ({ + Reroute: class Reroute { + constructor() {} + } +})) + +// Mock Positionable objects +// @ts-expect-error - Mock implementation for testing +class MockNode implements Positionable { + pos: [number, number] + size: [number, number] + + constructor( + pos: [number, number] = [0, 0], + size: [number, number] = [100, 100] + ) { + this.pos = pos + this.size = size + } +} + +class MockReroute extends Reroute implements Positionable { + // @ts-expect-error - Override for testing + override pos: [number, number] + size: [number, number] + + constructor( + pos: [number, number] = [0, 0], + size: [number, number] = [20, 20] + ) { + // @ts-expect-error - Mock constructor + super() + this.pos = pos + this.size = size + } +} + +describe('useSelectedLiteGraphItems', () => { + let canvasStore: ReturnType + let mockCanvas: any + + beforeEach(() => { + setActivePinia(createPinia()) + canvasStore = useCanvasStore() + + // Mock canvas with selectedItems Set + mockCanvas = { + selectedItems: new Set() + } + + // Mock getCanvas to return our mock canvas + vi.spyOn(canvasStore, 'getCanvas').mockReturnValue(mockCanvas) + }) + + describe('isIgnoredItem', () => { + it('should return true for Reroute instances', () => { + const { isIgnoredItem } = useSelectedLiteGraphItems() + const reroute = new MockReroute() + expect(isIgnoredItem(reroute)).toBe(true) + }) + + it('should return false for non-Reroute items', () => { + const { isIgnoredItem } = useSelectedLiteGraphItems() + const node = new MockNode() + // @ts-expect-error - Test mock + expect(isIgnoredItem(node)).toBe(false) + }) + }) + + describe('filterSelectableItems', () => { + it('should filter out Reroute items', () => { + const { filterSelectableItems } = useSelectedLiteGraphItems() + const node1 = new MockNode([0, 0]) + const node2 = new MockNode([100, 100]) + const reroute = new MockReroute([50, 50]) + + // @ts-expect-error - Test mocks + const items = new Set([node1, node2, reroute]) + const filtered = filterSelectableItems(items) + + expect(filtered.size).toBe(2) + // @ts-expect-error - Test mocks + expect(filtered.has(node1)).toBe(true) + // @ts-expect-error - Test mocks + expect(filtered.has(node2)).toBe(true) + expect(filtered.has(reroute)).toBe(false) + }) + + it('should return empty set when all items are ignored', () => { + const { filterSelectableItems } = useSelectedLiteGraphItems() + const reroute1 = new MockReroute([0, 0]) + const reroute2 = new MockReroute([50, 50]) + + const items = new Set([reroute1, reroute2]) + const filtered = filterSelectableItems(items) + + expect(filtered.size).toBe(0) + }) + + it('should handle empty set', () => { + const { filterSelectableItems } = useSelectedLiteGraphItems() + const items = new Set() + const filtered = filterSelectableItems(items) + + expect(filtered.size).toBe(0) + }) + }) + + describe('methods', () => { + it('getSelectableItems should return only non-ignored items', () => { + const { getSelectableItems } = useSelectedLiteGraphItems() + const node1 = new MockNode() + const node2 = new MockNode() + const reroute = new MockReroute() + + mockCanvas.selectedItems.add(node1) + mockCanvas.selectedItems.add(node2) + mockCanvas.selectedItems.add(reroute) + + const selectableItems = getSelectableItems() + expect(selectableItems.size).toBe(2) + // @ts-expect-error - Test mock + expect(selectableItems.has(node1)).toBe(true) + // @ts-expect-error - Test mock + expect(selectableItems.has(node2)).toBe(true) + expect(selectableItems.has(reroute)).toBe(false) + }) + + it('hasSelectableItems should be true when there are selectable items', () => { + const { hasSelectableItems } = useSelectedLiteGraphItems() + const node = new MockNode() + + expect(hasSelectableItems()).toBe(false) + + mockCanvas.selectedItems.add(node) + expect(hasSelectableItems()).toBe(true) + }) + + it('hasSelectableItems should be false when only ignored items are selected', () => { + const { hasSelectableItems } = useSelectedLiteGraphItems() + const reroute = new MockReroute() + + mockCanvas.selectedItems.add(reroute) + expect(hasSelectableItems()).toBe(false) + }) + + it('hasMultipleSelectableItems should be true when there are 2+ selectable items', () => { + const { hasMultipleSelectableItems } = useSelectedLiteGraphItems() + const node1 = new MockNode() + const node2 = new MockNode() + + expect(hasMultipleSelectableItems()).toBe(false) + + mockCanvas.selectedItems.add(node1) + expect(hasMultipleSelectableItems()).toBe(false) + + mockCanvas.selectedItems.add(node2) + expect(hasMultipleSelectableItems()).toBe(true) + }) + + it('hasMultipleSelectableItems should not count ignored items', () => { + const { hasMultipleSelectableItems } = useSelectedLiteGraphItems() + const node = new MockNode() + const reroute1 = new MockReroute() + const reroute2 = new MockReroute() + + mockCanvas.selectedItems.add(node) + mockCanvas.selectedItems.add(reroute1) + mockCanvas.selectedItems.add(reroute2) + + // Even though there are 3 items total, only 1 is selectable + expect(hasMultipleSelectableItems()).toBe(false) + }) + }) + + describe('dynamic behavior', () => { + it('methods should reflect changes when selectedItems change', () => { + const { + getSelectableItems, + hasSelectableItems, + hasMultipleSelectableItems + } = useSelectedLiteGraphItems() + const node1 = new MockNode() + const node2 = new MockNode() + + expect(hasSelectableItems()).toBe(false) + expect(hasMultipleSelectableItems()).toBe(false) + + // Add first node + mockCanvas.selectedItems.add(node1) + expect(hasSelectableItems()).toBe(true) + expect(hasMultipleSelectableItems()).toBe(false) + expect(getSelectableItems().size).toBe(1) + + // Add second node + mockCanvas.selectedItems.add(node2) + expect(hasSelectableItems()).toBe(true) + expect(hasMultipleSelectableItems()).toBe(true) + expect(getSelectableItems().size).toBe(2) + + // Remove a node + mockCanvas.selectedItems.delete(node1) + expect(hasSelectableItems()).toBe(true) + expect(hasMultipleSelectableItems()).toBe(false) + expect(getSelectableItems().size).toBe(1) + }) + }) +})