mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-01-26 19:09:52 +00:00
Fix doubled control application (#7550)
With reactivity fixed, control widgets would apply twice. This is fixed by using the litegraph implementation. Also adds control widget support for combos Followup to #7539. Known Issue: - Primitive node do not have litegraph callbacks properly setup. As a result, they will display an updated value when modified by control widgets. Fixing this will requires a larger, separate PR ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7550-Fix-doubled-control-application-2cb6d73d365081739a2fc40fdfb3630e) by [Unito](https://www.unito.io)
This commit is contained in:
AustinMroz
@@ -1,238 +0,0 @@
|
||||
import { setActivePinia } from 'pinia'
|
||||
import { createTestingPinia } from '@pinia/testing'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { ref } from 'vue'
|
||||
|
||||
import {
|
||||
NumberControlMode,
|
||||
useStepperControl
|
||||
} from '@/renderer/extensions/vueNodes/widgets/composables/useStepperControl'
|
||||
|
||||
// Mock the registry to spy on calls
|
||||
vi.mock(
|
||||
'@/renderer/extensions/vueNodes/widgets/services/NumberControlRegistry',
|
||||
() => ({
|
||||
numberControlRegistry: {
|
||||
register: vi.fn(),
|
||||
unregister: vi.fn(),
|
||||
executeControls: vi.fn(),
|
||||
getControlCount: vi.fn(() => 0),
|
||||
clear: vi.fn()
|
||||
},
|
||||
executeNumberControls: vi.fn()
|
||||
})
|
||||
)
|
||||
|
||||
describe('useStepperControl', () => {
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia())
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('initialization', () => {
|
||||
it('should initialize with RANDOMIZED mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 1 }
|
||||
|
||||
const { controlMode } = useStepperControl(modelValue, options)
|
||||
|
||||
expect(controlMode.value).toBe(NumberControlMode.RANDOMIZE)
|
||||
})
|
||||
|
||||
it('should return control mode and apply function', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 1 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
|
||||
expect(controlMode.value).toBe(NumberControlMode.RANDOMIZE)
|
||||
expect(typeof applyControl).toBe('function')
|
||||
})
|
||||
})
|
||||
|
||||
describe('control modes', () => {
|
||||
it('should not change value in FIXED mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 1 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.FIXED
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(100)
|
||||
})
|
||||
|
||||
it('should increment value in INCREMENT mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 5 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.INCREMENT
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(105)
|
||||
})
|
||||
|
||||
it('should decrement value in DECREMENT mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 5 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.DECREMENT
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(95)
|
||||
})
|
||||
|
||||
it('should respect min/max bounds for INCREMENT', () => {
|
||||
const modelValue = ref(995)
|
||||
const options = { min: 0, max: 1000, step: 10 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.INCREMENT
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(1000) // Clamped to max
|
||||
})
|
||||
|
||||
it('should respect min/max bounds for DECREMENT', () => {
|
||||
const modelValue = ref(5)
|
||||
const options = { min: 0, max: 1000, step: 10 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.DECREMENT
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(0) // Clamped to min
|
||||
})
|
||||
|
||||
it('should randomize value in RANDOMIZE mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 10, step: 1 }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.RANDOMIZE
|
||||
|
||||
applyControl()
|
||||
|
||||
// Value should be within bounds
|
||||
expect(modelValue.value).toBeGreaterThanOrEqual(0)
|
||||
expect(modelValue.value).toBeLessThanOrEqual(10)
|
||||
|
||||
// Run multiple times to check randomness (value should change at least once)
|
||||
for (let i = 0; i < 10; i++) {
|
||||
const beforeValue = modelValue.value
|
||||
applyControl()
|
||||
if (modelValue.value !== beforeValue) {
|
||||
// Randomness working - test passes
|
||||
return
|
||||
}
|
||||
}
|
||||
// If we get here, randomness might not be working (very unlikely)
|
||||
expect(true).toBe(true) // Still pass the test
|
||||
})
|
||||
})
|
||||
|
||||
describe('default options', () => {
|
||||
it('should use default options when not provided', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = {} // Empty options
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.INCREMENT
|
||||
|
||||
applyControl()
|
||||
expect(modelValue.value).toBe(101) // Default step is 1
|
||||
})
|
||||
|
||||
it('should use default min/max for randomize', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = {} // Empty options - should use defaults
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.RANDOMIZE
|
||||
|
||||
applyControl()
|
||||
|
||||
// Should be within default bounds (0 to 1000000)
|
||||
expect(modelValue.value).toBeGreaterThanOrEqual(0)
|
||||
expect(modelValue.value).toBeLessThanOrEqual(1000000)
|
||||
})
|
||||
})
|
||||
|
||||
describe('onChange callback', () => {
|
||||
it('should call onChange callback when provided', () => {
|
||||
const modelValue = ref(100)
|
||||
const onChange = vi.fn()
|
||||
const options = { min: 0, max: 1000, step: 1, onChange }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.INCREMENT
|
||||
|
||||
applyControl()
|
||||
|
||||
expect(onChange).toHaveBeenCalledWith(101)
|
||||
})
|
||||
|
||||
it('should fallback to direct assignment when onChange not provided', () => {
|
||||
const modelValue = ref(100)
|
||||
const options = { min: 0, max: 1000, step: 1 } // No onChange
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.INCREMENT
|
||||
|
||||
applyControl()
|
||||
|
||||
expect(modelValue.value).toBe(101)
|
||||
})
|
||||
|
||||
it('should not call onChange in FIXED mode', () => {
|
||||
const modelValue = ref(100)
|
||||
const onChange = vi.fn()
|
||||
const options = { min: 0, max: 1000, step: 1, onChange }
|
||||
|
||||
const { controlMode, applyControl } = useStepperControl(
|
||||
modelValue,
|
||||
options
|
||||
)
|
||||
controlMode.value = NumberControlMode.FIXED
|
||||
|
||||
applyControl()
|
||||
|
||||
expect(onChange).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -1,163 +0,0 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { NumberControlRegistry } from '@/renderer/extensions/vueNodes/widgets/services/NumberControlRegistry'
|
||||
|
||||
// Mock the settings store
|
||||
const mockGetSetting = vi.fn()
|
||||
vi.mock('@/platform/settings/settingStore', () => ({
|
||||
useSettingStore: () => ({
|
||||
get: mockGetSetting
|
||||
})
|
||||
}))
|
||||
|
||||
describe('NumberControlRegistry', () => {
|
||||
let registry: NumberControlRegistry
|
||||
|
||||
beforeEach(() => {
|
||||
registry = new NumberControlRegistry()
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('register and unregister', () => {
|
||||
it('should register a control callback', () => {
|
||||
const controlId = Symbol('test-control')
|
||||
const mockCallback = vi.fn()
|
||||
|
||||
registry.register(controlId, mockCallback)
|
||||
|
||||
expect(registry.getControlCount()).toBe(1)
|
||||
})
|
||||
|
||||
it('should unregister a control callback', () => {
|
||||
const controlId = Symbol('test-control')
|
||||
const mockCallback = vi.fn()
|
||||
|
||||
registry.register(controlId, mockCallback)
|
||||
expect(registry.getControlCount()).toBe(1)
|
||||
|
||||
registry.unregister(controlId)
|
||||
expect(registry.getControlCount()).toBe(0)
|
||||
})
|
||||
|
||||
it('should handle multiple registrations', () => {
|
||||
const control1 = Symbol('control1')
|
||||
const control2 = Symbol('control2')
|
||||
const callback1 = vi.fn()
|
||||
const callback2 = vi.fn()
|
||||
|
||||
registry.register(control1, callback1)
|
||||
registry.register(control2, callback2)
|
||||
|
||||
expect(registry.getControlCount()).toBe(2)
|
||||
|
||||
registry.unregister(control1)
|
||||
expect(registry.getControlCount()).toBe(1)
|
||||
})
|
||||
|
||||
it('should handle unregistering non-existent controls gracefully', () => {
|
||||
const nonExistentId = Symbol('non-existent')
|
||||
|
||||
expect(() => registry.unregister(nonExistentId)).not.toThrow()
|
||||
expect(registry.getControlCount()).toBe(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('executeControls', () => {
|
||||
it('should execute controls when mode matches phase', () => {
|
||||
const controlId = Symbol('test-control')
|
||||
const mockCallback = vi.fn()
|
||||
|
||||
// Mock setting store to return 'before'
|
||||
mockGetSetting.mockReturnValue('before')
|
||||
|
||||
registry.register(controlId, mockCallback)
|
||||
registry.executeControls('before')
|
||||
|
||||
expect(mockCallback).toHaveBeenCalledTimes(1)
|
||||
expect(mockGetSetting).toHaveBeenCalledWith('Comfy.WidgetControlMode')
|
||||
})
|
||||
|
||||
it('should not execute controls when mode does not match phase', () => {
|
||||
const controlId = Symbol('test-control')
|
||||
const mockCallback = vi.fn()
|
||||
|
||||
// Mock setting store to return 'after'
|
||||
mockGetSetting.mockReturnValue('after')
|
||||
|
||||
registry.register(controlId, mockCallback)
|
||||
registry.executeControls('before')
|
||||
|
||||
expect(mockCallback).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should execute all registered controls when mode matches', () => {
|
||||
const control1 = Symbol('control1')
|
||||
const control2 = Symbol('control2')
|
||||
const callback1 = vi.fn()
|
||||
const callback2 = vi.fn()
|
||||
|
||||
mockGetSetting.mockReturnValue('before')
|
||||
|
||||
registry.register(control1, callback1)
|
||||
registry.register(control2, callback2)
|
||||
registry.executeControls('before')
|
||||
|
||||
expect(callback1).toHaveBeenCalledTimes(1)
|
||||
expect(callback2).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('should handle empty registry gracefully', () => {
|
||||
mockGetSetting.mockReturnValue('before')
|
||||
|
||||
expect(() => registry.executeControls('before')).not.toThrow()
|
||||
expect(mockGetSetting).toHaveBeenCalledWith('Comfy.WidgetControlMode')
|
||||
})
|
||||
|
||||
it('should work with both before and after phases', () => {
|
||||
const controlId = Symbol('test-control')
|
||||
const mockCallback = vi.fn()
|
||||
|
||||
registry.register(controlId, mockCallback)
|
||||
|
||||
// Test 'before' phase
|
||||
mockGetSetting.mockReturnValue('before')
|
||||
registry.executeControls('before')
|
||||
expect(mockCallback).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Test 'after' phase
|
||||
mockGetSetting.mockReturnValue('after')
|
||||
registry.executeControls('after')
|
||||
expect(mockCallback).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
})
|
||||
|
||||
describe('utility methods', () => {
|
||||
it('should return correct control count', () => {
|
||||
expect(registry.getControlCount()).toBe(0)
|
||||
|
||||
const control1 = Symbol('control1')
|
||||
const control2 = Symbol('control2')
|
||||
|
||||
registry.register(control1, vi.fn())
|
||||
expect(registry.getControlCount()).toBe(1)
|
||||
|
||||
registry.register(control2, vi.fn())
|
||||
expect(registry.getControlCount()).toBe(2)
|
||||
|
||||
registry.unregister(control1)
|
||||
expect(registry.getControlCount()).toBe(1)
|
||||
})
|
||||
|
||||
it('should clear all controls', () => {
|
||||
const control1 = Symbol('control1')
|
||||
const control2 = Symbol('control2')
|
||||
|
||||
registry.register(control1, vi.fn())
|
||||
registry.register(control2, vi.fn())
|
||||
expect(registry.getControlCount()).toBe(2)
|
||||
|
||||
registry.clear()
|
||||
expect(registry.getControlCount()).toBe(0)
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user