From b64f7a9456c9f094951c3b90973594c088981995 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Wed, 12 May 2021 12:00:15 +1000 Subject: [PATCH] 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. --- .../src/scheduler/queue/builder/gathering.rs | 71 +++++++++++++------ rslib/src/scheduler/queue/builder/mod.rs | 52 ++++++++++++-- rslib/src/storage/card/due_cards.sql | 3 +- rslib/src/storage/card/mod.rs | 2 + rslib/src/storage/card/new_cards.sql | 3 +- 5 files changed, 99 insertions(+), 32 deletions(-) diff --git a/rslib/src/scheduler/queue/builder/gathering.rs b/rslib/src/scheduler/queue/builder/gathering.rs index b3e527763..223121bea 100644 --- a/rslib/src/scheduler/queue/builder/gathering.rs +++ b/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 { + 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)); diff --git a/rslib/src/scheduler/queue/builder/mod.rs b/rslib/src/scheduler/queue/builder/mod.rs index a6351ba52..4ca543d02 100644 --- a/rslib/src/scheduler/queue/builder/mod.rs +++ b/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 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, pub(super) review: Vec, pub(super) learning: Vec, pub(super) day_learning: Vec, - pub(super) seen_note_ids: HashSet, + pub(super) seen_note_ids: HashMap, 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) })?; } } diff --git a/rslib/src/storage/card/due_cards.sql b/rslib/src/storage/card/due_cards.sql index 96241c48e..b5c8ee9fd 100644 --- a/rslib/src/storage/card/due_cards.sql +++ b/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 ( diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs index 44d1e5a38..092adedde 100644 --- a/rslib/src/storage/card/mod.rs +++ b/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; } diff --git a/rslib/src/storage/card/new_cards.sql b/rslib/src/storage/card/new_cards.sql index aa8ec5987..8f68f57e1 100644 --- a/rslib/src/storage/card/new_cards.sql +++ b/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 \ No newline at end of file