From 92673c99d8f7ba397862401afb618f656b81b6b8 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 27 Dec 2019 18:14:19 +1000 Subject: [PATCH] rework the new scheduler's rollover/day counting code The previous implementation interpreted the creation date as a local time, and applied the rollover to that. If the initial creation date was around midnight local time, even a one hour change due to daylight savings could result in Anki skipping or doubling up on a day. To address this, the rollover is now applied to the current time instead of the creation date. The new code needs the current time passed into it. This makes it easier to unit test, and for AnkiWeb to be able to use the user's local timezone. The new timezone code is currently disabled, as this code needs to be ported to all clients before it can be activated. --- anki/backend.py | 13 ++++ anki/collection.py | 22 +++--- anki/schedv2.py | 41 +++++++++-- proto/backend.proto | 31 ++++++-- rs/Cargo.lock | 33 +++++++++ rs/ankirs/Cargo.toml | 1 + rs/ankirs/src/backend.rs | 17 +++++ rs/ankirs/src/lib.rs | 1 + rs/ankirs/src/sched.rs | 149 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 286 insertions(+), 22 deletions(-) create mode 100644 rs/ankirs/src/sched.rs diff --git a/anki/backend.py b/anki/backend.py index 1ce8d66d4..6aa3ff664 100644 --- a/anki/backend.py +++ b/anki/backend.py @@ -8,6 +8,8 @@ import anki.backend_pb2 as pb from .types import AllTemplateReqs +SchedTimingToday = pb.SchedTimingTodayOut + class BackendException(Exception): def __str__(self) -> str: @@ -70,3 +72,14 @@ class Backend: output = self._run_command(input).template_requirements reqs: List[pb.TemplateRequirement] = output.requirements # type: ignore return proto_template_reqs_to_legacy(reqs) + + def sched_timing_today( + self, start: int, end: int, offset: int, rollover: int + ) -> SchedTimingToday: + return self._run_command( + pb.BackendInput( + sched_timing_today=pb.SchedTimingTodayIn( + created=start, now=end, minutes_west=offset, rollover_hour=rollover, + ) + ) + ).sched_timing_today diff --git a/anki/collection.py b/anki/collection.py index 775b1457b..1c6376af7 100644 --- a/anki/collection.py +++ b/anki/collection.py @@ -66,13 +66,6 @@ defaultConf = { } -def timezoneOffset() -> int: - if time.localtime().tm_isdst: - return time.altzone // 60 - else: - return time.timezone // 60 - - # this is initialized by storage.Collection class _Collection: db: Optional[DB] @@ -110,8 +103,6 @@ class _Collection: d = datetime.datetime(d.year, d.month, d.day) d += datetime.timedelta(hours=4) self.crt = int(time.mktime(d.timetuple())) - if not server: - self.conf["localOffset"] = timezoneOffset() self._loadScheduler() if not self.conf.get("newBury", False): self.conf["newBury"] = True @@ -139,6 +130,8 @@ class _Collection: self.sched = V1Scheduler(self) elif ver == 2: self.sched = V2Scheduler(self) + if not self.server: + self.conf["localOffset"] = self.sched.timezoneOffset() def changeSchedulerVer(self, ver: int) -> None: if ver == self.schedVer(): @@ -161,6 +154,13 @@ class _Collection: self._loadScheduler() + def localOffset(self) -> Optional[int]: + "Minutes west of UTC. Only applies to V2 scheduler." + if isinstance(self.sched, V1Scheduler): + return None + else: + return self.sched.timezoneOffset() + # DB-related ########################################################################## @@ -194,7 +194,7 @@ DB operations and the deck/tag/model managers do this automatically, so this is only necessary if you modify properties of this object or the conf dict.""" self.db.mod = True - def flush(self, mod: None = None) -> None: + def flush(self, mod: Optional[int] = None) -> None: "Flush state to DB, updating mod time." self.mod = intTime(1000) if mod is None else mod self.db.execute( @@ -209,7 +209,7 @@ crt=?, mod=?, scm=?, dty=?, usn=?, ls=?, conf=?""", json.dumps(self.conf), ) - def save(self, name: Optional[str] = None, mod: None = None) -> None: + def save(self, name: Optional[str] = None, mod: Optional[int] = None) -> None: "Flush, commit DB, and take out another write lock." # let the managers conditionally flush self.models.flush() diff --git a/anki/schedv2.py b/anki/schedv2.py index b80c1b4c5..e608db973 100644 --- a/anki/schedv2.py +++ b/anki/schedv2.py @@ -12,6 +12,8 @@ from operator import itemgetter # from anki.collection import _Collection from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union +import anki # pylint: disable=unused-import +from anki.backend import SchedTimingToday from anki.cards import Card from anki.consts import * from anki.hooks import runHook @@ -30,8 +32,9 @@ class Scheduler: name = "std2" haveCustomStudy = True _burySiblingsOnAnswer = True + revCount: int - def __init__(self, col) -> None: + def __init__(self, col: "anki.storage._Collection") -> None: self.col = col self.queueLimit = 50 self.reportLimit = 1000 @@ -1325,12 +1328,18 @@ where id = ? def _updateCutoff(self) -> None: oldToday = self.today - # days since col created - self.today = self._daysSinceCreation() - # end of day cutoff - self.dayCutoff = self._dayCutoff() + timing = self._timingToday() + + if self._newTimezoneEnabled(): + self.today = timing.days_elapsed + self.dayCutoff = timing.next_day_at + else: + self.today = self._daysSinceCreation() + self.dayCutoff = self._dayCutoff() + if oldToday != self.today: self.col.log(self.today, self.dayCutoff) + # update all daily counts, but don't save decks to prevent needless # conflicts. we'll save on card answer instead def update(g): @@ -1367,10 +1376,30 @@ where id = ? def _daysSinceCreation(self) -> int: startDate = datetime.datetime.fromtimestamp(self.col.crt) startDate = startDate.replace( - hour=self.col.conf.get("rollover", 4), minute=0, second=0, microsecond=0 + hour=self._rolloverHour(), minute=0, second=0, microsecond=0 ) return int((time.time() - time.mktime(startDate.timetuple())) // 86400) + def _rolloverHour(self) -> int: + return self.col.conf.get("rollover", 4) + + def _newTimezoneEnabled(self) -> bool: + return self.col.conf.get("newTimezone", False) + + def _timingToday(self) -> SchedTimingToday: + return self.col.backend.sched_timing_today( + self.col.crt, intTime(), self.timezoneOffset(), self._rolloverHour(), + ) + + def timezoneOffset(self) -> int: + if self.col.server: + return self.col.conf.get("localOffset", 0) + else: + if time.localtime().tm_isdst: + return time.altzone // 60 + else: + return time.timezone // 60 + # Deck finished state ########################################################################## diff --git a/proto/backend.proto b/proto/backend.proto index f8a0a61a3..ff8391b9e 100644 --- a/proto/backend.proto +++ b/proto/backend.proto @@ -4,18 +4,27 @@ package backend_proto; message Empty {} +// 1-15 reserved for future use; 2047 for errors + message BackendInput { + reserved 2047; oneof value { - PlusOneIn plus_one = 2; - TemplateRequirementsIn template_requirements = 3; + TemplateRequirementsIn template_requirements = 16; + SchedTimingTodayIn sched_timing_today = 17; + + PlusOneIn plus_one = 2046; // temporary, for testing + } } message BackendOutput { oneof value { - BackendError error = 1; - PlusOneOut plus_one = 2; - TemplateRequirementsOut template_requirements = 3; + TemplateRequirementsOut template_requirements = 16; + SchedTimingTodayOut sched_timing_today = 17; + + PlusOneOut plus_one = 2046; // temporary, for testing + + BackendError error = 2047; } } @@ -66,3 +75,15 @@ message TemplateRequirementAll { message TemplateRequirementAny { repeated uint32 ords = 1; } + +message SchedTimingTodayIn { + int64 created = 1; + int64 now = 2; + sint32 minutes_west = 3; + sint32 rollover_hour = 4; +} + +message SchedTimingTodayOut { + uint32 days_elapsed = 1; + int64 next_day_at = 2; +} diff --git a/rs/Cargo.lock b/rs/Cargo.lock index f1f3c14db..c9b7be63f 100644 --- a/rs/Cargo.lock +++ b/rs/Cargo.lock @@ -14,6 +14,7 @@ name = "ankirs" version = "0.1.0" dependencies = [ "bytes", + "chrono", "failure", "nom", "prost", @@ -94,6 +95,17 @@ version = "0.1.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" +[[package]] +name = "chrono" +version = "0.4.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "31850b4a4d6bae316f7a09e691c944c28299298837edc0a03f755618c23cbc01" +dependencies = [ + "num-integer", + "num-traits", + "time", +] + [[package]] name = "ctor" version = "0.1.12" @@ -301,6 +313,16 @@ dependencies = [ "version_check 0.1.5", ] +[[package]] +name = "num-integer" +version = "0.1.41" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b85e541ef8255f6cf42bbfe4ef361305c6c135d10919ecc26126c4e5ae94bc09" +dependencies = [ + "autocfg", + "num-traits", +] + [[package]] name = "num-traits" version = "0.2.10" @@ -708,6 +730,17 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "time" +version = "0.1.42" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db8dcfca086c1143c9270ac42a2bbd8a7ee477b78ac8e45b19abfb0cbede4b6f" +dependencies = [ + "libc", + "redox_syscall", + "winapi", +] + [[package]] name = "unicode-segmentation" version = "1.6.0" diff --git a/rs/ankirs/Cargo.toml b/rs/ankirs/Cargo.toml index eb11872f3..7e19fa2f0 100644 --- a/rs/ankirs/Cargo.toml +++ b/rs/ankirs/Cargo.toml @@ -9,6 +9,7 @@ nom = "5.0.1" failure = "0.1.6" prost = "0.5.0" bytes = "0.4" +chrono = "0.4.10" [build-dependencies] prost-build = "0.5.0" diff --git a/rs/ankirs/src/backend.rs b/rs/ankirs/src/backend.rs index 3a730126b..86fe70e36 100644 --- a/rs/ankirs/src/backend.rs +++ b/rs/ankirs/src/backend.rs @@ -1,6 +1,7 @@ use crate::backend_proto as pt; use crate::backend_proto::backend_input::Value; use crate::err::{AnkiError, Result}; +use crate::sched::sched_timing_today; use crate::template::{FieldMap, FieldRequirements, ParsedTemplate}; use prost::Message; use std::collections::HashSet; @@ -85,6 +86,9 @@ impl Backend { OValue::TemplateRequirements(self.template_requirements(input)?) } Value::PlusOne(input) => OValue::PlusOne(self.plus_one(input)?), + Value::SchedTimingToday(input) => { + OValue::SchedTimingToday(self.sched_timing_today(input)) + } }) } @@ -132,6 +136,19 @@ impl Backend { requirements: all_reqs, }) } + + fn sched_timing_today(&self, input: pt::SchedTimingTodayIn) -> pt::SchedTimingTodayOut { + let today = sched_timing_today( + input.created as i64, + input.now as i64, + input.minutes_west, + input.rollover_hour as i8, + ); + pt::SchedTimingTodayOut { + days_elapsed: today.days_elapsed, + next_day_at: today.next_day_at, + } + } } fn ords_hash_to_set(ords: HashSet) -> Vec { diff --git a/rs/ankirs/src/lib.rs b/rs/ankirs/src/lib.rs index c42efa231..965c9cd60 100644 --- a/rs/ankirs/src/lib.rs +++ b/rs/ankirs/src/lib.rs @@ -2,4 +2,5 @@ mod backend_proto; pub mod backend; pub mod err; +pub mod sched; pub mod template; diff --git a/rs/ankirs/src/sched.rs b/rs/ankirs/src/sched.rs new file mode 100644 index 000000000..7ce3bf636 --- /dev/null +++ b/rs/ankirs/src/sched.rs @@ -0,0 +1,149 @@ +use chrono::{DateTime, FixedOffset, Local, TimeZone, Timelike}; + +pub struct SchedTimingToday { + /// The number of days that have passed since the collection was created. + pub days_elapsed: u32, + /// Timestamp of the next day rollover. + pub next_day_at: i64, +} + +/// Timing information for the current day. +/// - created is the collection creation time +/// - now is the current UTC timestamp +/// - minutes_west is relative to the local timezone (eg UTC+10 hours is -600) +/// - rollover_hour is the hour of the day the rollover happens (eg 4 for 4am) +pub fn sched_timing_today( + created: i64, + now: i64, + minutes_west: i32, + rollover_hour: i8, +) -> SchedTimingToday { + let rollover_today = rollover_for_today(now, minutes_west, rollover_hour).timestamp(); + + SchedTimingToday { + days_elapsed: days_elapsed(created, now, rollover_today), + next_day_at: rollover_today + 86_400, + } +} + +/// Convert timestamp to the local timezone, with the provided rollover hour. +fn rollover_for_today( + timestamp: i64, + minutes_west: i32, + rollover_hour: i8, +) -> DateTime { + let local_offset = fixed_offset_from_minutes(minutes_west); + let rollover_hour = normalized_rollover_hour(rollover_hour); + local_offset + .timestamp(timestamp, 0) + .with_hour(rollover_hour as u32) + .unwrap() +} + +/// The number of times the day rolled over between two timestamps. +fn days_elapsed(start: i64, end: i64, rollover_today: i64) -> u32 { + // get the number of full days that have elapsed + let secs = (end - start).max(0); + let days = (secs / 86_400) as u32; + + // minus one if today's cutoff hasn't passed + if days > 0 && end < rollover_today { + days - 1 + } else { + days + } +} + +/// Negative rollover hours are relative to the next day, eg -1 = 23. +/// Cap hour to 23. +fn normalized_rollover_hour(hour: i8) -> u8 { + let capped_hour = hour.max(-23).min(23); + if capped_hour < 0 { + (24 + capped_hour) as u8 + } else { + capped_hour as u8 + } +} + +/// Build a FixedOffset struct, capping minutes_west if out of bounds. +fn fixed_offset_from_minutes(minutes_west: i32) -> FixedOffset { + let bounded_minutes = minutes_west.max(-23 * 60).min(23 * 60); + FixedOffset::west(bounded_minutes * 60) +} + +/// Relative to the local timezone, the number of minutes UTC differs by. +/// eg, Australia at +10 hours is -600. +/// Includes the daylight savings offset if applicable. +#[allow(dead_code)] +fn utc_minus_local_mins() -> i32 { + Local::now().offset().utc_minus_local() / 60 +} + +#[cfg(test)] +mod test { + use crate::sched::{ + fixed_offset_from_minutes, normalized_rollover_hour, sched_timing_today, + utc_minus_local_mins, + }; + use chrono::{FixedOffset, TimeZone}; + + #[test] + fn test_rollover() { + assert_eq!(normalized_rollover_hour(4), 4); + assert_eq!(normalized_rollover_hour(23), 23); + assert_eq!(normalized_rollover_hour(24), 23); + assert_eq!(normalized_rollover_hour(-1), 23); + assert_eq!(normalized_rollover_hour(-2), 22); + assert_eq!(normalized_rollover_hour(-23), 1); + assert_eq!(normalized_rollover_hour(-24), 1); + } + + #[test] + fn test_fixed_offset() { + let offset = fixed_offset_from_minutes(-600); + assert_eq!(offset.utc_minus_local(), -600 * 60); + } + + // helper + fn elap(start: i64, end: i64, west: i32, rollhour: i8) -> u32 { + let today = sched_timing_today(start, end, west, rollhour); + today.days_elapsed + } + + #[test] + fn test_days_elapsed() { + let offset = utc_minus_local_mins(); + + let created_dt = FixedOffset::west(offset * 60) + .ymd(2019, 12, 1) + .and_hms(2, 0, 0); + let crt = created_dt.timestamp(); + + // days can't be negative + assert_eq!(elap(crt, crt, offset, 4), 0); + assert_eq!(elap(crt, crt - 86_400, offset, 4), 0); + + // 2am the next day is still the same day + assert_eq!(elap(crt, crt + 24 * 3600, offset, 4), 0); + + // day rolls over at 4am + assert_eq!(elap(crt, crt + 26 * 3600, offset, 4), 1); + + // the longest extra delay is +23, or 19 hours past the 4 hour default + assert_eq!(elap(crt, crt + (26 + 18) * 3600, offset, 23), 0); + assert_eq!(elap(crt, crt + (26 + 19) * 3600, offset, 23), 1); + + // a collection created @ midnight in MDT in the past + let mdt = FixedOffset::west(6 * 60 * 60); + let mst = FixedOffset::west(7 * 60 * 60); + let crt = mdt.ymd(2018, 8, 6).and_hms(0, 0, 0).timestamp(); + // with the current time being MST + let now = mst.ymd(2019, 12, 26).and_hms(20, 0, 0).timestamp(); + let offset = mst.utc_minus_local() / 60; + assert_eq!(elap(crt, now, offset, 4), 507); + // the previous implementation generated a diferent elapsed number of days with a change + // to DST, but the number shouldn't change + let offset = mdt.utc_minus_local() / 60; + assert_eq!(elap(crt, now, offset, 4), 507); + } +}