Adjust remaining steps after config update (#1956)

* Adjust remaining steps after config update

* Handle relearning steps separately

Also refactor a lot.

* Also adjust remaining steps after deck change

* Test step adjustment after config update

* Fix `SearchBuilder::(re)learning_cards()`

* Fix step adjustment after deck change

* Test step adjustment after deck change

* Fix test name

* Readjust remaining steps according to last delay

Also atomize tests and add some tooling.

rslib/src/card/mod.rs

@@ -3,7 +3,7 @@
 
 pub(crate) mod undo;
 
-use std::collections::HashSet;
+use std::collections::{hash_map::Entry, HashMap, HashSet};
 
 use num_enum::TryFromPrimitive;
 use serde_repr::{Deserialize_repr, Serialize_repr};
@@ -145,9 +145,7 @@ impl Card {
     pub fn is_intraday_learning(&self) -> bool {
         matches!(self.queue, CardQueue::Learn | CardQueue::PreviewRepeat)
     }
-}
 
-impl Card {
     pub fn new(note_id: NoteId, template_idx: u16, deck_id: DeckId, due: i32) -> Self {
         Card {
             note_id,
@@ -157,6 +155,27 @@ impl Card {
             ..Default::default()
         }
     }
+
+    /// Remaining steps after configured steps have changed, disregarding "remaining today".
+    /// [None] if same as before. A step counts as remaining if the card has not passed a step
+    /// with the same or a greater delay, but output will be at least 1.
+    fn new_remaining_steps(&self, new_steps: &[f32], old_steps: &[f32]) -> Option<u32> {
+        let remaining = self.remaining_steps();
+        let new_remaining = old_steps
+            .len()
+            .checked_sub(remaining as usize + 1)
+            .and_then(|last_index| {
+                new_steps
+                    .iter()
+                    .rev()
+                    .position(|&step| step <= old_steps[last_index])
+            })
+            // no last delay or last delay is less than all new steps → all steps remain
+            .unwrap_or(new_steps.len())
+            // (re)learning card must have at least 1 step remaining
+            .max(1) as u32;
+        (remaining != new_remaining).then(|| new_remaining)
+    }
 }
 
 impl Collection {
@@ -250,9 +269,11 @@ impl Collection {
 
     pub fn set_deck(&mut self, cards: &[CardId], deck_id: DeckId) -> Result<OpOutput<usize>> {
         let deck = self.get_deck(deck_id)?.ok_or(AnkiError::NotFound)?;
-        if deck.is_filtered() {
+        let config_id = deck.config_id().ok_or(AnkiError::FilteredDeckError(
-            return Err(FilteredDeckError::CanNotMoveCardsInto.into());
+            FilteredDeckError::CanNotMoveCardsInto,
-        }
+        ))?;
+        let config = self.get_deck_config(config_id, true)?.unwrap();
+        let mut steps_adjuster = RemainingStepsAdjuster::new(&config);
         self.storage.set_search_table_to_card_ids(cards, false)?;
         let sched = self.scheduler_version();
         let usn = self.usn()?;
@@ -264,6 +285,7 @@ impl Collection {
                 }
                 count += 1;
                 let original = card.clone();
+                steps_adjuster.adjust_remaining_steps(col, &mut card)?;
                 card.set_deck(deck_id, sched);
                 col.update_card_inner(&mut card, original, usn)?;
             }
@@ -306,4 +328,95 @@ impl Collection {
 
         Ok(DeckConfig::default())
     }
+
+    /// Adjust the remaining steps of the card according to the steps change.
+    /// Steps must be learning or relearning steps according to the card's type.
+    pub(crate) fn adjust_remaining_steps(
+        &mut self,
+        card: &mut Card,
+        old_steps: &[f32],
+        new_steps: &[f32],
+        usn: Usn,
+    ) -> Result<()> {
+        if let Some(new_remaining) = card.new_remaining_steps(new_steps, old_steps) {
+            let original = card.clone();
+            card.remaining_steps = new_remaining;
+            self.update_card_inner(card, original, usn)
+        } else {
+            Ok(())
+        }
+    }
+}
+
+/// Adjusts the remaining steps of cards after their deck config has changed.
+struct RemainingStepsAdjuster<'a> {
+    learn_steps: &'a [f32],
+    relearn_steps: &'a [f32],
+    configs: HashMap<DeckId, DeckConfig>,
+}
+
+impl<'a> RemainingStepsAdjuster<'a> {
+    fn new(new_config: &'a DeckConfig) -> Self {
+        RemainingStepsAdjuster {
+            learn_steps: &new_config.inner.learn_steps,
+            relearn_steps: &new_config.inner.relearn_steps,
+            configs: HashMap::new(),
+        }
+    }
+
+    fn adjust_remaining_steps(&mut self, col: &mut Collection, card: &mut Card) -> Result<()> {
+        if let Some(remaining) = match card.ctype {
+            CardType::Learn => card.new_remaining_steps(
+                self.learn_steps,
+                &self.config_for_card(col, card)?.inner.learn_steps,
+            ),
+            CardType::Relearn => card.new_remaining_steps(
+                self.relearn_steps,
+                &self.config_for_card(col, card)?.inner.relearn_steps,
+            ),
+            _ => None,
+        } {
+            card.remaining_steps = remaining;
+        }
+        Ok(())
+    }
+
+    fn config_for_card(&mut self, col: &mut Collection, card: &Card) -> Result<&mut DeckConfig> {
+        Ok(
+            match self.configs.entry(card.original_or_current_deck_id()) {
+                Entry::Occupied(e) => e.into_mut(),
+                Entry::Vacant(e) => e.insert(col.deck_config_for_card(card)?),
+            },
+        )
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use crate::tests::{
+        open_test_collection_with_learning_card, open_test_collection_with_relearning_card,
+        DeckAdder,
+    };
+
+    #[test]
+    fn should_increase_remaining_learning_steps_if_new_deck_has_more_unpassed_ones() {
+        let mut col = open_test_collection_with_learning_card();
+        let deck = DeckAdder::new("target")
+            .with_config(|config| config.inner.learn_steps.push(100.))
+            .add(&mut col);
+        let card_id = col.get_first_card().id;
+        col.set_deck(&[card_id], deck.id).unwrap();
+        assert_eq!(col.get_first_card().remaining_steps, 3);
+    }
+
+    #[test]
+    fn should_increase_remaining_relearning_steps_if_new_deck_has_more_unpassed_ones() {
+        let mut col = open_test_collection_with_relearning_card();
+        let deck = DeckAdder::new("target")
+            .with_config(|config| config.inner.relearn_steps.push(100.))
+            .add(&mut col);
+        let card_id = col.get_first_card().id;
+        col.set_deck(&[card_id], deck.id).unwrap();
+        assert_eq!(col.get_first_card().remaining_steps, 2);
+    }
 }

rslib/src/deckconfig/update.rs

@@ -13,6 +13,7 @@ use crate::{
     pb,
     pb::deck_configs_for_update::{ConfigWithExtra, CurrentDeck},
     prelude::*,
+    search::{JoinSearches, SearchNode},
 };
 
 #[derive(Debug, Clone)]
@@ -153,8 +154,8 @@ impl Collection {
 
                 // previous order
                 let previous_config_id = DeckConfigId(normal.config_id);
-                let previous_order = configs_before_update
+                let previous_config = configs_before_update.get(&previous_config_id);
-                    .get(&previous_config_id)
+                let previous_order = previous_config
                     .map(|c| c.inner.new_card_insert_order())
                     .unwrap_or_default();
 
@@ -171,13 +172,15 @@ impl Collection {
                 };
 
                 // if new order differs, deck needs re-sorting
-                let current_order = configs_after_update
+                let current_config = configs_after_update.get(&current_config_id);
-                    .get(&current_config_id)
+                let current_order = current_config
                     .map(|c| c.inner.new_card_insert_order())
                     .unwrap_or_default();
                 if previous_order != current_order {
                     self.sort_deck(deck_id, current_order, usn)?;
                 }
+
+                self.adjust_remaining_steps_in_deck(deck_id, previous_config, current_config, usn)?;
             }
         }
 
@@ -185,12 +188,51 @@ impl Collection {
 
         Ok(())
     }
+
+    /// Adjust the remaining steps of cards in the given deck according to the config change.
+    fn adjust_remaining_steps_in_deck(
+        &mut self,
+        deck: DeckId,
+        previous_config: Option<&DeckConfig>,
+        current_config: Option<&DeckConfig>,
+        usn: Usn,
+    ) -> Result<()> {
+        if let (Some(old), Some(new)) = (previous_config, current_config) {
+            for (search, old_steps, new_steps) in [
+                (
+                    SearchBuilder::learning_cards(),
+                    &old.inner.learn_steps,
+                    &new.inner.learn_steps,
+                ),
+                (
+                    SearchBuilder::relearning_cards(),
+                    &old.inner.relearn_steps,
+                    &new.inner.relearn_steps,
+                ),
+            ] {
+                if old_steps == new_steps {
+                    continue;
+                }
+                let search = search.clone().and(SearchNode::from_deck_id(deck, false));
+                for mut card in self.all_cards_for_search(search)? {
+                    self.adjust_remaining_steps(&mut card, old_steps, new_steps, usn)?;
+                }
+            }
+        }
+        Ok(())
+    }
 }
 
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::{collection::open_test_collection, deckconfig::NewCardInsertOrder};
+    use crate::{
+        collection::open_test_collection,
+        deckconfig::NewCardInsertOrder,
+        tests::{
+            open_test_collection_with_learning_card, open_test_collection_with_relearning_card,
+        },
+    };
 
     #[test]
     fn updating() -> Result<()> {
@@ -284,4 +326,66 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn should_increase_remaining_learning_steps_if_unpassed_learning_step_added() {
+        let mut col = open_test_collection_with_learning_card();
+        col.set_default_learn_steps(vec![1., 10., 100.]);
+        assert_eq!(col.get_first_card().remaining_steps, 3);
+    }
+
+    #[test]
+    fn should_keep_remaining_learning_steps_if_unpassed_relearning_step_added() {
+        let mut col = open_test_collection_with_learning_card();
+        col.set_default_relearn_steps(vec![1., 10., 100.]);
+        assert_eq!(col.get_first_card().remaining_steps, 2);
+    }
+
+    #[test]
+    fn should_keep_remaining_learning_steps_if_passed_learning_step_added() {
+        let mut col = open_test_collection_with_learning_card();
+        col.answer_good();
+        col.set_default_learn_steps(vec![1., 1., 10.]);
+        assert_eq!(col.get_first_card().remaining_steps, 1);
+    }
+
+    #[test]
+    fn should_keep_at_least_one_remaining_learning_step() {
+        let mut col = open_test_collection_with_learning_card();
+        col.answer_good();
+        col.set_default_learn_steps(vec![1.]);
+        assert_eq!(col.get_first_card().remaining_steps, 1);
+    }
+
+    #[test]
+    fn should_increase_remaining_relearning_steps_if_unpassed_relearning_step_added() {
+        let mut col = open_test_collection_with_relearning_card();
+        col.set_default_relearn_steps(vec![1., 10., 100.]);
+        assert_eq!(col.get_first_card().remaining_steps, 3);
+    }
+
+    #[test]
+    fn should_keep_remaining_relearning_steps_if_unpassed_learning_step_added() {
+        let mut col = open_test_collection_with_relearning_card();
+        col.set_default_learn_steps(vec![1., 10., 100.]);
+        assert_eq!(col.get_first_card().remaining_steps, 1);
+    }
+
+    #[test]
+    fn should_keep_remaining_relearning_steps_if_passed_relearning_step_added() {
+        let mut col = open_test_collection_with_relearning_card();
+        col.set_default_relearn_steps(vec![10., 100.]);
+        col.answer_good();
+        col.set_default_relearn_steps(vec![1., 10., 100.]);
+        assert_eq!(col.get_first_card().remaining_steps, 1);
+    }
+
+    #[test]
+    fn should_keep_at_least_one_remaining_relearning_step() {
+        let mut col = open_test_collection_with_relearning_card();
+        col.set_default_relearn_steps(vec![10., 100.]);
+        col.answer_good();
+        col.set_default_relearn_steps(vec![1.]);
+        assert_eq!(col.get_first_card().remaining_steps, 1);
+    }
 }

rslib/src/decks/name.rs

@@ -6,7 +6,7 @@ use itertools::Itertools;
 
 use crate::{prelude::*, text::normalize_to_nfc};
 
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Default, PartialEq)]
 pub struct NativeDeckName(String);
 
 impl NativeDeckName {

rslib/src/search/builder.rs

@@ -113,6 +113,21 @@ impl SearchBuilder {
     pub fn write(&self) -> String {
         write_nodes(&self.0)
     }
+
+    /// Construct [SearchBuilder] matching any given deck, excluding children.
+    pub fn from_decks(decks: &[DeckId]) -> Self {
+        Self::any(decks.iter().copied().map(SearchNode::DeckIdWithoutChildren))
+    }
+
+    /// Construct [SearchBuilder] matching learning, but not relearning cards.
+    pub fn learning_cards() -> Self {
+        StateKind::Learning.and(StateKind::Review.negated())
+    }
+
+    /// Construct [SearchBuilder] matching relearning cards.
+    pub fn relearning_cards() -> Self {
+        StateKind::Learning.and(StateKind::Review)
+    }
 }
 
 impl<T: Into<Node>> From<T> for SearchBuilder {

rslib/src/search/mod.rs

@@ -18,7 +18,7 @@ pub use writer::replace_search_node;
 
 use crate::{
     browser_table::Column,
-    card::{CardId, CardType},
+    card::{Card, CardId, CardType},
     collection::Collection,
     error::Result,
     notes::NoteId,
@@ -216,6 +216,16 @@ impl Collection {
             .map_err(Into::into)
     }
 
+    pub(crate) fn all_cards_for_search<N>(&mut self, search: N) -> Result<Vec<Card>>
+    where
+        N: TryIntoSearch,
+    {
+        self.search_cards_into_table(search, SortMode::NoOrder)?;
+        let cards = self.storage.all_searched_cards();
+        self.storage.clear_searched_cards_table()?;
+        cards
+    }
+
     /// Place the matched note ids into a temporary 'search_nids' table
     /// instead of returning them. Use clear_searched_notes() to remove it.
     /// Returns number of added notes.

rslib/src/storage/card/mod.rs

@@ -653,6 +653,17 @@ impl super::SqliteStorage {
 
         Ok(())
     }
+
+    #[cfg(test)]
+    pub(crate) fn get_all_cards(&self) -> Vec<Card> {
+        self.db
+            .prepare("SELECT * FROM cards")
+            .unwrap()
+            .query_and_then([], row_to_card)
+            .unwrap()
+            .collect::<rusqlite::Result<_>>()
+            .unwrap()
+    }
 }
 
 #[derive(Clone, Copy)]

rslib/src/tests.rs

@@ -5,7 +5,12 @@
 
 use tempfile::{tempdir, TempDir};
 
-use crate::{collection::CollectionBuilder, media::MediaManager, prelude::*};
+use crate::{
+    collection::{open_test_collection, CollectionBuilder},
+    deckconfig::UpdateDeckConfigsRequest,
+    media::MediaManager,
+    prelude::*,
+};
 
 pub(crate) fn open_fs_test_collection(name: &str) -> (Collection, TempDir) {
     let tempdir = tempdir().unwrap();
@@ -19,6 +24,26 @@ pub(crate) fn open_fs_test_collection(name: &str) -> (Collection, TempDir) {
     (col, tempdir)
 }
 
+pub(crate) fn open_test_collection_with_learning_card() -> Collection {
+    let mut col = open_test_collection();
+    col.add_new_note("basic");
+    col.answer_again();
+    col
+}
+
+pub(crate) fn open_test_collection_with_relearning_card() -> Collection {
+    let mut col = open_test_collection();
+    col.add_new_note("basic");
+    col.answer_easy();
+    col.storage
+        .db
+        .execute_batch("UPDATE cards SET due = 0")
+        .unwrap();
+    col.clear_study_queues();
+    col.answer_again();
+    col
+}
+
 impl Collection {
     pub(crate) fn add_media(&self, media: &[(&str, &[u8])]) {
         let mgr = MediaManager::new(&self.media_folder, &self.media_db).unwrap();
@@ -57,6 +82,37 @@ impl Collection {
         self.add_deck_inner(&mut deck, Usn(1)).unwrap();
         deck
     }
+
+    pub(crate) fn get_first_card(&self) -> Card {
+        self.storage.get_all_cards().pop().unwrap()
+    }
+
+    pub(crate) fn get_first_deck_config(&mut self) -> DeckConfig {
+        self.storage.all_deck_config().unwrap().pop().unwrap()
+    }
+
+    pub(crate) fn set_default_learn_steps(&mut self, steps: Vec<f32>) {
+        let mut config = self.get_first_deck_config();
+        config.inner.learn_steps = steps;
+        self.update_default_deck_config(config);
+    }
+
+    pub(crate) fn set_default_relearn_steps(&mut self, steps: Vec<f32>) {
+        let mut config = self.get_first_deck_config();
+        config.inner.relearn_steps = steps;
+        self.update_default_deck_config(config);
+    }
+
+    pub(crate) fn update_default_deck_config(&mut self, config: DeckConfig) {
+        self.update_deck_configs(UpdateDeckConfigsRequest {
+            target_deck_id: DeckId(1),
+            configs: vec![config],
+            removed_config_ids: vec![],
+            apply_to_children: false,
+            card_state_customizer: "".to_string(),
+        })
+        .unwrap();
+    }
 }
 
 pub(crate) fn new_deck_with_machine_name(name: &str, filtered: bool) -> Deck {
@@ -68,3 +124,49 @@ pub(crate) fn new_deck_with_machine_name(name: &str, filtered: bool) -> Deck {
     deck.name = NativeDeckName::from_native_str(name);
     deck
 }
+
+#[derive(Debug, Default, Clone)]
+pub(crate) struct DeckAdder {
+    name: NativeDeckName,
+    filtered: bool,
+    config: Option<DeckConfig>,
+}
+
+impl DeckAdder {
+    pub(crate) fn new(machine_name: impl Into<String>) -> Self {
+        Self {
+            name: NativeDeckName::from_native_str(machine_name),
+            ..Default::default()
+        }
+    }
+
+    #[allow(dead_code)]
+    pub(crate) fn filtered(mut self, filtered: bool) -> Self {
+        self.filtered = filtered;
+        self
+    }
+
+    pub(crate) fn with_config(mut self, modifier: impl FnOnce(&mut DeckConfig)) -> Self {
+        let mut config = DeckConfig::default();
+        modifier(&mut config);
+        self.config = Some(config);
+        self
+    }
+
+    pub(crate) fn add(self, col: &mut Collection) -> Deck {
+        let mut deck = if self.filtered {
+            Deck::new_filtered()
+        } else {
+            Deck::new_normal()
+        };
+        deck.name = self.name;
+        if let Some(mut config) = self.config {
+            col.add_or_update_deck_config(&mut config).unwrap();
+            deck.normal_mut()
+                .expect("can't set config for filtered deck")
+                .config_id = config.id.0;
+        }
+        col.add_or_update_deck(&mut deck).unwrap();
+        deck
+    }
+}