From 8830d338264017712a9c964121735574d3aa7f9b Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 19 Aug 2021 16:40:12 +1000 Subject: [PATCH] revert some interday learning changes in v3 Interday learning cards are now counted in the learning count again, and are no longer subject to the daily review limit. The thinking behind the original change was that interday learning cards are scheduled more like reviews, and counting them in the review count would allow the learning count to focus on intraday learning - the red number reflecting the fact that they are the most fragile memories. And counting them together made it practical to apply the review limit to both at once. Since the release, there have been a number of users expecting to see interday learning cards included in the learning count (the latest being https://forums.ankiweb.net/t/feedback-and-a-feature-adjustment-request-for-2-1-45/12308), and a good argument can be made for that too - they are, after all, listed in the learning steps, and do tend to be harder than reviews. Short of introducing another count to keep track of interday and intraday learning separately, moving back to the old behaviour seems like the best move. This also means it is not really practical to apply the review limit to interday learning cards anymore, as the limit would be split between two different numbers, and how much each number is capped would depend on the order cards are introduced. The scheduler could figure this out, but the deck list code does not know card order, and would need significant changes to be able to produce numbers that matched the scheduler. And even if we ignore implementation complexities, I think it would be more difficult for users to reason about - the influence of the review limit on new cards is confusing enough as it is. --- pylib/tests/test_schedv2.py | 6 +- rslib/src/decks/counts.rs | 2 - rslib/src/decks/tree.rs | 2 +- rslib/src/scheduler/answering/mod.rs | 2 +- .../src/scheduler/queue/builder/gathering.rs | 20 ++---- rslib/src/scheduler/queue/builder/mod.rs | 71 +++++++++++++------ rslib/src/scheduler/queue/entry.rs | 1 + rslib/src/scheduler/queue/learning.rs | 4 +- rslib/src/scheduler/queue/main.rs | 3 + rslib/src/scheduler/queue/mod.rs | 2 +- rslib/src/storage/card/due_cards.sql | 5 +- rslib/src/storage/card/mod.rs | 41 ++++++----- rslib/src/storage/deck/mod.rs | 14 ++-- 13 files changed, 95 insertions(+), 78 deletions(-) diff --git a/pylib/tests/test_schedv2.py b/pylib/tests/test_schedv2.py index b18d51690..e374d733a 100644 --- a/pylib/tests/test_schedv2.py +++ b/pylib/tests/test_schedv2.py @@ -317,11 +317,7 @@ def test_learn_day(): c.due -= 1 c.flush() col.reset() - if is_2021(): - # it appears in the review queue - assert col.sched.counts() == (0, 0, 1) - else: - assert col.sched.counts() == (0, 1, 0) + assert col.sched.counts() == (0, 1, 0) c = col.sched.getCard() # nextIvl should work assert ni(c, 3) == 86400 * 2 diff --git a/rslib/src/decks/counts.rs b/rslib/src/decks/counts.rs index 3a4af0c9b..9dd905222 100644 --- a/rslib/src/decks/counts.rs +++ b/rslib/src/decks/counts.rs @@ -30,14 +30,12 @@ impl Collection { days_elapsed: u32, learn_cutoff: u32, limit_to: Option<&str>, - v3: bool, ) -> Result> { self.storage.due_counts( self.scheduler_version(), days_elapsed, learn_cutoff, limit_to, - v3, ) } diff --git a/rslib/src/decks/tree.rs b/rslib/src/decks/tree.rs index c40b065d9..433a6d988 100644 --- a/rslib/src/decks/tree.rs +++ b/rslib/src/decks/tree.rs @@ -277,7 +277,7 @@ impl Collection { let learn_cutoff = (now.0 as u32) + self.learn_ahead_secs(); let sched_ver = self.scheduler_version(); let v3 = self.get_config_bool(BoolKey::Sched2021); - let counts = self.due_counts(days_elapsed, learn_cutoff, limit, v3)?; + let counts = self.due_counts(days_elapsed, learn_cutoff, limit)?; let dconf = self.storage.get_deck_config_map()?; add_counts(&mut tree, &counts); let limits = remaining_limits_map(decks_map.values(), &dconf, days_elapsed); diff --git a/rslib/src/scheduler/answering/mod.rs b/rslib/src/scheduler/answering/mod.rs index 310884805..8222169bc 100644 --- a/rslib/src/scheduler/answering/mod.rs +++ b/rslib/src/scheduler/answering/mod.rs @@ -315,7 +315,7 @@ impl Collection { let mut review_delta = 0; match from_queue { CardQueue::New => new_delta += 1, - CardQueue::Review | CardQueue::DayLearn => review_delta += 1, + CardQueue::Review => review_delta += 1, _ => {} } self.update_deck_stats( diff --git a/rslib/src/scheduler/queue/builder/gathering.rs b/rslib/src/scheduler/queue/builder/gathering.rs index 4180ef162..80f6453db 100644 --- a/rslib/src/scheduler/queue/builder/gathering.rs +++ b/rslib/src/scheduler/queue/builder/gathering.rs @@ -2,7 +2,7 @@ // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html use super::{BuryMode, DueCard, NewCard, QueueBuilder}; -use crate::{card::CardQueue, prelude::*}; +use crate::{prelude::*, scheduler::queue::DueCardKind}; impl QueueBuilder { pub(in super::super) fn add_intraday_learning_card( @@ -15,12 +15,7 @@ impl QueueBuilder { } /// True if limit should be decremented. - pub(in super::super) fn add_due_card( - &mut self, - queue: CardQueue, - card: DueCard, - bury_mode: BuryMode, - ) -> bool { + pub(in super::super) fn add_due_card(&mut self, card: DueCard, bury_mode: BuryMode) -> bool { let bury_this_card = self .get_and_update_bury_mode_for_note(card.note_id, bury_mode) .map(|mode| mode.bury_reviews) @@ -28,14 +23,9 @@ impl QueueBuilder { if bury_this_card { false } else { - match queue { - CardQueue::DayLearn => { - self.day_learning.push(card); - } - CardQueue::Review => { - self.review.push(card); - } - _ => unreachable!(), + match card.kind { + DueCardKind::Review => self.review.push(card), + DueCardKind::Learning => self.day_learning.push(card), } true diff --git a/rslib/src/scheduler/queue/builder/mod.rs b/rslib/src/scheduler/queue/builder/mod.rs index 6eedf3d84..0b1192ab0 100644 --- a/rslib/src/scheduler/queue/builder/mod.rs +++ b/rslib/src/scheduler/queue/builder/mod.rs @@ -19,7 +19,7 @@ use crate::{ }; /// Temporary holder for review cards that will be built into a queue. -#[derive(Debug, Default, Clone)] +#[derive(Debug, Clone)] pub(crate) struct DueCard { pub id: CardId, pub note_id: NoteId, @@ -29,6 +29,13 @@ pub(crate) struct DueCard { pub hash: u64, pub current_deck_id: DeckId, pub original_deck_id: DeckId, + pub kind: DueCardKind, +} + +#[derive(Debug, Clone, Copy)] +pub(crate) enum DueCardKind { + Review, + Learning, } /// Temporary holder for new cards that will be built into a queue. @@ -48,7 +55,10 @@ impl From for MainQueueEntry { MainQueueEntry { id: c.id, mtime: c.mtime, - kind: MainQueueEntryKind::Review, + kind: match c.kind { + DueCardKind::Review => MainQueueEntryKind::Review, + DueCardKind::Learning => MainQueueEntryKind::InterdayLearning, + }, } } } @@ -117,25 +127,33 @@ impl QueueBuilder { ) -> CardQueues { self.sort_new(); - // intraday learning - let learning = sort_learning(self.learning); + // intraday learning and total learn count + let intraday_learning = sort_learning(self.learning); let now = TimestampSecs::now(); let cutoff = now.adding_secs(learn_ahead_secs); - let learn_count = learning.iter().take_while(|e| e.due <= cutoff).count(); + let learn_count = intraday_learning + .iter() + .take_while(|e| e.due <= cutoff) + .count() + + self.day_learning.len(); - // merge interday learning into main, and cap to parent review count - let main_iter = merge_day_learning( + // cap and note down review + new counts + self.review.truncate(top_deck_limits.review as usize); + let review_count = self.review.len(); + self.new.truncate(top_deck_limits.new as usize); + let new_count = self.new.len(); + + // merge interday and new cards into main + let with_interday_learn = merge_day_learning( self.review, self.day_learning, self.sort_options.day_learn_mix, ); - let main_iter = main_iter.take(top_deck_limits.review as usize); - let review_count = main_iter.len(); - - // cap to parent new count, note down the new count, then merge new in - self.new.truncate(top_deck_limits.new as usize); - let new_count = self.new.len(); - let main_iter = merge_new(main_iter, self.new, self.sort_options.new_review_mix); + let main_iter = merge_new( + with_interday_learn, + self.new, + self.sort_options.new_review_mix, + ); CardQueues { counts: Counts { @@ -144,7 +162,7 @@ impl QueueBuilder { learning: learn_count, }, main: main_iter.collect(), - intraday_learning: learning, + intraday_learning, learn_ahead_secs, selected_deck, current_day, @@ -192,7 +210,7 @@ impl Collection { let now = TimestampSecs::now(); let timing = self.timing_for_timestamp(now)?; let decks = self.storage.deck_with_children(deck_id)?; - // need full map, since filtered decks may contain cards from decks/ + // need full map, since filtered decks may contain cards from decks // outside tree let deck_map = self.storage.get_decks_map()?; let config = self.storage.get_deck_config_map()?; @@ -245,18 +263,31 @@ impl Collection { queues.add_intraday_learning_card(card, bury) })?; - // reviews and interday learning next + // interday learning + self.storage.for_each_due_card_in_active_decks( + timing.days_elapsed, + sort_options.review_order, + DueCardKind::Learning, + |card| { + let bury = get_bury_mode(card.original_deck_id.or(card.current_deck_id)); + queues.add_due_card(card, bury); + true + }, + )?; + + // reviews if selected_deck_limits.review != 0 { - self.storage.for_each_review_card_in_active_decks( + self.storage.for_each_due_card_in_active_decks( timing.days_elapsed, sort_options.review_order, - |queue, card| { + DueCardKind::Review, + |card| { if selected_deck_limits.review == 0 { return false; } let bury = get_bury_mode(card.original_deck_id.or(card.current_deck_id)); let limits = remaining.get_mut(&card.current_deck_id).unwrap(); - if limits.review != 0 && queues.add_due_card(queue, card, bury) { + if limits.review != 0 && queues.add_due_card(card, bury) { selected_deck_limits.review -= 1; limits.review -= 1; } diff --git a/rslib/src/scheduler/queue/entry.rs b/rslib/src/scheduler/queue/entry.rs index ad1ba2134..201616625 100644 --- a/rslib/src/scheduler/queue/entry.rs +++ b/rslib/src/scheduler/queue/entry.rs @@ -31,6 +31,7 @@ impl QueueEntry { QueueEntry::Main(e) => match e.kind { MainQueueEntryKind::New => QueueEntryKind::New, MainQueueEntryKind::Review => QueueEntryKind::Review, + MainQueueEntryKind::InterdayLearning => QueueEntryKind::Learning, }, } } diff --git a/rslib/src/scheduler/queue/learning.rs b/rslib/src/scheduler/queue/learning.rs index 6ca2aa75c..3f3052fd1 100644 --- a/rslib/src/scheduler/queue/learning.rs +++ b/rslib/src/scheduler/queue/learning.rs @@ -181,8 +181,8 @@ impl CardQueues { } } -/// Adapted from the Rust stdlib VecDeque implementation; we can drop this when the following -/// lands: https://github.com/rust-lang/rust/issues/78021 +/// Adapted from the Rust stdlib VecDeque implementation; we can drop this after updating +/// to Rust 1.54.0 fn binary_search_by<'a, F, T>(deque: &'a VecDeque, mut f: F) -> Result where F: FnMut(&'a T) -> Ordering, diff --git a/rslib/src/scheduler/queue/main.rs b/rslib/src/scheduler/queue/main.rs index ab0f6b1c9..ae48a4c36 100644 --- a/rslib/src/scheduler/queue/main.rs +++ b/rslib/src/scheduler/queue/main.rs @@ -15,6 +15,7 @@ pub(crate) struct MainQueueEntry { pub(crate) enum MainQueueEntryKind { New, Review, + InterdayLearning, } impl CardQueues { @@ -24,6 +25,7 @@ impl CardQueues { match head.kind { MainQueueEntryKind::New => self.counts.new -= 1, MainQueueEntryKind::Review => self.counts.review -= 1, + MainQueueEntryKind::InterdayLearning => self.counts.learning -= 1, }; head }) @@ -34,6 +36,7 @@ impl CardQueues { match entry.kind { MainQueueEntryKind::New => self.counts.new += 1, MainQueueEntryKind::Review => self.counts.review += 1, + MainQueueEntryKind::InterdayLearning => self.counts.learning += 1, }; self.main.push_front(entry); } diff --git a/rslib/src/scheduler/queue/mod.rs b/rslib/src/scheduler/queue/mod.rs index e02ae4c39..bd39040ec 100644 --- a/rslib/src/scheduler/queue/mod.rs +++ b/rslib/src/scheduler/queue/mod.rs @@ -9,7 +9,7 @@ pub(crate) mod undo; use std::collections::VecDeque; -pub(crate) use builder::{DueCard, NewCard}; +pub(crate) use builder::{DueCard, DueCardKind, NewCard}; pub(crate) use entry::{QueueEntry, QueueEntryKind}; pub(crate) use learning::LearningQueueEntry; pub(crate) use main::{MainQueueEntry, MainQueueEntryKind}; diff --git a/rslib/src/storage/card/due_cards.sql b/rslib/src/storage/card/due_cards.sql index 1ac3a6fec..0c29b5a31 100644 --- a/rslib/src/storage/card/due_cards.sql +++ b/rslib/src/storage/card/due_cards.sql @@ -1,5 +1,4 @@ -SELECT queue, - id, +SELECT id, nid, due, cast(ivl AS integer), @@ -12,6 +11,6 @@ WHERE did IN ( FROM active_decks ) AND ( - queue IN (2, 3) + queue = ? AND due <= ? ) \ No newline at end of file diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs index 8e1c77070..38d407328 100644 --- a/rslib/src/storage/card/mod.rs +++ b/rslib/src/storage/card/mod.rs @@ -20,7 +20,7 @@ use crate::{ notes::NoteId, scheduler::{ congrats::CongratsInfo, - queue::{DueCard, NewCard}, + queue::{DueCard, DueCardKind, NewCard}, }, timestamp::{TimestampMillis, TimestampSecs}, types::Usn, @@ -184,20 +184,24 @@ impl super::SqliteStorage { original_deck_id: row.get(5)?, interval: 0, hash: 0, + kind: DueCardKind::Learning, }) } Ok(()) } - pub(crate) fn for_each_review_card_in_active_decks( + /// Call func() for each review card or interday learning card, stopping + /// when it returns false or no more cards found. + pub(crate) fn for_each_due_card_in_active_decks( &self, day_cutoff: u32, order: ReviewCardOrder, + kind: DueCardKind, mut func: F, ) -> Result<()> where - F: FnMut(CardQueue, DueCard) -> bool, + F: FnMut(DueCard) -> bool, { let order_clause = review_order_sql(order); let mut stmt = self.db.prepare_cached(&format!( @@ -205,22 +209,23 @@ impl super::SqliteStorage { include_str!("due_cards.sql"), order_clause ))?; - let mut rows = stmt.query(params![day_cutoff])?; + let queue = match kind { + DueCardKind::Review => CardQueue::Review, + DueCardKind::Learning => CardQueue::DayLearn, + }; + let mut rows = stmt.query(params![queue as i8, day_cutoff])?; while let Some(row) = rows.next()? { - let queue: CardQueue = row.get(0)?; - if !func( - queue, - DueCard { - id: row.get(1)?, - note_id: row.get(2)?, - due: row.get(3).ok().unwrap_or_default(), - interval: row.get(4)?, - mtime: row.get(5)?, - current_deck_id: row.get(6)?, - original_deck_id: row.get(7)?, - hash: 0, - }, - ) { + if !func(DueCard { + id: row.get(0)?, + note_id: row.get(1)?, + due: row.get(2).ok().unwrap_or_default(), + interval: row.get(3)?, + mtime: row.get(4)?, + current_deck_id: row.get(5)?, + original_deck_id: row.get(6)?, + hash: 0, + kind, + }) { break; } } diff --git a/rslib/src/storage/deck/mod.rs b/rslib/src/storage/deck/mod.rs index ceb0094e8..5ca7aa1a3 100644 --- a/rslib/src/storage/deck/mod.rs +++ b/rslib/src/storage/deck/mod.rs @@ -38,18 +38,13 @@ fn row_to_deck(row: &Row) -> Result { }) } -fn row_to_due_counts(row: &Row, v3: bool) -> Result<(DeckId, DueCounts)> { +fn row_to_due_counts(row: &Row) -> Result<(DeckId, DueCounts)> { let deck_id = row.get(0)?; let new = row.get(1)?; - let mut review = row.get(2)?; + let review = row.get(2)?; let interday: u32 = row.get(3)?; let intraday: u32 = row.get(4)?; - let learning = if v3 { - review += interday; - intraday - } else { - intraday + interday - }; + let learning = intraday + interday; Ok(( deck_id, DueCounts { @@ -265,7 +260,6 @@ impl SqliteStorage { day_cutoff: u32, learn_cutoff: u32, top_deck: Option<&str>, - v3: bool, ) -> Result> { let sched_ver = sched as u8; let mut params = named_params! { @@ -306,7 +300,7 @@ impl SqliteStorage { self.db .prepare_cached(sql)? - .query_and_then(&*params, |row| row_to_due_counts(row, v3))? + .query_and_then(&*params, |row| row_to_due_counts(row))? .collect() }