Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'Discard changes' dialog appearing even when no changes are made #3495

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

hikaru-y
Copy link
Contributor

Fixes #3491

Attaching a repro case provided by a user.
collection-2024-10-12@10-58-10.zip

I was unable to reproduce the issue using the provided collection or newly created profiles. However, I managed to reproduce it with some of my existing profiles, so I investigated the cause.

it appears to be the absence of limits.newToday/reviewToday in the current preset vs the original.

Firstly, I don't think the absence of limits.newToday/reviewToday should be the cause of the issue.

When working on #3478, I recognized that some of the properties of limits could change during the initialization of the DailyLimits component. For DailyLimits, all missing properties in the orginal config default to undefined. Therefore, in #3478, properties with the value undefined are excluded when comparing orginal and current to prevent false positives.

After running git bisect, I found that the issue has been introduced in #3442.

The issue is caused by the fact that the EasyDays component, like DailyLimits, may modify the config object during its initialization.

if ($config.easyDaysPercentages.length !== 7) {
$config.easyDaysPercentages = defaults.easyDaysPercentages;
}

Approach in this PR

Retrieve the original configs asynchronously after all components are mounted, allowing child components to adjust the config during their initialization phase.


Curiously when I hit esc instead of using the x on the window, no confirmation pops up.

From testing, it appears that this only happens if the user never interacts with the webview (e.g., clicking on the webview or pressing a key) from the time the window is opened until the Esc key is pressed. In this case, AnkiWebView's onEsc() is not called, and DeckOptionsDialog's reject() is called directly. As a result, check_pending_changes() is not executed.

I don't think this is something that needs to be addressed, because as a practical matter it's impossible for a user to change deck options without interacting with the webview at all, and the expected behavior is for the confirmation dialog not to pop up if no changes have been made.

Apart from the above, I found another issue while investigating.

Since #3410, we use evt.accept() instead of closeEvent() of the base class when closing the window, but it seems that evt.accept() does not trigger reject() afterwards. As a result, the cleanup process within reject() does not run. You can confirm this issue in Anki 24.10 beta (all of 1, 2, and 3) by the fact that if you close the window without saving the config, the geometry is not saved.

Approach in this PR
Call closeEvent() of the base class instead of evt.accept().

@dae
Copy link
Member

dae commented Oct 15, 2024

Great work Hikaru - the new approach is cleaner, and your research and commit message is excellent. Apologies for leading you up the wrong path with the limited triaging I did prior to posting this 🙇

@dae dae merged commit 694a508 into ankitects:main Oct 15, 2024
1 check passed
@hikaru-y hikaru-y deleted the fix-discard-changes-2 branch October 17, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deck options appear changed from start
2 participants