Fix issues with 'Discard changes' confirmation dialog (#3478)

* Prevent memory leak

* Fix deck option changes not detected until focus is lost

* Accurately determine if there are any pending changes

This makes it so that the confirmation dialog appears when it should,
and not when it shouldn't.

qt/aqt/deckoptions.py

@@ -62,41 +62,38 @@ class DeckOptionsDialog(QDialog):
         if cmd == "deckOptionsReady":
             self._ready = True
             gui_hooks.deck_options_did_load(self)
+        elif cmd == "confirmDiscardChanges":
+            self.confirm_discard_changes()
+        elif cmd == "_close":
+            self._close()
 
     def closeEvent(self, evt: QCloseEvent) -> None:
         if self._close_event_has_cleaned_up:
             evt.accept()
             return
         evt.ignore()
-        self.if_can_close()
+        self.check_pending_changes()
 
     def _close(self):
         """Close. Ensure the closeEvent is not ignored."""
         self._close_event_has_cleaned_up = True
         self.close()
 
-    def if_can_close(self):
+    def confirm_discard_changes(self) -> None:
-        """Close if there was no modification. Otherwise ask for confirmation first."""
+        def callbackWithUserChoice(choice: int) -> None:
-
-        def callbackWithUserChoice(choice: int):
             if choice == 0:
                 # The user accepted to discard current input.
                 self._close()
 
-        def if_can_close_callback_with_data_information(has_modified_dataData: bool):
+        ask_user_dialog(
-            if has_modified_dataData:
+            tr.card_templates_discard_changes(),
-                ask_user_dialog(
+            callback=callbackWithUserChoice,
-                    tr.card_templates_discard_changes(),
+            buttons=[
-                    callback=callbackWithUserChoice,
+                QMessageBox.StandardButton.Discard,
-                    buttons=[
+                (tr.adding_keep_editing(), QMessageBox.ButtonRole.RejectRole),
-                        QMessageBox.StandardButton.Discard,
+            ],
-                        (tr.adding_keep_editing(), QMessageBox.ButtonRole.RejectRole),
+            parent=self,
-                    ],
+        )
-                )
-            else:
-                self._close()
-
-        self.has_modified_data(if_can_close_callback_with_data_information)
 
     def reject(self) -> None:
         self.mw.col.set_wants_abort()
@@ -105,12 +102,11 @@ class DeckOptionsDialog(QDialog):
         saveGeom(self, self.TITLE)
         QDialog.reject(self)
 
-    def has_modified_data(self, callback: Callable[[bool], None]):
+    def check_pending_changes(self):
-        """Calls `callback` with the information of whether any deck options are modified."""
         if self._ready:
-            self.web.evalWithCallback("anki.deckOptionsPendingChanges()", callback)
+            self.web.eval("anki.deckOptionsPendingChanges();")
         else:
-            callback(False)
+            self._close()
 
 
 def confirm_deck_then_display_options(active_card: Card | None = None) -> None:

ts/routes/deck-options/SaveButton.svelte

@@ -21,6 +21,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     import WithFloating from "$lib/components/WithFloating.svelte";
 
     import type { DeckOptionsState } from "./lib";
+    import { commitEditing } from "./lib";
 
     const rtl: boolean = window.getComputedStyle(document.body).direction == "rtl";
 
@@ -28,14 +29,6 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
     export let state: DeckOptionsState;
 
-    /** Ensure blur handler has fired so changes get committed. */
-    async function commitEditing(): Promise<void> {
-        if (document.activeElement instanceof HTMLElement) {
-            document.activeElement.blur();
-        }
-        await tick();
-    }
-
     async function removeConfig(): Promise<void> {
         // show pop-up after dropdown has gone away
         await tick();

ts/routes/deck-options/[deckId]/+page.svelte

@@ -5,6 +5,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 <script lang="ts">
     import { onMount } from "svelte";
     import DeckOptionsPage from "../DeckOptionsPage.svelte";
+    import { commitEditing } from "../lib";
     import type { PageData } from "./$types";
     import { bridgeCommand, bridgeCommandsAvailable } from "@tslib/bridgecommand";
 
@@ -12,9 +13,17 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     let page: DeckOptionsPage;
 
     globalThis.anki ||= {};
-    globalThis.anki.deckOptionsPendingChanges = () => {
+    globalThis.anki.deckOptionsPendingChanges = async (): Promise<void> => {
-        return data.state.isModified();
+        await commitEditing();
+        if (bridgeCommandsAvailable()) {
+            if (data.state.isModified()) {
+                bridgeCommand("confirmDiscardChanges");
+            } else {
+                bridgeCommand("_close");
+            }
+        }
     };
+
     onMount(() => {
         globalThis.$deckOptions = new Promise((resolve, _reject) => {
             resolve(page);

ts/routes/deck-options/lib.ts

@@ -11,7 +11,8 @@ import type {
 import { DeckConfig, DeckConfig_Config, DeckConfigsForUpdate_CurrentDeck_Limits } from "@generated/anki/deck_config_pb";
 import { updateDeckConfigs } from "@generated/backend";
 import { localeCompare } from "@tslib/i18n";
-import { cloneDeep, isEqual } from "lodash-es";
+import { cloneDeep, isEqual, pickBy } from "lodash-es";
+import { tick } from "svelte";
 import type { Readable, Writable } from "svelte/store";
 import { get, readable, writable } from "svelte/store";
 
@@ -32,6 +33,25 @@ export interface ConfigListEntry {
     current: boolean;
 }
 
+type Configs =
+    & Required<
+        Pick<
+            PlainMessage<UpdateDeckConfigsRequest>,
+            | "configs"
+            | "cardStateCustomizer"
+            | "limits"
+            | "newCardsIgnoreReviewLimit"
+            | "applyAllParentLimits"
+            | "fsrs"
+            | "fsrsReschedule"
+        >
+    >
+    & { currentConfig: DeckConfig_Config };
+
+type DeepPartial<T extends object, K extends keyof T> = {
+    [key in keyof T]: key extends K ? Partial<T[key]> : T[key];
+};
+
 export class DeckOptionsState {
     readonly currentConfig: Writable<DeckConfig_Config>;
     readonly currentAuxData: Writable<Record<string, unknown>>;
@@ -47,6 +67,8 @@ export class DeckOptionsState {
     readonly fsrsReschedule: Writable<boolean> = writable(false);
     readonly daysSinceLastOptimization: Writable<number>;
     readonly currentPresetName: Writable<string>;
+    /** Used to detect if there are any pending changes */
+    readonly originalConfigs: Configs;
 
     private targetDeckId: DeckOptionsId;
     private configs: ConfigWithCount[];
@@ -101,6 +123,17 @@ export class DeckOptionsState {
         // update our state when the current config is changed
         this.currentConfig.subscribe((val) => this.onCurrentConfigChanged(val));
         this.currentAuxData.subscribe((val) => this.onCurrentAuxDataChanged(val));
+
+        this.originalConfigs = cloneDeep<Configs>({
+            configs: this.configs.map(c => c.config!),
+            cardStateCustomizer: data.cardStateCustomizer,
+            limits: get(this.deckLimits),
+            newCardsIgnoreReviewLimit: data.newCardsIgnoreReviewLimit,
+            applyAllParentLimits: data.applyAllParentLimits,
+            fsrs: data.fsrs,
+            fsrsReschedule: get(this.fsrsReschedule),
+            currentConfig: get(this.currentConfig),
+        });
     }
 
     setCurrentIndex(index: number): void {
@@ -294,7 +327,22 @@ export class DeckOptionsState {
     }
 
     isModified(): boolean {
-        return this.removedConfigs.length > 0 || this.modifiedConfigs.size > 0;
+        const original: DeepPartial<Configs, "limits"> = {
+            ...this.originalConfigs,
+            limits: omitUndefined(this.originalConfigs.limits),
+        };
+        const current: typeof original = {
+            configs: this.configs.map(c => c.config),
+            cardStateCustomizer: get(this.cardStateCustomizer),
+            limits: omitUndefined(get(this.deckLimits)),
+            newCardsIgnoreReviewLimit: get(this.newCardsIgnoreReviewLimit),
+            applyAllParentLimits: get(this.applyAllParentLimits),
+            fsrs: get(this.fsrs),
+            fsrsReschedule: get(this.fsrsReschedule),
+            currentConfig: get(this.currentConfig),
+        };
+
+        return !isEqual(original, current);
     }
 }
 
@@ -364,3 +412,16 @@ export class ValueTab {
         this.setter(value);
     }
 }
+
+/** Returns a copy of the given object with the properties whose values are 'undefined' omitted */
+function omitUndefined<T extends object>(obj: T): Partial<T> {
+    return pickBy(obj, val => val !== undefined);
+}
+
+/** Ensure blur handler has fired so changes get committed. */
+export async function commitEditing(): Promise<void> {
+    if (document.activeElement instanceof HTMLElement) {
+        document.activeElement.blur();
+    }
+    await tick();
+}