From 407e2dc43b719f499d1d28f734e26620ce6a5fde Mon Sep 17 00:00:00 2001
From: "Hikaru Y." <hkrysg@gmail.com>
Date: Sun, 6 Oct 2024 13:21:00 +0900
Subject: [PATCH] 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                        | 42 ++++++-------
 ts/routes/deck-options/SaveButton.svelte     |  9 +--
 ts/routes/deck-options/[deckId]/+page.svelte | 13 +++-
 ts/routes/deck-options/lib.ts                | 65 +++++++++++++++++++-
 4 files changed, 94 insertions(+), 35 deletions(-)

diff --git a/qt/aqt/deckoptions.py b/qt/aqt/deckoptions.py
index 0a1e778d18a..e4802f98ad4 100644
--- a/qt/aqt/deckoptions.py
+++ b/qt/aqt/deckoptions.py
@@ -62,41 +62,38 @@ def _on_bridge_cmd(self, cmd: str) -> None:
         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):
-        """Close if there was no modification. Otherwise ask for confirmation first."""
-
-        def callbackWithUserChoice(choice: int):
+    def confirm_discard_changes(self) -> None:
+        def callbackWithUserChoice(choice: int) -> None:
             if choice == 0:
                 # The user accepted to discard current input.
                 self._close()
 
-        def if_can_close_callback_with_data_information(has_modified_dataData: bool):
-            if has_modified_dataData:
-                ask_user_dialog(
-                    tr.card_templates_discard_changes(),
-                    callback=callbackWithUserChoice,
-                    buttons=[
-                        QMessageBox.StandardButton.Discard,
-                        (tr.adding_keep_editing(), QMessageBox.ButtonRole.RejectRole),
-                    ],
-                )
-            else:
-                self._close()
-
-        self.has_modified_data(if_can_close_callback_with_data_information)
+        ask_user_dialog(
+            tr.card_templates_discard_changes(),
+            callback=callbackWithUserChoice,
+            buttons=[
+                QMessageBox.StandardButton.Discard,
+                (tr.adding_keep_editing(), QMessageBox.ButtonRole.RejectRole),
+            ],
+            parent=self,
+        )
 
     def reject(self) -> None:
         self.mw.col.set_wants_abort()
@@ -105,12 +102,11 @@ def reject(self) -> None:
         saveGeom(self, self.TITLE)
         QDialog.reject(self)
 
-    def has_modified_data(self, callback: Callable[[bool], None]):
-        """Calls `callback` with the information of whether any deck options are modified."""
+    def check_pending_changes(self):
         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:
diff --git a/ts/routes/deck-options/SaveButton.svelte b/ts/routes/deck-options/SaveButton.svelte
index 247f8c66b0b..166ed003fdf 100644
--- a/ts/routes/deck-options/SaveButton.svelte
+++ b/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();
diff --git a/ts/routes/deck-options/[deckId]/+page.svelte b/ts/routes/deck-options/[deckId]/+page.svelte
index e3a5f6432a7..ec99facc152 100644
--- a/ts/routes/deck-options/[deckId]/+page.svelte
+++ b/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 = () => {
-        return data.state.isModified();
+    globalThis.anki.deckOptionsPendingChanges = async (): Promise<void> => {
+        await commitEditing();
+        if (bridgeCommandsAvailable()) {
+            if (data.state.isModified()) {
+                bridgeCommand("confirmDiscardChanges");
+            } else {
+                bridgeCommand("_close");
+            }
+        }
     };
+
     onMount(() => {
         globalThis.$deckOptions = new Promise((resolve, _reject) => {
             resolve(page);
diff --git a/ts/routes/deck-options/lib.ts b/ts/routes/deck-options/lib.ts
index 5df94ecfbeb..04f48793b1c 100644
--- a/ts/routes/deck-options/lib.ts
+++ b/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();
+}