diff --git a/rslib/src/scheduler/queue/builder/mod.rs b/rslib/src/scheduler/queue/builder/mod.rs index e97d44912..07786f2d8 100644 --- a/rslib/src/scheduler/queue/builder/mod.rs +++ b/rslib/src/scheduler/queue/builder/mod.rs @@ -166,6 +166,7 @@ impl QueueBuilder { learn_ahead_secs, selected_deck, current_day, + build_time: TimestampMillis::now(), current_learning_cutoff: now, } } diff --git a/rslib/src/scheduler/queue/learning.rs b/rslib/src/scheduler/queue/learning.rs index 3f3052fd1..0edb61264 100644 --- a/rslib/src/scheduler/queue/learning.rs +++ b/rslib/src/scheduler/queue/learning.rs @@ -35,7 +35,7 @@ impl CardQueues { /// Increase the cutoff to the current time, and increase the learning count /// for any new cards that now fall within the cutoff. - pub(super) fn update_learning_cutoff_and_count(&mut self) -> Box { + pub(super) fn update_learning_cutoff_and_count(&mut self) -> CutoffSnapshot { let change = CutoffSnapshot { learning_count: self.counts.learning, learning_cutoff: self.current_learning_cutoff, @@ -50,7 +50,7 @@ impl CardQueues { .count(); self.counts.learning += new_learning_cards; - Box::new(change) + change } /// Given the just-answered `card`, place it back in the learning queues if it's still diff --git a/rslib/src/scheduler/queue/mod.rs b/rslib/src/scheduler/queue/mod.rs index bd39040ec..3c39b3c8b 100644 --- a/rslib/src/scheduler/queue/mod.rs +++ b/rslib/src/scheduler/queue/mod.rs @@ -26,6 +26,7 @@ pub(crate) struct CardQueues { selected_deck: DeckId, current_day: u32, learn_ahead_secs: i64, + build_time: TimestampMillis, /// Updated each time a card is answered, and by get_queued_cards() when the /// counts are zero. Ensures we don't show a newly-due learning card after a /// user returns from editing a review card. @@ -190,12 +191,14 @@ impl Collection { if let Some(queues) = &mut self.state.card_queues { let entry = queues.pop_entry(card.id)?; let requeued_learning = queues.maybe_requeue_learning_card(card, timing); - let cutoff_change = queues.update_learning_cutoff_and_count(); + let cutoff_snapshot = queues.update_learning_cutoff_and_count(); + let queue_build_time = queues.build_time; self.save_queue_update_undo(Box::new(QueueUpdate { entry, learning_requeue: requeued_learning, + queue_build_time, + cutoff_snapshot, })); - self.save_cutoff_change(cutoff_change); } else { // we currenly allow the queues to be empty for unit tests } @@ -203,9 +206,40 @@ impl Collection { Ok(()) } + /// Get the card queues, building if necessary. pub(crate) fn get_queues(&mut self) -> Result<&mut CardQueues> { - let timing = self.timing_today()?; let deck = self.get_current_deck()?; + self.clear_queues_if_day_changed()?; + if self.state.card_queues.is_none() { + self.state.card_queues = Some(self.build_queues(deck.id)?); + } + + Ok(self.state.card_queues.as_mut().unwrap()) + } + + // Returns queues if they are valid and have not been rebuilt. If build time has changed, + // they are cleared. + pub(crate) fn get_or_invalidate_queues( + &mut self, + build_time: TimestampMillis, + ) -> Result> { + self.clear_queues_if_day_changed()?; + let same_build = self + .state + .card_queues + .as_ref() + .map(|q| q.build_time == build_time) + .unwrap_or_default(); + if same_build { + Ok(self.state.card_queues.as_mut()) + } else { + self.clear_study_queues(); + Ok(None) + } + } + + fn clear_queues_if_day_changed(&mut self) -> Result<()> { + let timing = self.timing_today()?; let day_rolled_over = self .state .card_queues @@ -215,11 +249,7 @@ impl Collection { if day_rolled_over { self.discard_undo_and_study_queues(); } - if self.state.card_queues.is_none() { - self.state.card_queues = Some(self.build_queues(deck.id)?); - } - - Ok(self.state.card_queues.as_mut().unwrap()) + Ok(()) } } diff --git a/rslib/src/scheduler/queue/undo.rs b/rslib/src/scheduler/queue/undo.rs index 8fdb13991..f268e6a52 100644 --- a/rslib/src/scheduler/queue/undo.rs +++ b/rslib/src/scheduler/queue/undo.rs @@ -8,13 +8,14 @@ use crate::prelude::*; pub(crate) enum UndoableQueueChange { CardAnswered(Box), CardAnswerUndone(Box), - CutoffChange(Box), } #[derive(Debug)] pub(crate) struct QueueUpdate { pub entry: QueueEntry, pub learning_requeue: Option, + pub queue_build_time: TimestampMillis, + pub cutoff_snapshot: CutoffSnapshot, } /// Stores the old learning count and cutoff prior to the @@ -29,30 +30,27 @@ impl Collection { pub(crate) fn undo_queue_change(&mut self, change: UndoableQueueChange) -> Result<()> { match change { UndoableQueueChange::CardAnswered(update) => { - let queues = self.get_queues()?; - if let Some(learning) = &update.learning_requeue { - queues.remove_intraday_learning_card(learning.id); + if let Some(queues) = self.get_or_invalidate_queues(update.queue_build_time)? { + queues.restore_cutoff(&update.cutoff_snapshot); + if let Some(learning) = &update.learning_requeue { + queues.remove_intraday_learning_card(learning.id); + } + queues.push_undo_entry(update.entry); } - queues.push_undo_entry(update.entry); self.save_undo(UndoableQueueChange::CardAnswerUndone(update)); Ok(()) } UndoableQueueChange::CardAnswerUndone(update) => { - let queues = self.get_queues()?; - queues.pop_entry(update.entry.card_id())?; - if let Some(learning) = update.learning_requeue { - queues.insert_intraday_learning_card(learning); + if let Some(queues) = self.get_or_invalidate_queues(update.queue_build_time)? { + queues.pop_entry(update.entry.card_id())?; + if let Some(learning) = update.learning_requeue { + queues.insert_intraday_learning_card(learning); + } + queues.restore_cutoff(&update.cutoff_snapshot); } self.save_undo(UndoableQueueChange::CardAnswered(update)); - Ok(()) - } - UndoableQueueChange::CutoffChange(change) => { - let queues = self.get_queues()?; - let current = queues.restore_cutoff(&change); - self.save_cutoff_change(current); - Ok(()) } } @@ -61,10 +59,6 @@ impl Collection { pub(super) fn save_queue_update_undo(&mut self, change: Box) { self.save_undo(UndoableQueueChange::CardAnswered(change)) } - - pub(super) fn save_cutoff_change(&mut self, change: Box) { - self.save_undo(UndoableQueueChange::CutoffChange(change)) - } } #[cfg(test)] @@ -216,9 +210,7 @@ mod test { // first card graduates col.answer_good(); assert_eq!(col.counts(), [0, 1, 0]); - // other card is all that is left, so the - // last step is deferred - col.answer_good(); + col.answer_easy(); assert_eq!(col.counts(), [0, 0, 0]); // now work backwards @@ -251,4 +243,38 @@ mod test { Ok(()) } + + #[test] + fn redo_after_queue_invalidation_bug() -> Result<()> { + // add a note to the default deck + let mut col = open_test_collection(); + let _nid = add_note(&mut col, true)?; + + // add a deck and select it + let mut deck = Deck::new_normal(); + deck.name = NativeDeckName::from_human_name("foo"); + col.add_deck(&mut deck)?; + col.set_current_deck(deck.id)?; + + // select default again, which invalidates current queues + col.set_current_deck(DeckId(1))?; + + // get the first card and answer it + col.answer_easy(); + + // undo answer + col.undo()?; + + // undo deck select, which invalidates the queues again + col.undo()?; + + // redo deck select (another invalidation) + col.redo()?; + + // when the card answer is redone, it shouldn't fail because + // the queues are rebuilt after the card state is restored + col.redo()?; + + Ok(()) + } }