fix burying in test scheduler

The bury new/review flags are now pulled from each card's home deck,
instead of using a global setting that had not been hooked up. This
unfortunately means we need to fetch the map of all decks up front, as
we need to be able to look up a deck configuration for cards that are
in filtered decks.

Fixes a "card was modified" error caused by cards being buried during
review, when they weren't removed up-front.

rslib/src/scheduler/queue/builder/gathering.rs

@@ -1,7 +1,7 @@
 // Copyright: Ankitects Pty Ltd and contributors
 // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
-use super::{super::limits::RemainingLimits, DueCard, NewCard, QueueBuilder};
+use super::{super::limits::RemainingLimits, BuryMode, DueCard, NewCard, QueueBuilder};
 use crate::{card::CardQueue, prelude::*};
 
 impl QueueBuilder {
@@ -12,8 +12,12 @@ impl QueueBuilder {
         limit: &mut RemainingLimits,
         queue: CardQueue,
         card: DueCard,
+        bury_mode: BuryMode,
     ) -> bool {
-        let should_add = self.should_add_review_card(card.note_id);
+        let bury_reviews = self
+            .get_and_update_bury_mode_for_note(card.note_id, bury_mode)
+            .map(|mode| mode.bury_reviews)
+            .unwrap_or_default();
 
         match queue {
             CardQueue::Learn | CardQueue::PreviewRepeat => self.learning.push(card),
@@ -21,7 +25,7 @@ impl QueueBuilder {
                 self.day_learning.push(card);
             }
             CardQueue::Review => {
-                if should_add {
+                if !bury_reviews {
                     self.review.push(card);
                     limit.review -= 1;
                 }
@@ -41,13 +45,18 @@ impl QueueBuilder {
         &mut self,
         limit: &mut RemainingLimits,
         card: NewCard,
+        bury_mode: BuryMode,
     ) -> bool {
-        let already_seen = self.have_seen_note_id(card.note_id);
-        if !already_seen {
+        let previous_bury_mode = self
+            .get_and_update_bury_mode_for_note(card.note_id, bury_mode)
+            .map(|mode| mode.bury_new);
+        // no previous siblings seen?
+        if previous_bury_mode.is_none() {
             self.new.push(card);
             limit.new -= 1;
             return limit.new != 0;
         }
+        let bury_new = previous_bury_mode.unwrap();
 
         // Cards will be arriving in (due, card_id) order, with all
         // siblings sharing the same due number by default. In the
@@ -65,13 +74,13 @@ impl QueueBuilder {
             .unwrap_or(false);
 
         if previous_card_was_sibling_with_higher_ordinal {
-            if self.bury_new {
+            if bury_new {
                 // When burying is enabled, we replace the existing sibling
                 // with the lower ordinal one.
                 *self.new.last_mut().unwrap() = card;
             } else {
-                // When burying disabled, we'll want to add this card as well, but
-                // not at the end of the list.
+                // When burying disabled, we'll want to add this card as well, but we
+                // need to insert it in front of the later-ordinal card(s).
                 let target_idx = self
                     .new
                     .iter()
@@ -91,7 +100,7 @@ impl QueueBuilder {
             }
         } else {
             // card has arrived in expected order - add if burying disabled
-            if !self.bury_new {
+            if !bury_new {
                 self.new.push(card);
                 limit.new -= 1;
             }
@@ -100,13 +109,25 @@ impl QueueBuilder {
         limit.new != 0
     }
 
-    fn should_add_review_card(&mut self, note_id: NoteId) -> bool {
-        !self.have_seen_note_id(note_id) || !self.bury_reviews
-    }
+    /// If burying is enabled in `new_settings`, existing entry will be updated.
+    /// Returns a copy made before changing the entry, so that a card with burying
+    /// enabled will bury future siblings, but not itself.
+    fn get_and_update_bury_mode_for_note(
+        &mut self,
+        note_id: NoteId,
+        new_mode: BuryMode,
+    ) -> Option<BuryMode> {
+        let mut previous_mode = None;
+        self.seen_note_ids
+            .entry(note_id)
+            .and_modify(|entry| {
+                previous_mode = Some(*entry);
+                entry.bury_new |= new_mode.bury_new;
+                entry.bury_reviews |= new_mode.bury_reviews;
+            })
+            .or_insert(new_mode);
 
-    /// Mark note seen, and return true if seen before.
-    fn have_seen_note_id(&mut self, note_id: NoteId) -> bool {
-        !self.seen_note_ids.insert(note_id)
+        previous_mode
     }
 }
 
@@ -116,10 +137,7 @@ mod test {
 
     #[test]
     fn new_siblings() {
-        let mut builder = QueueBuilder {
-            bury_new: true,
-            ..Default::default()
-        };
+        let mut builder = QueueBuilder::default();
         let mut limits = RemainingLimits {
             review: 0,
             new: 100,
@@ -147,13 +165,21 @@ mod test {
             NewCard {
                 id: CardId(4),
                 note_id: NoteId(2),
+                // lowest ordinal, should be used instead of card 2/3
                 extra: 0,
                 ..Default::default()
             },
         ];
 
         for card in &cards {
-            builder.add_new_card(&mut limits, card.clone());
+            builder.add_new_card(
+                &mut limits,
+                card.clone(),
+                BuryMode {
+                    bury_new: true,
+                    ..Default::default()
+                },
+            );
         }
 
         assert_eq!(builder.new[0].id, CardId(1));
@@ -161,11 +187,10 @@ mod test {
         assert_eq!(builder.new.len(), 2);
 
         // with burying disabled, we should get all siblings in order
-        builder.bury_new = false;
-        builder.new.truncate(0);
+        let mut builder = QueueBuilder::default();
 
         for card in &cards {
-            builder.add_new_card(&mut limits, card.clone());
+            builder.add_new_card(&mut limits, card.clone(), Default::default());
         }
 
         assert_eq!(builder.new[0].id, CardId(1));

rslib/src/scheduler/queue/builder/mod.rs

@@ -8,7 +8,7 @@ mod sorting;
 
 use std::{
     cmp::Reverse,
-    collections::{BinaryHeap, HashSet, VecDeque},
+    collections::{BinaryHeap, HashMap, VecDeque},
 };
 
 use intersperser::Intersperser;
@@ -32,6 +32,7 @@ pub(crate) struct DueCard {
     pub due: i32,
     pub interval: u32,
     pub hash: u64,
+    pub original_deck_id: DeckId,
 }
 
 /// Temporary holder for new cards that will be built into a queue.
@@ -41,6 +42,7 @@ pub(crate) struct NewCard {
     pub note_id: NoteId,
     pub mtime: TimestampSecs,
     pub due: i32,
+    pub original_deck_id: DeckId,
     /// Used to store template_idx, and for shuffling
     pub extra: u64,
 }
@@ -75,19 +77,25 @@ impl From<DueCard> for LearningQueueEntry {
     }
 }
 
+/// When we encounter a card with new or review burying enabled, all future
+/// siblings need to be buried, regardless of their own settings.
+#[derive(Default, Debug, Clone, Copy)]
+pub(super) struct BuryMode {
+    bury_new: bool,
+    bury_reviews: bool,
+}
+
 #[derive(Default)]
 pub(super) struct QueueBuilder {
     pub(super) new: Vec<NewCard>,
     pub(super) review: Vec<DueCard>,
     pub(super) learning: Vec<DueCard>,
     pub(super) day_learning: Vec<DueCard>,
-    pub(super) seen_note_ids: HashSet<NoteId>,
+    pub(super) seen_note_ids: HashMap<NoteId, BuryMode>,
     pub(super) new_order: NewCardOrder,
     pub(super) review_order: ReviewCardOrder,
     pub(super) day_learn_mix: ReviewMix,
     pub(super) new_review_mix: ReviewMix,
-    pub(super) bury_new: bool,
-    pub(super) bury_reviews: bool,
 }
 
 impl QueueBuilder {
@@ -197,10 +205,10 @@ impl Collection {
         let now = TimestampSecs::now();
         let timing = self.timing_for_timestamp(now)?;
         let (decks, parent_count) = self.storage.deck_with_parents_and_children(deck_id)?;
+        let deck_map = self.storage.get_decks_map()?;
         let config = self.storage.get_deck_config_map()?;
         let limits = remaining_limits_capped_to_parents(&decks, &config, timing.days_elapsed);
         let selected_deck_limits = limits[parent_count];
-
         let mut queues = QueueBuilder::default();
 
         for (deck, mut limit) in decks.iter().zip(limits).skip(parent_count) {
@@ -209,12 +217,42 @@ impl Collection {
                     timing.days_elapsed,
                     timing.next_day_at,
                     deck.id,
-                    |queue, card| queues.add_due_card(&mut limit, queue, card),
+                    |queue, card| {
+                        let home_deck = if card.original_deck_id.0 == 0 {
+                            deck.id
+                        } else {
+                            card.original_deck_id
+                        };
+                        let bury = deck_map
+                            .get(&home_deck)
+                            .and_then(|deck| deck.config_id())
+                            .and_then(|config_id| config.get(&config_id))
+                            .map(|config| BuryMode {
+                                bury_new: config.inner.bury_new,
+                                bury_reviews: config.inner.bury_reviews,
+                            })
+                            .unwrap_or_default();
+                        queues.add_due_card(&mut limit, queue, card, bury)
+                    },
                 )?;
             }
             if limit.new > 0 {
                 self.storage.for_each_new_card_in_deck(deck.id, |card| {
-                    queues.add_new_card(&mut limit, card)
+                    let home_deck = if card.original_deck_id.0 == 0 {
+                        deck.id
+                    } else {
+                        card.original_deck_id
+                    };
+                    let bury = deck_map
+                        .get(&home_deck)
+                        .and_then(|deck| deck.config_id())
+                        .and_then(|config_id| config.get(&config_id))
+                        .map(|config| BuryMode {
+                            bury_new: config.inner.bury_new,
+                            bury_reviews: config.inner.bury_reviews,
+                        })
+                        .unwrap_or_default();
+                    queues.add_new_card(&mut limit, card, bury)
                 })?;
             }
         }

rslib/src/storage/card/due_cards.sql

@@ -3,7 +3,8 @@ SELECT queue,
   nid,
   due,
   cast(ivl AS integer),
-  cast(mod AS integer)
+  cast(mod AS integer),
+  odid
 FROM cards
 WHERE did = ?1
   AND (

rslib/src/storage/card/mod.rs

@@ -194,6 +194,7 @@ impl super::SqliteStorage {
                     interval: row.get(4)?,
                     mtime: row.get(5)?,
                     hash: 0,
+                    original_deck_id: row.get(6)?,
                 },
             ) {
                 break;
@@ -218,6 +219,7 @@ impl super::SqliteStorage {
                 due: row.get(2)?,
                 extra: row.get::<_, u32>(3)? as u64,
                 mtime: row.get(4)?,
+                original_deck_id: row.get(5)?,
             }) {
                 break;
             }

rslib/src/storage/card/new_cards.sql

@@ -2,7 +2,8 @@ SELECT id,
   nid,
   due,
   ord,
-  cast(mod AS integer)
+  cast(mod AS integer),
+  odid
 FROM cards
 WHERE did = ?
   AND queue = 0