From 94f4147f920921b96c10f4e8b261591401a4964b Mon Sep 17 00:00:00 2001 From: Chenlei Hu Date: Wed, 30 Oct 2024 19:55:46 -0400 Subject: [PATCH] Fix double trigger of setting onChange callback (#1385) * Fix onChange double trigger * nit * Add playwright test --- browser_tests/extensionAPI.spec.ts | 66 ++++++++++++++----- browser_tests/fixtures/ComfyPage.ts | 3 + .../fixtures/components/SettingDialog.ts | 34 ++++++++++ .../dialog/content/setting/SettingGroup.vue | 1 - src/scripts/ui/settings.ts | 7 +- 5 files changed, 90 insertions(+), 21 deletions(-) create mode 100644 browser_tests/fixtures/components/SettingDialog.ts diff --git a/browser_tests/extensionAPI.spec.ts b/browser_tests/extensionAPI.spec.ts index 7ceec5bc41..01fbfd24ad 100644 --- a/browser_tests/extensionAPI.spec.ts +++ b/browser_tests/extensionAPI.spec.ts @@ -80,22 +80,58 @@ test.describe('Topbar commands', () => { ) }) - test('Should allow adding settings', async ({ comfyPage }) => { - await comfyPage.page.evaluate(() => { - window['app'].registerExtension({ - name: 'TestExtension1', - settings: [ - { - id: 'TestSetting', - name: 'Test Setting', - type: 'text', - defaultValue: 'Hello, world!' - } - ] + test.describe('Settings', () => { + test('Should allow adding settings', async ({ comfyPage }) => { + await comfyPage.page.evaluate(() => { + window['app'].registerExtension({ + name: 'TestExtension1', + settings: [ + { + id: 'TestSetting', + name: 'Test Setting', + type: 'text', + defaultValue: 'Hello, world!', + onChange: () => { + window['changeCount'] = (window['changeCount'] ?? 0) + 1 + } + } + ] + }) }) + // onChange is called when the setting is first added + expect(await comfyPage.page.evaluate(() => window['changeCount'])).toBe(1) + expect(await comfyPage.getSetting('TestSetting')).toBe('Hello, world!') + + await comfyPage.setSetting('TestSetting', 'Hello, universe!') + expect(await comfyPage.getSetting('TestSetting')).toBe('Hello, universe!') + expect(await comfyPage.page.evaluate(() => window['changeCount'])).toBe(2) + }) + + test('Should allow setting boolean settings', async ({ comfyPage }) => { + await comfyPage.page.evaluate(() => { + window['app'].registerExtension({ + name: 'TestExtension1', + settings: [ + { + id: 'Comfy.TestSetting', + name: 'Test Setting', + type: 'boolean', + defaultValue: false, + onChange: () => { + window['changeCount'] = (window['changeCount'] ?? 0) + 1 + } + } + ] + }) + }) + + expect(await comfyPage.getSetting('Comfy.TestSetting')).toBe(false) + expect(await comfyPage.page.evaluate(() => window['changeCount'])).toBe(1) + + await comfyPage.settingDialog.open() + await comfyPage.settingDialog.toggleBooleanSetting('Comfy.TestSetting') + expect(await comfyPage.getSetting('Comfy.TestSetting')).toBe(true) + expect(await comfyPage.page.evaluate(() => window['changeCount'])).toBe(2) }) - expect(await comfyPage.getSetting('TestSetting')).toBe('Hello, world!') - await comfyPage.setSetting('TestSetting', 'Hello, universe!') - expect(await comfyPage.getSetting('TestSetting')).toBe('Hello, universe!') }) }) diff --git a/browser_tests/fixtures/ComfyPage.ts b/browser_tests/fixtures/ComfyPage.ts index af4fc7cf23..73397732a9 100644 --- a/browser_tests/fixtures/ComfyPage.ts +++ b/browser_tests/fixtures/ComfyPage.ts @@ -17,6 +17,7 @@ import { import { Topbar } from './components/Topbar' import { NodeReference } from './utils/litegraphUtils' import type { Position, Size } from './types' +import { SettingDialog } from './components/SettingDialog' class ComfyMenu { public readonly sideToolbar: Locator @@ -89,6 +90,7 @@ export class ComfyPage { public readonly menu: ComfyMenu public readonly actionbar: ComfyActionbar public readonly templates: ComfyTemplates + public readonly settingDialog: SettingDialog constructor( public readonly page: Page, @@ -104,6 +106,7 @@ export class ComfyPage { this.menu = new ComfyMenu(page) this.actionbar = new ComfyActionbar(page) this.templates = new ComfyTemplates(page) + this.settingDialog = new SettingDialog(page) } convertLeafToContent(structure: FolderStructure): FolderStructure { diff --git a/browser_tests/fixtures/components/SettingDialog.ts b/browser_tests/fixtures/components/SettingDialog.ts new file mode 100644 index 0000000000..ee1e7fad4d --- /dev/null +++ b/browser_tests/fixtures/components/SettingDialog.ts @@ -0,0 +1,34 @@ +import { Page } from '@playwright/test' + +export class SettingDialog { + constructor(public readonly page: Page) {} + + async open() { + const button = this.page.locator('button.comfy-settings-btn:visible') + await button.click() + await this.page.waitForSelector('div.settings-container') + } + + /** + * Set the value of a text setting + * @param id - The id of the setting + * @param value - The value to set + */ + async setStringSetting(id: string, value: string) { + const settingInputDiv = this.page.locator( + `div.settings-container div[id="${id}"]` + ) + await settingInputDiv.locator('input').fill(value) + } + + /** + * Toggle the value of a boolean setting + * @param id - The id of the setting + */ + async toggleBooleanSetting(id: string) { + const settingInputDiv = this.page.locator( + `div.settings-container div[id="${id}"]` + ) + await settingInputDiv.locator('input').click() + } +} diff --git a/src/components/dialog/content/setting/SettingGroup.vue b/src/components/dialog/content/setting/SettingGroup.vue index ed033b602c..aeca731525 100644 --- a/src/components/dialog/content/setting/SettingGroup.vue +++ b/src/components/dialog/content/setting/SettingGroup.vue @@ -86,7 +86,6 @@ function getSettingAttrs(setting: SettingParams) { } const updateSetting = (setting: SettingParams, value: any) => { - if (setting.onChange) setting.onChange(value, settingStore.get(setting.id)) settingStore.set(setting.id, value) } diff --git a/src/scripts/ui/settings.ts b/src/scripts/ui/settings.ts index 7583fe4869..3676e07621 100644 --- a/src/scripts/ui/settings.ts +++ b/src/scripts/ui/settings.ts @@ -160,7 +160,6 @@ export class ComfySettingsDialog extends ComfyDialog { throw new Error(`Setting ${id} of type ${type} must have a unique ID.`) } - let skipOnChange = false let value = this.getSettingValue(id) if (value == null) { if (this.app.isNewUserSession) { @@ -177,10 +176,8 @@ export class ComfySettingsDialog extends ComfyDialog { } // Trigger initial setting of value - if (!skipOnChange) { - onChange?.(value, undefined) - this.#dispatchChange(id, value) - } + onChange?.(value, undefined) + this.#dispatchChange(id, value) this.settingsParamLookup[id] = params if (this.app.vueAppReady) {