Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sched: Enable New Timezone Handling Code #8054

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Jan 26, 2021

Purpose / Description

Anki Desktop has modified their code to better handle timezone changes. See linked issue

Fixes

Fixes #5805

Approach

Copied from Anki at commit ankitects/anki@131d37d
Except one line in Collection which accepts data if server == true.

https://github.com/ankitects/anki/blob/131d37dca52c29033432e0149052093ab1c79461/pylib/anki/collection.py
https://github.com/ankitects/anki/blob/131d37dca52c29033432e0149052093ab1c79461/pylib/anki/schedv2.py

  • Bumps SYNC_VERSION to 10 if using Rust
  • Add local_minutes_west and sched_timing_today to Backend
  • Add "New timezone handling" preference: sets creationOffset
  • Set "localOffset" preference if using SchedV2 under Rust
  • Fix SchedV2:_updateCutoff to better handle timezones

We will calculate this information in the Rust in V2 of the Rust Conversion

We can't do this yet as we're still on V11 of the database schema

How Has This Been Tested?

  • Compared with Anki Desktop - config variables were the same
  • Unit Tested
  • Syncing config did not work - investigated with help from @dae - likely cause: ankitects/anki@73679b0

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #8054 (c12a438) into master (7459828) will decrease coverage by 0.02%.
The diff coverage is 46.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8054      +/-   ##
============================================
- Coverage     36.94%   36.92%   -0.03%     
- Complexity     3751     3766      +15     
============================================
  Files           350      352       +2     
  Lines         35736    35803      +67     
  Branches       4730     4739       +9     
============================================
+ Hits          13204    13220      +16     
- Misses        21023    21070      +47     
- Partials       1509     1513       +4     
Impacted Files Coverage Δ Complexity Δ
...roid/src/main/java/com/ichi2/anki/Preferences.java 11.24% <0.00%> (-0.34%) 15.00 <0.00> (ø)
...va/com/ichi2/libanki/backend/JavaDroidBackend.java 37.50% <0.00%> (-12.50%) 3.00 <0.00> (ø)
...ackend/exception/BackendNotSupportedException.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...in/java/com/ichi2/libanki/sched/AbstractSched.java 50.00% <ø> (ø) 5.00 <0.00> (ø)
...id/src/main/java/com/ichi2/libanki/Collection.java 52.01% <40.00%> (+0.04%) 165.00 <0.00> (+1.00)
...2/libanki/backend/model/SchedTimingTodayProto.java 60.00% <60.00%> (ø) 1.00 <1.00> (?)
...src/main/java/com/ichi2/libanki/sched/SchedV2.java 81.45% <63.63%> (-0.79%) 405.00 <8.00> (+6.00) ⬇️
...iDroid/src/main/java/com/ichi2/libanki/Consts.java 75.00% <100.00%> (+8.33%) 1.00 <0.00> (ø)
...com/ichi2/libanki/backend/DroidBackendFactory.java 76.92% <100.00%> (+4.19%) 5.00 <0.00> (+1.00)
...va/com/ichi2/libanki/backend/RustDroidBackend.java 68.75% <100.00%> (+14.90%) 7.00 <2.00> (+3.00)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7459828...c12a438. Read the comment docs.

@david-allison david-allison marked this pull request as ready for review January 26, 2021 08:46
@mikehardy mikehardy added this to the 2.15 release milestone Jan 26, 2021
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool! Starting to get some features courtesy of rust :-)
I had one note that was code structure, I didn't see anything off in general, so this LGTM pending that
You ready to merge this, as in you feel like the initial conversion is solid enough to throw this in on top? I would not hesitate personally but you're the one watching issue flow I'm sure
As such, I think you merge + release on your preference to control same - either immediately if you like or whatever delay you prefer if so

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jan 28, 2021
@david-allison
Copy link
Member Author

Oh, also could use a quick skim over ankidroid/Anki-Android-Backend@edbac00

Copied from Anki at commit 131d37dca52c29033432e0149052093ab1c79461
Except one line in Collection which accepts data if server == true.

https://github.com/ankitects/anki/blob/131d37dca52c29033432e0149052093ab1c79461/pylib/anki/collection.py
https://github.com/ankitects/anki/blob/131d37dca52c29033432e0149052093ab1c79461/pylib/anki/schedv2.py

* Bumps SYNC_VERSION to 10 if using Rust
* Add local_minutes_west and sched_timing_today to Backend
* Add "New timezone handling" preference
* Set "localOffset" preference if using SchedV2 under Rust
* Fix SchedV2:_updateCutoff to better handle timezones

We will calculate this information in the Rust in V2 of the Rust Conversion

We can't do this yet as we're still on V11 of the database schema

Fixes 5805
@david-allison david-allison added Needs Review and removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Needs Author Reply Waiting for a reply from the original author labels Jan 29, 2021
@david-allison
Copy link
Member Author

Force pushed to remove conflicts - unsure what the "waiting for feedback" label was for.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Review labels Jan 29, 2021
@mikehardy
Copy link
Member

Ready at your leisure for merge + release

@david-allison david-allison merged commit a8654c7 into ankidroid:master Jan 29, 2021
@david-allison david-allison deleted the 5805-rust-sched branch January 29, 2021 17:11
@david-allison david-allison removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timezone handling changes in the new scheduler + Rust
2 participants