From a48ad1cb41242a87129b56e7fc9fd430aaae4e6d Mon Sep 17 00:00:00 2001 From: Chenlei Hu Date: Sat, 28 Dec 2024 21:31:09 -0500 Subject: [PATCH] Move setting impl from ComfySettingsDialog to settingStore (#2085) --- browser_tests/extensionAPI.spec.ts | 2 +- src/components/graph/GraphCanvas.vue | 5 + src/scripts/app.ts | 8 +- src/scripts/ui/settings.ts | 164 ++++++------------ src/services/extensionService.ts | 4 +- src/stores/settingStore.ts | 128 +++++++++----- src/views/GraphView.vue | 3 +- .../tests/fast/store/settingStore.test.ts | 141 +++++++++++++++ tests-ui/utils/index.ts | 5 +- tests-ui/utils/setup.ts | 27 --- 10 files changed, 299 insertions(+), 188 deletions(-) create mode 100644 tests-ui/tests/fast/store/settingStore.test.ts diff --git a/browser_tests/extensionAPI.spec.ts b/browser_tests/extensionAPI.spec.ts index ce075c5dd..d4e638156 100644 --- a/browser_tests/extensionAPI.spec.ts +++ b/browser_tests/extensionAPI.spec.ts @@ -1,4 +1,4 @@ -import { expect, Locator } from '@playwright/test' +import { expect } from '@playwright/test' import { comfyPageFixture as test } from './fixtures/ComfyPage' test.describe('Topbar commands', () => { diff --git a/src/components/graph/GraphCanvas.vue b/src/components/graph/GraphCanvas.vue index cdc7734ed..8e0d62631 100644 --- a/src/components/graph/GraphCanvas.vue +++ b/src/components/graph/GraphCanvas.vue @@ -68,6 +68,7 @@ import { useWorkflowService } from '@/services/workflowService' import { useColorPaletteStore } from '@/stores/workspace/colorPaletteStore' import { useColorPaletteService } from '@/services/colorPaletteService' import { IS_CONTROL_WIDGET, updateControlWidgetLabel } from '@/scripts/widgets' +import { CORE_SETTINGS } from '@/constants/coreSettings' const emit = defineEmits(['ready']) const canvasRef = ref(null) @@ -337,6 +338,10 @@ onMounted(async () => { // ChangeTracker needs to be initialized before setup, as it will overwrite // some listeners of litegraph canvas. ChangeTracker.init(comfyApp) + await settingStore.loadSettingValues() + CORE_SETTINGS.forEach((setting) => { + settingStore.addSetting(setting) + }) await comfyApp.setup(canvasRef.value) canvasStore.canvas = comfyApp.canvas canvasStore.canvas.render_canvas_border = false diff --git a/src/scripts/app.ts b/src/scripts/app.ts index b7b657bb5..b037abd4c 100644 --- a/src/scripts/app.ts +++ b/src/scripts/app.ts @@ -1001,15 +1001,9 @@ export class ComfyApp { */ async setup(canvasEl: HTMLCanvasElement) { this.canvasEl = canvasEl - // Show menu container for GraphView. - this.ui.menuContainer.style.display = 'block' - this.resizeCanvas() - await Promise.all([ - useWorkspaceStore().workflow.syncWorkflows(), - this.ui.settings.load() - ]) + await useWorkspaceStore().workflow.syncWorkflows() await useExtensionService().loadExtensions() this.#addProcessMouseHandler() diff --git a/src/scripts/ui/settings.ts b/src/scripts/ui/settings.ts index 8cfe3cda9..d379031e2 100644 --- a/src/scripts/ui/settings.ts +++ b/src/scripts/ui/settings.ts @@ -1,49 +1,19 @@ -import { api } from '@/scripts/api' import type { ComfyApp } from '@/scripts/app' import { ComfyDialog } from './dialog' -import type { Setting, SettingParams } from '@/types/settingTypes' +import type { SettingParams } from '@/types/settingTypes' import type { Settings } from '@/types/apiTypes' import { useSettingStore } from '@/stores/settingStore' import { useToastStore } from '@/stores/toastStore' export class ComfySettingsDialog extends ComfyDialog { app: ComfyApp - settingsValues: any - settingsLookup: Record - settingsParamLookup: Record constructor(app: ComfyApp) { super() this.app = app - this.settingsValues = {} - this.settingsLookup = {} - this.settingsParamLookup = {} } - get settings() { - return Object.values(this.settingsLookup) - } - - private tryMigrateDeprecatedValue(id: string, value: any) { - if (this.app.vueAppReady) { - const settingStore = useSettingStore() - const setting = settingStore.settingsById[id] - if (setting?.migrateDeprecatedValue) { - return setting.migrateDeprecatedValue(value) - } - } - return value - } - - #dispatchChange(id: string, value: T, oldValue?: T) { - // Keep the settingStore updated. Not using `store.set` as it would trigger - // setSettingValue again. - // `load` re-dispatch the change for any settings added before load so - // settingStore is always up to date. - if (this.app.vueAppReady) { - useSettingStore().settingValues[id] = value - } - + dispatchChange(id: string, value: T, oldValue?: T) { this.dispatchEvent( new CustomEvent(id + '.change', { detail: { @@ -54,68 +24,74 @@ export class ComfySettingsDialog extends ComfyDialog { ) } - async load() { - this.settingsValues = await api.getSettings() - - // Trigger onChange for any settings added before load - for (const id in this.settingsLookup) { - const compatId = id - this.settingsValues[compatId] = this.tryMigrateDeprecatedValue( - id, - this.settingsValues[compatId] - ) - const value = this.settingsValues[compatId] - this.settingsLookup[id].onChange?.(value) - this.#dispatchChange(id, value) - } + /** + * @deprecated Use `settingStore.settingValues` instead. + */ + get settingsValues() { + return useSettingStore().settingValues } + /** + * @deprecated Use `settingStore.settingsById` instead. + */ + get settingsLookup() { + return useSettingStore().settingsById + } + + /** + * @deprecated Use `settingStore.settingsById` instead. + */ + get settingsParamLookup() { + return useSettingStore().settingsById + } + + /** + * @deprecated Use `settingStore.get` instead. + */ getSettingValue( id: K, defaultValue?: Settings[K] ): Settings[K] { - let value = this.settingsValues[id] - return (value ?? defaultValue) as Settings[K] + if (defaultValue !== undefined) { + console.warn( + `Parameter defaultValue is deprecated. The default value in settings definition will be used instead.` + ) + } + return useSettingStore().get(id) } - getSettingDefaultValue(id: string) { - const param = this.settingsParamLookup[id] - return typeof param?.defaultValue === 'function' - ? param.defaultValue() - : param?.defaultValue + /** + * @deprecated Use `settingStore.getDefaultValue` instead. + */ + getSettingDefaultValue(id: K): Settings[K] { + return useSettingStore().getDefaultValue(id) } + /** + * @deprecated Use `settingStore.set` instead. + */ async setSettingValueAsync( id: K, value: Settings[K] ) { - value = this.tryMigrateDeprecatedValue(id, value) - - let oldValue = this.getSettingValue(id, undefined) - this.settingsValues[id] = value - - if (id in this.settingsLookup) { - this.settingsLookup[id].onChange?.(value, oldValue) - } - this.#dispatchChange(id, value, oldValue) - - await api.storeSetting(id, value) - } - - setSettingValue(id: K, value: Settings[K]) { - this.setSettingValueAsync(id, value).catch((err) => { - useToastStore().addAlert(`Error saving setting '${id}': ${err}`) - }) - } - - refreshSetting(id: keyof Settings) { - const value = this.getSettingValue(id) - this.settingsLookup[id].onChange?.(value) - this.#dispatchChange(id, value) + await useSettingStore().set(id, value) } /** - * Deprecated for external callers/extensions. Use `ComfyExtension.settings` field instead. + * @deprecated Use `settingStore.set` instead. + */ + setSettingValue(id: K, value: Settings[K]) { + useSettingStore() + .set(id, value) + .catch((err) => { + useToastStore().addAlert(`Error saving setting '${id}': ${err}`) + }) + } + + /** + * @deprecated Deprecated for external callers/extensions. Use + * `ComfyExtension.settings` field instead. + * * Example: * ```ts * app.registerExtension({ @@ -132,41 +108,15 @@ export class ComfySettingsDialog extends ComfyDialog { * ``` */ addSetting(params: SettingParams) { - const { id, name, type, defaultValue, onChange } = params - if (!id) { - throw new Error('Settings must have an ID') - } + const settingStore = useSettingStore() + settingStore.addSetting(params) - if (id in this.settingsLookup) { - throw new Error(`Setting ${id} of type ${type} must have a unique ID.`) - } - - const value = this.getSettingValue(id) ?? defaultValue - - // Trigger initial setting of value - onChange?.(value, undefined) - this.#dispatchChange(id, value) - - this.settingsParamLookup[id] = params - if (this.app.vueAppReady) { - useSettingStore().settingsById[id] = params - } - this.settingsLookup[id] = { - id, - onChange, - name, - render: () => { - console.warn('[ComfyUI] Setting render is deprecated', id) - } - } as Setting - - const self = this return { get value() { - return self.getSettingValue(id, defaultValue) + return settingStore.get(params.id) }, set value(v) { - self.setSettingValue(id, v) + settingStore.set(params.id, v) } } } diff --git a/src/services/extensionService.ts b/src/services/extensionService.ts index 259d2a4e1..6cc7900e2 100644 --- a/src/services/extensionService.ts +++ b/src/services/extensionService.ts @@ -50,7 +50,9 @@ export const useExtensionService = () => { useKeybindingStore().loadExtensionKeybindings(extension) useCommandStore().loadExtensionCommands(extension) useMenuItemStore().loadExtensionMenuCommands(extension) - settingStore.loadExtensionSettings(extension) + extension.settings?.forEach((setting) => { + settingStore.addSetting(setting) + }) useBottomPanelStore().registerExtensionBottomPanelTabs(extension) if (extension.getCustomWidgets) { // TODO(huchenlei): We should deprecate the async return value of diff --git a/src/stores/settingStore.ts b/src/stores/settingStore.ts index 1936068e1..196764fb9 100644 --- a/src/stores/settingStore.ts +++ b/src/stores/settingStore.ts @@ -1,22 +1,11 @@ -/** - * TODO: Migrate scripts/ui/settings.ts here - * - * Currently the reactive settings act as a proxy of the legacy settings. - * Every time a setting is changed, the settingStore dispatch the change to the - * legacy settings. Every time the legacy settings are changed, the legacy - * settings directly updates the settingStore.settingValues. - */ - import { ref, computed } from 'vue' import { defineStore } from 'pinia' -import { app } from '@/scripts/app' -import { ComfySettingsDialog } from '@/scripts/ui/settings' import type { Settings } from '@/types/apiTypes' import type { SettingParams } from '@/types/settingTypes' import type { TreeNode } from 'primevue/treenode' -import type { ComfyExtension } from '@/types/comfy' import { buildTree } from '@/utils/treeUtil' -import { CORE_SETTINGS } from '@/constants/coreSettings' +import { api } from '@/scripts/api' +import { app } from '@/scripts/app' export const getSettingInfo = (setting: SettingParams) => { const parts = setting.category || setting.id.split('.') @@ -31,6 +20,19 @@ export interface SettingTreeNode extends TreeNode { data?: SettingParams } +function tryMigrateDeprecatedValue(setting: SettingParams, value: any) { + return setting?.migrateDeprecatedValue?.(value) ?? value +} + +function onChange(setting: SettingParams, value: any, oldValue: any) { + if (setting?.onChange && value !== oldValue) { + setting.onChange(value) + // Backward compatibility with old settings dialog. + // Some extensions still listens event emitted by the old settings dialog. + app.ui.settings.dispatchChange(setting.id, value, oldValue) + } +} + export const useSettingStore = defineStore('setting', () => { const settingValues = ref>({}) const settingsById = ref>({}) @@ -57,47 +59,95 @@ export const useSettingStore = defineStore('setting', () => { return root }) - function addSettings(settingsDialog: ComfySettingsDialog) { - for (const id in settingsDialog.settingsLookup) { - const value = settingsDialog.getSettingValue(id) - settingValues.value[id] = value - } - settingsById.value = settingsDialog.settingsParamLookup - - CORE_SETTINGS.forEach((setting: SettingParams) => { - settingsDialog.addSetting(setting) - }) - } - - function loadExtensionSettings(extension: ComfyExtension) { - extension.settings?.forEach((setting: SettingParams) => { - app.ui.settings.addSetting(setting) - }) - } - + /** + * Check if a setting's value exists, i.e. if the user has set it manually. + * @param key - The key of the setting to check. + * @returns Whether the setting exists. + */ function exists(key: string) { return settingValues.value[key] !== undefined } + /** + * Set a setting value. + * @param key - The key of the setting to set. + * @param value - The value to set. + */ async function set(key: K, value: Settings[K]) { - settingValues.value[key] = value - await app.ui.settings.setSettingValueAsync(key, value) + const newValue = tryMigrateDeprecatedValue(settingsById.value[key], value) + const oldValue = settingValues.value[key] + onChange(settingsById.value[key], newValue, oldValue) + + settingValues.value[key] = newValue + await api.storeSetting(key, newValue) } + /** + * Get a setting value. + * @param key - The key of the setting to get. + * @returns The value of the setting. + */ function get(key: K): Settings[K] { - return ( - settingValues.value[key] ?? app.ui.settings.getSettingDefaultValue(key) - ) + return settingValues.value[key] ?? getDefaultValue(key) + } + + /** + * Get the default value of a setting. + * @param key - The key of the setting to get. + * @returns The default value of the setting. + */ + function getDefaultValue(key: K): Settings[K] { + const param = settingsById.value[key] + return typeof param?.defaultValue === 'function' + ? param.defaultValue() + : param?.defaultValue + } + + /** + * Register a setting. + * @param setting - The setting to register. + */ + function addSetting(setting: SettingParams) { + if (!setting.id) { + throw new Error('Settings must have an ID') + } + if (setting.id in settingsById.value) { + throw new Error(`Setting ${setting.id} must have a unique ID.`) + } + + settingsById.value[setting.id] = setting + + if (settingValues.value[setting.id] !== undefined) { + settingValues.value[setting.id] = tryMigrateDeprecatedValue( + setting, + settingValues.value[setting.id] + ) + } + onChange(setting, get(setting.id), undefined) + } + + /* + * Load setting values from server. + * This needs to be called before any setting is registered. + */ + async function loadSettingValues() { + if (Object.keys(settingsById.value).length) { + throw new Error( + 'Setting values must be loaded before any setting is registered.' + ) + } + settingValues.value = await api.getSettings() } return { settingValues, settingsById, settingTree, - addSettings, - loadExtensionSettings, + addSetting, + loadSettingValues, set, get, - exists + exists, + getDefaultValue } }) diff --git a/src/views/GraphView.vue b/src/views/GraphView.vue index ae43ef229..165a7973b 100644 --- a/src/views/GraphView.vue +++ b/src/views/GraphView.vue @@ -94,7 +94,7 @@ watchEffect(() => { watchEffect(() => { const useNewMenu = settingStore.get('Comfy.UseNewMenu') if (useNewMenu === 'Disabled') { - app.ui.menuContainer.style.removeProperty('display') + app.ui.menuContainer.style.setProperty('display', 'block') app.ui.restoreMenuPosition() } else { app.ui.menuContainer.style.setProperty('display', 'none') @@ -108,7 +108,6 @@ watchEffect(() => { }) const init = () => { - settingStore.addSettings(app.ui.settings) const coreCommands = useCoreCommands() useCommandStore().registerCommands(coreCommands) useMenuItemStore().registerCoreMenuCommands() diff --git a/tests-ui/tests/fast/store/settingStore.test.ts b/tests-ui/tests/fast/store/settingStore.test.ts new file mode 100644 index 000000000..41ffe3e91 --- /dev/null +++ b/tests-ui/tests/fast/store/settingStore.test.ts @@ -0,0 +1,141 @@ +import { setActivePinia, createPinia } from 'pinia' +import { useSettingStore } from '@/stores/settingStore' +import { api } from '@/scripts/api' +import type { SettingParams } from '@/types/settingTypes' + +// Mock the api +jest.mock('@/scripts/api', () => ({ + api: { + getSettings: jest.fn(), + storeSetting: jest.fn() + } +})) + +// Mock the app +jest.mock('@/scripts/app', () => ({ + app: { + ui: { + settings: { + dispatchChange: jest.fn() + } + } + } +})) + +describe('useSettingStore', () => { + let store: ReturnType + + beforeEach(() => { + setActivePinia(createPinia()) + store = useSettingStore() + jest.clearAllMocks() + }) + + it('should initialize with empty settings', () => { + expect(store.settingValues).toEqual({}) + expect(store.settingsById).toEqual({}) + expect(store.settingTree.children).toEqual([]) + }) + + describe('loadSettingValues', () => { + it('should load settings from API', async () => { + const mockSettings = { 'test.setting': 'value' } + ;(api.getSettings as jest.Mock).mockResolvedValue(mockSettings) + + await store.loadSettingValues() + + expect(store.settingValues).toEqual(mockSettings) + expect(api.getSettings).toHaveBeenCalled() + }) + + it('should throw error if settings are loaded after registration', async () => { + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default' + } + store.addSetting(setting) + + await expect(store.loadSettingValues()).rejects.toThrow( + 'Setting values must be loaded before any setting is registered.' + ) + }) + }) + + describe('addSetting', () => { + it('should register a new setting', () => { + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default' + } + + store.addSetting(setting) + + expect(store.settingsById['test.setting']).toEqual(setting) + }) + + it('should throw error for duplicate setting ID', () => { + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default' + } + + store.addSetting(setting) + expect(() => store.addSetting(setting)).toThrow( + 'Setting test.setting must have a unique ID.' + ) + }) + + it('should migrate deprecated values', () => { + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default', + migrateDeprecatedValue: (value: string) => value.toUpperCase() + } + + store.settingValues['test.setting'] = 'oldvalue' + store.addSetting(setting) + + expect(store.settingValues['test.setting']).toBe('OLDVALUE') + }) + }) + + describe('get and set', () => { + it('should get default value when setting not exists', () => { + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default' + } + store.addSetting(setting) + + expect(store.get('test.setting')).toBe('default') + }) + + it('should set value and trigger onChange', async () => { + const onChangeMock = jest.fn() + const setting: SettingParams = { + id: 'test.setting', + name: 'test.setting', + type: 'text', + defaultValue: 'default', + onChange: onChangeMock + } + store.addSetting(setting) + + await store.set('test.setting', 'newvalue') + + expect(store.get('test.setting')).toBe('newvalue') + expect(onChangeMock).toHaveBeenCalledWith('newvalue') + expect(api.storeSetting).toHaveBeenCalledWith('test.setting', 'newvalue') + }) + }) +}) diff --git a/tests-ui/utils/index.ts b/tests-ui/utils/index.ts index 661af8bf8..5031b448d 100644 --- a/tests-ui/utils/index.ts +++ b/tests-ui/utils/index.ts @@ -1,5 +1,5 @@ // @ts-strict-ignore -import { APIConfig, mockApi, mockSettingStore, mockNodeDefStore } from './setup' +import { APIConfig, mockApi, mockNodeDefStore } from './setup' import { Ez, EzGraph, EzNameSpace } from './ezgraph' import lg from './litegraph' import fs from 'fs' @@ -41,10 +41,7 @@ export async function start(config: StartConfig = {}): Promise { document.body.innerHTML = html.toString() mockApi(config) - mockSettingStore() const { app } = await import('../../src/scripts/app') - const { useSettingStore } = await import('../../src/stores/settingStore') - useSettingStore().addSettings(app.ui.settings) mockNodeDefStore() const { LiteGraph, LGraphCanvas } = await import('@comfyorg/litegraph') diff --git a/tests-ui/utils/setup.ts b/tests-ui/utils/setup.ts index 8e2c4c538..79b73434b 100644 --- a/tests-ui/utils/setup.ts +++ b/tests-ui/utils/setup.ts @@ -1,6 +1,4 @@ // @ts-strict-ignore -import type { ComfySettingsDialog } from '@/scripts/ui/settings' -import type { ComfyApp } from '@/scripts/app' import '../../src/scripts/api' import { ComfyNodeDef } from '@/types/apiTypes' @@ -98,31 +96,6 @@ export function mockApi(config: APIConfig = {}) { })) } -export const mockSettingStore = () => { - let app: ComfyApp | null = null - - const mockedSettingStore = { - addSettings(settings: ComfySettingsDialog) { - app = settings.app - }, - - set(key: string, value: any) { - app?.ui.settings.setSettingValue(key, value) - }, - - get(key: string) { - return ( - app?.ui.settings.getSettingValue(key) ?? - app?.ui.settings.getSettingDefaultValue(key) - ) - } - } - - jest.mock('@/stores/settingStore', () => ({ - useSettingStore: jest.fn(() => mockedSettingStore) - })) -} - export const mockNodeDefStore = () => { const mockedNodeDefStore = { addNodeDef: jest.fn((nodeDef: ComfyNodeDef) => {})