From 62223499c41848803ddea0dc0e29d713a58f275e Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sat, 21 Aug 2021 19:07:25 +1000 Subject: [PATCH] fix errors when undoing/redoing after a queue-invalidating operation There were a few issues going on here: - If some operation had invalidated the queues, they were subsequently recreated with a call to .get_queues() in the undo handling code. This could happen after the changes to the card had already been reverted, leading to a queue state that didn't match our expectations. - More generally, it's not safe to assume our mutations will apply cleanly after the queue has been rebuilt. The next card will vary depending on the number of remaining cards when interspersing cards of different types, and a queue-invalidating operation will have changed the learning cutoff. So rather than rebuilding the queues on demand, we now check that they already exist, and were created at the time we expect. If not, we invalidate them and skip applying the mutations, and a subsequent refresh of the UI should rebuild the queues correctly. As part of this change, the cutoff snapshot has been moved into the normal answer update object. One possible downside here is that adding a note during review may cause a newly due learning card to appear when undoing a different review. If this proves to be a problem, we could potentially note down the learning cutoff and apply it when queues are rebuilt later. --- rslib/src/scheduler/queue/builder/mod.rs | 1 + rslib/src/scheduler/queue/learning.rs | 4 +- rslib/src/scheduler/queue/mod.rs | 46 ++++++++++++--- rslib/src/scheduler/queue/undo.rs | 72 ++++++++++++++++-------- 4 files changed, 90 insertions(+), 33 deletions(-) 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(()) + } }