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/FSRS progress sometime shows 0 reviews #3591

Merged

Conversation

L-M-Sherlock
Copy link
Contributor

@L-M-Sherlock L-M-Sherlock commented Nov 19, 2024

source: https://forums.ankiweb.net/t/anki-24-10-beta/49989/223?u=l.m.sherlock

If the optimization is done in 100ms, the thread cannot update the progress before the function return the results.

#3420 cannot deal with this case.

@dae
Copy link
Member

dae commented Nov 19, 2024

Appreciate the fix as always :-) But blocking the main thread like that is not ideal, nor is adding 100ms even when not necessary. @abdnh, can you think of a better approach?

@L-M-Sherlock
Copy link
Contributor Author

I think it's better than my previous idea.

progress_thread.join().ok();

But it still blocks the main thread.

@abdnh
Copy link
Collaborator

abdnh commented Nov 20, 2024

Hmm, the join solution doesn't seem to solve the issue according to my tests. Will do more tests.

@abdnh
Copy link
Collaborator

abdnh commented Nov 20, 2024

I think the issue is in the timeout hack I added here: 1a0dfc3

Adding a tick() call works better. The review count is now not shown until the alert is closed.

diff --git a/rslib/src/scheduler/fsrs/params.rs b/rslib/src/scheduler/fsrs/params.rs
index e01b260d6..bea1f5e82 100644
--- a/rslib/src/scheduler/fsrs/params.rs
+++ b/rslib/src/scheduler/fsrs/params.rs
@@ -81,7 +81,7 @@ impl Collection {
         // adapt the progress handler to our built-in progress handling
         let progress = CombinedProgressState::new_shared();
         let progress2 = progress.clone();
-        let progress_thread = thread::spawn(move || {
+        thread::spawn(move || {
             let mut finished = false;
             while !finished {
                 thread::sleep(Duration::from_millis(100));
@@ -98,7 +98,6 @@ impl Collection {
             }
         });
         let mut params = FSRS::new(None)?.compute_parameters(items.clone(), Some(progress2))?;
-        progress_thread.join().ok();
         if let Ok(fsrs) = FSRS::new(Some(current_params)) {
             let current_rmse = fsrs.evaluate(items.clone(), |_| true)?.rmse_bins;
             let optimized_fsrs = FSRS::new(Some(&params))?;
diff --git a/ts/routes/deck-options/FsrsOptions.svelte b/ts/routes/deck-options/FsrsOptions.svelte
index 6a3501ea4..0dd0a983d 100644
--- a/ts/routes/deck-options/FsrsOptions.svelte
+++ b/ts/routes/deck-options/FsrsOptions.svelte
@@ -41,6 +41,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     import TableData from "../graphs/TableData.svelte";
     import { defaultGraphBounds, type TableDatum } from "../graphs/graph-helpers";
     import InputBox from "../graphs/InputBox.svelte";
+    import { tick } from "svelte";

     export let state: DeckOptionsState;
     export let openHelpModal: (String) => void;
@@ -155,7 +156,8 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                             )) ||
                         resp.params.length === 0
                     ) {
-                        setTimeout(() => alert(tr.deckConfigFsrsParamsOptimal()), 100);
+                        await tick();
+                        alert(tr.deckConfigFsrsParamsOptimal());
                     } else {
                         $config.fsrsParams5 = resp.params;
                     }

@L-M-Sherlock
Copy link
Contributor Author

Adding a tick() call works better. The review count is now not shown until the alert is closed.

It doesn't work when the current params are not optimal:

image

@L-M-Sherlock
Copy link
Contributor Author

Now it works well in my cases.

@abdnh
Copy link
Collaborator

abdnh commented Nov 21, 2024

Had no luck reproducing "0 reviews" in the non-optimal case yet, but a .join() in the backend for that case and a timeout/tick() for the optimal case sounds like a good solution 👍 (still partial to tick() because it looks more elegant).

@L-M-Sherlock
Copy link
Contributor Author

Had no luck reproducing "0 reviews" in the non-optimal case yet

  1. Create a new deck with a new preset
  2. Move a old card with some review logs into the new deck
  3. Optimize in the new preset

@dae
Copy link
Member

dae commented Nov 23, 2024

Does that change work for you @L-M-Sherlock?

@L-M-Sherlock
Copy link
Contributor Author

image

tick() doesn't work for me.

image

@dae
Copy link
Member

dae commented Nov 24, 2024

Ok, thanks for clarifying. Let's not hold this up any further. :-)

@dae dae force-pushed the Fix/FSRS-progress-sometime-shows-0-reviews branch from 74fb337 to b1ead24 Compare November 24, 2024 10:51
@dae dae merged commit 9a013d8 into ankitects:main Nov 24, 2024
1 check passed
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.

3 participants