Fix double trigger of setting onChange callback (#1385)

* Fix onChange double trigger

* nit

* Add playwright test

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!')
   })
 })

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 {

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()
+  }
+}

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)
 }
 

src/scripts/ui/settings.ts

@@ -160,7 +160,6 @@ export class ComfySettingsDialog extends ComfyDialog<HTMLDialogElement> {
       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<HTMLDialogElement> {
     }
 
     // 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) {