Skip to content

Conversation

graves
Copy link
Contributor

@graves graves commented Mar 8, 2025

This PR fixes a bug reported on the forums.

Steps to reproduce the bug.

  1. Create a new profile so that everything is set to default.
  2. Create a new card.
  3. Click Good.
  4. Open deck options and empty learning steps. Save.
  5. No go back and put 1m 10m as LS.
  6. Go back to the card and it should show 10m on the Good button.

Changes

Add a condition to check if old_steps is empty and if it is use the remaining steps as the new_remaining steps. Add a test.

Evidence

skipped-learning-steps-fix.mov

close #3699

graves added 4 commits March 8, 2025 16:20
Bug report reproduction steps:
Create a new profile so that everything is set to default.
Create a new card.
Click Good.
Open deck options and empty learning steps. Save.
No go back and put 1m 10m as LS.
Go back to the card and it should show 10m on the Good button.

Check if old_steps is empty and if it is just use remaining
steps for the new_remaining steps. Add test.
Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this Thomas! Just a minor style issue, and a conflict on the CONTRIBUTORS file to resolve and then I can merge it in.

@@ -490,6 +496,9 @@ impl From<MemoryState> for FsrsMemoryState {

#[cfg(test)]
mod test {
use crate::prelude::AnkiError;
use crate::prelude::Collection;
use crate::prelude::DeckId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more idiomatic to import crate::prelude::* and/or super::*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thank you!

@graves
Copy link
Contributor Author

graves commented Mar 11, 2025

Thanks for digging into this Thomas! Just a minor style issue, and a conflict on the CONTRIBUTORS file to resolve and then I can merge it in.

Fixed and ready for merge! Thank you!

@dae
Copy link
Member

dae commented Mar 14, 2025

Thank you too!

@dae dae merged commit f5f22ed into ankitects:main Mar 14, 2025
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.

Better handle step changes?
2 participants