diff --git a/rslib/src/scheduler/answering/mod.rs b/rslib/src/scheduler/answering/mod.rs index 9e319ee03..78a9f8d51 100644 --- a/rslib/src/scheduler/answering/mod.rs +++ b/rslib/src/scheduler/answering/mod.rs @@ -7,7 +7,6 @@ mod preview; mod relearning; mod review; mod revlog; -mod undo; use revlog::RevlogEntryPartial; @@ -375,6 +374,43 @@ impl Collection { } } +// test helpers +#[cfg(test)] +impl Collection { + pub(crate) fn answer_again(&mut self) { + self.answer(|states| states.again, Rating::Again).unwrap() + } + + #[allow(dead_code)] + pub(crate) fn answer_hard(&mut self) { + self.answer(|states| states.hard, Rating::Hard).unwrap() + } + + pub(crate) fn answer_good(&mut self) { + self.answer(|states| states.good, Rating::Good).unwrap() + } + + pub(crate) fn answer_easy(&mut self) { + self.answer(|states| states.easy, Rating::Easy).unwrap() + } + + fn answer(&mut self, get_state: F, rating: Rating) -> Result<()> + where + F: FnOnce(&NextCardStates) -> CardState, + { + let queued = self.next_card()?.unwrap(); + self.answer_card(&CardAnswer { + card_id: queued.card.id, + current_state: queued.next_states.current, + new_state: get_state(&queued.next_states), + rating, + answered_at: TimestampMillis::now(), + milliseconds_taken: 0, + })?; + Ok(()) + } +} + /// Return a consistent seed for a given card at a given number of reps. /// If in test environment, disable fuzzing. fn get_fuzz_seed(card: &Card) -> Option { diff --git a/rslib/src/scheduler/answering/undo.rs b/rslib/src/scheduler/answering/undo.rs deleted file mode 100644 index e2424765a..000000000 --- a/rslib/src/scheduler/answering/undo.rs +++ /dev/null @@ -1,145 +0,0 @@ -// Copyright: Ankitects Pty Ltd and contributors -// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html - -#[cfg(test)] -mod test { - use crate::{ - card::{CardQueue, CardType}, - collection::open_test_collection, - deckconfig::LeechAction, - prelude::*, - scheduler::answering::{CardAnswer, Rating}, - }; - - #[test] - fn undo() -> Result<()> { - // add a note - let mut col = open_test_collection(); - let nt = col - .get_notetype_by_name("Basic (and reversed card)")? - .unwrap(); - let mut note = nt.new_note(); - note.set_field(0, "one")?; - note.set_field(1, "two")?; - col.add_note(&mut note, DeckId(1))?; - - // turn burying and leech suspension on - let mut conf = col.storage.get_deck_config(DeckConfigId(1))?.unwrap(); - conf.inner.bury_new = true; - conf.inner.leech_action = LeechAction::Suspend as i32; - col.storage.update_deck_conf(&conf)?; - - // get the first card - let queued = col.next_card()?.unwrap(); - let nid = note.id; - let cid = queued.card.id; - let sibling_cid = col.storage.all_card_ids_of_note_in_order(nid)?[1]; - - let assert_initial_state = |col: &mut Collection| -> Result<()> { - let first = col.storage.get_card(cid)?.unwrap(); - assert_eq!(first.queue, CardQueue::New); - let sibling = col.storage.get_card(sibling_cid)?.unwrap(); - assert_eq!(sibling.queue, CardQueue::New); - Ok(()) - }; - - assert_initial_state(&mut col)?; - - // immediately graduate the first card - col.answer_card(&CardAnswer { - card_id: queued.card.id, - current_state: queued.next_states.current, - new_state: queued.next_states.easy, - rating: Rating::Easy, - answered_at: TimestampMillis::now(), - milliseconds_taken: 0, - })?; - - // the sibling will be buried - let sibling = col.storage.get_card(sibling_cid)?.unwrap(); - assert_eq!(sibling.queue, CardQueue::SchedBuried); - - // make it due now, with 7 lapses. we use the storage layer directly, - // bypassing undo - let mut card = col.storage.get_card(cid)?.unwrap(); - assert_eq!(card.ctype, CardType::Review); - card.lapses = 7; - card.due = 0; - col.storage.update_card(&card)?; - - // fail it, which should cause it to be marked as a leech - col.clear_study_queues(); - let queued = col.next_card()?.unwrap(); - dbg!(&queued); - col.answer_card(&CardAnswer { - card_id: queued.card.id, - current_state: queued.next_states.current, - new_state: queued.next_states.again, - rating: Rating::Again, - answered_at: TimestampMillis::now(), - milliseconds_taken: 0, - })?; - - let assert_post_review_state = |col: &mut Collection| -> Result<()> { - let card = col.storage.get_card(cid)?.unwrap(); - assert_eq!(card.interval, 1); - assert_eq!(card.lapses, 8); - - assert_eq!( - col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), - 2 - ); - - let note = col.storage.get_note(nid)?.unwrap(); - assert_eq!(note.tags, vec!["leech".to_string()]); - assert_eq!(col.storage.all_tags()?.is_empty(), false); - - let deck = col.get_deck(DeckId(1))?.unwrap(); - assert_eq!(deck.common.review_studied, 1); - - dbg!(&col.next_card()?); - assert_eq!(col.next_card()?.is_some(), false); - - Ok(()) - }; - - let assert_pre_review_state = |col: &mut Collection| -> Result<()> { - // the card should have its old state, but a new mtime (which we can't - // easily test without waiting) - let card = col.storage.get_card(cid)?.unwrap(); - assert_eq!(card.interval, 4); - assert_eq!(card.lapses, 7); - - // the revlog entry should have been removed - assert_eq!( - col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), - 1 - ); - - // the note should no longer be tagged as a leech - let note = col.storage.get_note(nid)?.unwrap(); - assert_eq!(note.tags.is_empty(), true); - assert_eq!(col.storage.all_tags()?.is_empty(), true); - - let deck = col.get_deck(DeckId(1))?.unwrap(); - assert_eq!(deck.common.review_studied, 0); - - assert_eq!(col.next_card()?.is_some(), true); - - Ok(()) - }; - - // ensure everything is restored on undo/redo - assert_post_review_state(&mut col)?; - col.undo()?; - assert_pre_review_state(&mut col)?; - col.redo()?; - assert_post_review_state(&mut col)?; - col.undo()?; - assert_pre_review_state(&mut col)?; - col.undo()?; - assert_initial_state(&mut col)?; - - Ok(()) - } -} diff --git a/rslib/src/scheduler/queue/builder/mod.rs b/rslib/src/scheduler/queue/builder/mod.rs index fe58e46f8..47b189db8 100644 --- a/rslib/src/scheduler/queue/builder/mod.rs +++ b/rslib/src/scheduler/queue/builder/mod.rs @@ -6,10 +6,7 @@ pub(crate) mod intersperser; pub(crate) mod sized_chain; mod sorting; -use std::{ - cmp::Reverse, - collections::{BinaryHeap, HashMap, VecDeque}, -}; +use std::collections::{HashMap, VecDeque}; use intersperser::Intersperser; use sized_chain::SizedChain; @@ -114,19 +111,19 @@ impl QueueBuilder { pub(super) fn build( mut self, top_deck_limits: RemainingLimits, - learn_ahead_secs: u32, + learn_ahead_secs: i64, selected_deck: DeckId, current_day: u32, ) -> CardQueues { self.sort_new(); self.sort_reviews(current_day); - // split and sort learning - let learn_ahead_secs = learn_ahead_secs as i64; - let (due_learning, later_learning) = split_learning(self.learning, learn_ahead_secs); - let learn_count = due_learning.len(); + // intraday learning + let learning = sort_learning(self.learning); + let cutoff = TimestampSecs::now().adding_secs(learn_ahead_secs); + let learn_count = learning.iter().take_while(|e| e.due <= cutoff).count(); - // merge day learning in, and cap to parent review count + // merge interday learning into main, and cap to parent review count let main_iter = merge_day_learning( self.review, self.day_learning, @@ -148,8 +145,7 @@ impl QueueBuilder { }, undo: Vec::new(), main: main_iter.collect(), - due_learning, - later_learning, + learning, learn_ahead_secs, selected_deck, current_day, @@ -186,34 +182,9 @@ fn merge_new( } } -/// Split the learning queue into cards due within limit, and cards due later -/// today. Learning does not need to be sorted in advance, as the sorting is -/// done as the heaps/dequeues are built. -fn split_learning( - learning: Vec, - learn_ahead_secs: i64, -) -> ( - VecDeque, - BinaryHeap>, -) { - let cutoff = TimestampSecs(TimestampSecs::now().0 + learn_ahead_secs); - - // split learning into now and later - let (mut now, later): (Vec<_>, Vec<_>) = learning - .into_iter() - .map(LearningQueueEntry::from) - .partition(|c| c.due <= cutoff); - - // sort due items in ascending order, as we pop the deque from the front - now.sort_unstable_by(|a, b| a.due.cmp(&b.due)); - // partition() requires both outputs to be the same, so we need to create the deque - // separately - let now = VecDeque::from(now); - - // build the binary min heap - let later: BinaryHeap<_> = later.into_iter().map(Reverse).collect(); - - (now, later) +fn sort_learning(mut learning: Vec) -> VecDeque { + learning.sort_unstable_by(|a, b| a.due.cmp(&b.due)); + learning.into_iter().map(LearningQueueEntry::from).collect() } impl Collection { @@ -293,7 +264,7 @@ impl Collection { let queues = queues.build( selected_deck_limits, - self.learn_ahead_secs(), + self.learn_ahead_secs() as i64, deck_id, timing.days_elapsed, ); diff --git a/rslib/src/scheduler/queue/learning.rs b/rslib/src/scheduler/queue/learning.rs index 4ce4a713f..c74818eed 100644 --- a/rslib/src/scheduler/queue/learning.rs +++ b/rslib/src/scheduler/queue/learning.rs @@ -1,10 +1,7 @@ // Copyright: Ankitects Pty Ltd and contributors // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html -use std::{ - cmp::{Ordering, Reverse}, - collections::VecDeque, -}; +use std::{cmp::Ordering, collections::VecDeque}; use super::CardQueues; use crate::{prelude::*, scheduler::timing::SchedTimingToday}; @@ -34,24 +31,16 @@ impl CardQueues { /// Does not check for newly due cards, as that is already done by /// next_learning_entry_due_before_now() pub(super) fn next_learning_entry_learning_ahead(&self) -> Option { - self.due_learning.front().copied() + self.learning.front().copied() } pub(super) fn pop_learning_entry(&mut self, id: CardId) -> Option { - if let Some(top) = self.due_learning.front() { + if let Some(top) = self.learning.front() { if top.id == id { - self.counts.learning -= 1; - return self.due_learning.pop_front(); - } - } - - // fixme: remove this in the future - // the current python unit tests answer learning cards before they're due, - // so for now we also check the head of the later_due queue - if let Some(top) = self.later_learning.peek() { - if top.0.id == id { - // self.counts.learning -= 1; - return self.later_learning.pop().map(|c| c.0); + // under normal circumstances this should not go below 0, but currently + // the Python unit tests answer learning cards before they're due + self.counts.learning = self.counts.learning.saturating_sub(1); + return self.learning.pop_front(); } } @@ -85,31 +74,28 @@ impl CardQueues { mut entry: LearningQueueEntry, timing: SchedTimingToday, ) -> LearningQueueEntry { - let learn_ahead_limit = timing.now.adding_secs(self.learn_ahead_secs); - - if entry.due < learn_ahead_limit { - if self.learning_collapsed() { - if let Some(next) = self.due_learning.front() { - if next.due >= entry.due { - // the earliest due card is due later than this one; make this one - // due after that one - entry.due = next.due.adding_secs(1); + let cutoff = timing.now.adding_secs(self.learn_ahead_secs); + if entry.due <= cutoff && self.learning_collapsed() { + if let Some(next) = self.learning.front() { + // ensure the card is scheduled after the next due card + if next.due >= entry.due { + if next.due < cutoff { + entry.due = next.due.adding_secs(1) + } else { + // or outside the cutoff, in cases where the next due + // card is due later than the cutoff + entry.due = cutoff.adding_secs(60); } - self.push_due_learning_card(entry); - } else { - // nothing else waiting to review; make this due in a minute - entry.due = learn_ahead_limit.adding_secs(60); - self.later_learning.push(Reverse(entry)); } } else { - // not collapsed; can add normally - self.push_due_learning_card(entry); + // nothing else waiting to review; make this due a minute after + // cutoff + entry.due = cutoff.adding_secs(60); } - } else { - // due outside current learn ahead limit, but later today - self.later_learning.push(Reverse(entry)); } + self.push_learning_card(entry); + entry } @@ -117,41 +103,39 @@ impl CardQueues { self.main.is_empty() } - /// Adds card, maintaining correct sort order, and increments learning count. - pub(super) fn push_due_learning_card(&mut self, entry: LearningQueueEntry) { - self.counts.learning += 1; + /// Adds card, maintaining correct sort order, and increments learning count if + /// card is due within cutoff. + pub(super) fn push_learning_card(&mut self, entry: LearningQueueEntry) { let target_idx = - binary_search_by(&self.due_learning, |e| e.due.cmp(&entry.due)).unwrap_or_else(|e| e); - self.due_learning.insert(target_idx, entry); + binary_search_by(&self.learning, |e| e.due.cmp(&entry.due)).unwrap_or_else(|e| e); + if target_idx < self.counts.learning { + self.counts.learning += 1; + } + self.learning.insert(target_idx, entry); } fn check_for_newly_due_learning_cards(&mut self, cutoff: TimestampSecs) { - while let Some(earliest) = self.later_learning.peek() { - if earliest.0.due > cutoff { - break; - } - let entry = self.later_learning.pop().unwrap().0; - self.push_due_learning_card(entry); - } + self.counts.learning += self + .learning + .iter() + .skip(self.counts.learning) + .take_while(|e| e.due <= cutoff) + .count(); } pub(super) fn remove_requeued_learning_card_after_undo(&mut self, id: CardId) { - let due_idx = self - .due_learning - .iter() - .enumerate() - .find_map(|(idx, entry)| if entry.id == id { Some(idx) } else { None }); + let due_idx = self.learning.iter().enumerate().find_map(|(idx, entry)| { + if entry.id == id { + Some(idx) + } else { + None + } + }); if let Some(idx) = due_idx { - self.counts.learning -= 1; - self.due_learning.remove(idx); - } else { - // card may be in the later_learning binary heap - we can't remove - // it in place, so we have to rebuild it - self.later_learning = self - .later_learning - .drain() - .filter(|e| e.0.id != id) - .collect(); + // FIXME: if we remove the saturating_sub from pop_learning(), maybe + // this can go too? + self.counts.learning = self.counts.learning.saturating_sub(1); + self.learning.remove(idx); } } } diff --git a/rslib/src/scheduler/queue/mod.rs b/rslib/src/scheduler/queue/mod.rs index aa78ec8fa..3e5b0a7bc 100644 --- a/rslib/src/scheduler/queue/mod.rs +++ b/rslib/src/scheduler/queue/mod.rs @@ -8,10 +8,7 @@ mod limits; mod main; pub(crate) mod undo; -use std::{ - cmp::Reverse, - collections::{BinaryHeap, VecDeque}, -}; +use std::collections::VecDeque; pub(crate) use builder::{DueCard, NewCard}; pub(crate) use entry::{QueueEntry, QueueEntryKind}; @@ -28,8 +25,7 @@ pub(crate) struct CardQueues { /// Any undone items take precedence. undo: Vec, main: VecDeque, - due_learning: VecDeque, - later_learning: BinaryHeap>, + learning: VecDeque, selected_deck: DeckId, current_day: u32, learn_ahead_secs: i64, @@ -208,11 +204,24 @@ impl Collection { })) } } +} - #[cfg(test)] +// test helpers +#[cfg(test)] +impl Collection { pub(crate) fn next_card(&mut self) -> Result> { Ok(self .next_cards(1, false)? .map(|mut resp| resp.cards.pop().unwrap())) } + + pub(crate) fn get_queue_single(&mut self) -> Result { + self.next_cards(1, false)?.ok_or(AnkiError::NotFound) + } + + pub(crate) fn counts(&mut self) -> [usize; 3] { + self.get_queue_single() + .map(|q| [q.new_count, q.learning_count, q.review_count]) + .unwrap_or([0; 3]) + } } diff --git a/rslib/src/scheduler/queue/undo.rs b/rslib/src/scheduler/queue/undo.rs index 5fe5bccf2..f1bebdffc 100644 --- a/rslib/src/scheduler/queue/undo.rs +++ b/rslib/src/scheduler/queue/undo.rs @@ -77,3 +77,216 @@ impl CardQueues { self.undo.push(entry); } } + +#[cfg(test)] +mod test { + use crate::{ + card::{CardQueue, CardType}, + collection::open_test_collection, + deckconfig::LeechAction, + prelude::*, + }; + + fn add_note(col: &mut Collection, with_reverse: bool) -> Result { + let nt = col + .get_notetype_by_name("Basic (and reversed card)")? + .unwrap(); + let mut note = nt.new_note(); + note.set_field(0, "one")?; + if with_reverse { + note.set_field(1, "two")?; + } + col.add_note(&mut note, DeckId(1))?; + Ok(note.id) + } + + #[test] + fn undo() -> Result<()> { + // add a note + let mut col = open_test_collection(); + let nid = add_note(&mut col, true)?; + + // turn burying and leech suspension on + let mut conf = col.storage.get_deck_config(DeckConfigId(1))?.unwrap(); + conf.inner.bury_new = true; + conf.inner.leech_action = LeechAction::Suspend as i32; + col.storage.update_deck_conf(&conf)?; + + // get the first card + let queued = col.next_card()?.unwrap(); + let cid = queued.card.id; + let sibling_cid = col.storage.all_card_ids_of_note_in_order(nid)?[1]; + + let assert_initial_state = |col: &mut Collection| -> Result<()> { + let first = col.storage.get_card(cid)?.unwrap(); + assert_eq!(first.queue, CardQueue::New); + let sibling = col.storage.get_card(sibling_cid)?.unwrap(); + assert_eq!(sibling.queue, CardQueue::New); + Ok(()) + }; + + assert_initial_state(&mut col)?; + + // immediately graduate the first card + col.answer_easy(); + + // the sibling will be buried + let sibling = col.storage.get_card(sibling_cid)?.unwrap(); + assert_eq!(sibling.queue, CardQueue::SchedBuried); + + // make it due now, with 7 lapses. we use the storage layer directly, + // bypassing undo + let mut card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.ctype, CardType::Review); + card.lapses = 7; + card.due = 0; + col.storage.update_card(&card)?; + + // fail it, which should cause it to be marked as a leech + col.clear_study_queues(); + col.answer_again(); + + let assert_post_review_state = |col: &mut Collection| -> Result<()> { + let card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.interval, 1); + assert_eq!(card.lapses, 8); + + assert_eq!( + col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), + 2 + ); + + let note = col.storage.get_note(nid)?.unwrap(); + assert_eq!(note.tags, vec!["leech".to_string()]); + assert_eq!(col.storage.all_tags()?.is_empty(), false); + + let deck = col.get_deck(DeckId(1))?.unwrap(); + assert_eq!(deck.common.review_studied, 1); + + assert_eq!(col.next_card()?.is_some(), false); + + Ok(()) + }; + + let assert_pre_review_state = |col: &mut Collection| -> Result<()> { + // the card should have its old state, but a new mtime (which we can't + // easily test without waiting) + let card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.interval, 4); + assert_eq!(card.lapses, 7); + + // the revlog entry should have been removed + assert_eq!( + col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), + 1 + ); + + // the note should no longer be tagged as a leech + let note = col.storage.get_note(nid)?.unwrap(); + assert_eq!(note.tags.is_empty(), true); + assert_eq!(col.storage.all_tags()?.is_empty(), true); + + let deck = col.get_deck(DeckId(1))?.unwrap(); + assert_eq!(deck.common.review_studied, 0); + + assert_eq!(col.next_card()?.is_some(), true); + + let q = col.get_queue_single()?; + assert_eq!(&[q.new_count, q.learning_count, q.review_count], &[0, 0, 1]); + + Ok(()) + }; + + // ensure everything is restored on undo/redo + assert_post_review_state(&mut col)?; + col.undo()?; + assert_pre_review_state(&mut col)?; + col.redo()?; + assert_post_review_state(&mut col)?; + col.undo()?; + assert_pre_review_state(&mut col)?; + col.undo()?; + assert_initial_state(&mut col)?; + + Ok(()) + } + + #[test] + #[ignore = "undo code is currently broken"] + fn undo_counts1() -> Result<()> { + let mut col = open_test_collection(); + + assert_eq!(col.counts(), [0, 0, 0]); + add_note(&mut col, true)?; + assert_eq!(col.counts(), [2, 0, 0]); + col.answer_good(); + assert_eq!(col.counts(), [1, 1, 0]); + col.answer_good(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_good(); + assert_eq!(col.counts(), [0, 1, 0]); + col.answer_good(); + assert_eq!(col.counts(), [0, 0, 0]); + + // now work backwards + col.undo()?; + assert_eq!(col.counts(), [0, 1, 0]); + col.undo()?; + assert_eq!(col.counts(), [0, 2, 0]); + col.undo()?; + assert_eq!(col.counts(), [1, 1, 0]); + col.undo()?; + assert_eq!(col.counts(), [2, 0, 0]); + col.undo()?; + assert_eq!(col.counts(), [0, 0, 0]); + + Ok(()) + } + + #[test] + fn undo_counts2() -> Result<()> { + let mut col = open_test_collection(); + + assert_eq!(col.counts(), [0, 0, 0]); + add_note(&mut col, true)?; + assert_eq!(col.counts(), [2, 0, 0]); + col.answer_again(); + assert_eq!(col.counts(), [1, 1, 0]); + col.answer_again(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_again(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_again(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_good(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_easy(); + assert_eq!(col.counts(), [0, 1, 0]); + col.answer_good(); + // last card, due in a minute + assert_eq!(col.counts(), [0, 0, 0]); + + Ok(()) + } + + #[test] + fn undo_counts_relearn() -> Result<()> { + let mut col = open_test_collection(); + + add_note(&mut col, true)?; + col.storage + .db + .execute_batch("update cards set due=0,queue=2,type=2")?; + assert_eq!(col.counts(), [0, 0, 2]); + col.answer_again(); + assert_eq!(col.counts(), [0, 1, 1]); + col.answer_again(); + assert_eq!(col.counts(), [0, 2, 0]); + col.answer_easy(); + assert_eq!(col.counts(), [0, 1, 0]); + col.answer_easy(); + assert_eq!(col.counts(), [0, 0, 0]); + + Ok(()) + } +}