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

Add support for the V3 scheduler; drop support for V1 #11808

Merged
merged 10 commits into from
Jul 27, 2022

Conversation

dae
Copy link
Contributor

@dae dae commented Jul 9, 2022

Includes #11740 and #11802

Will compile and pass legacy schema tests without a backend update, so this can be merged before updating. It does require backend changes to pass the tests when legacy_schema is false.

  • Users still on V1 will be prompted to update when they tap on a deck. The update uses the backend code so that learning cards are not lost.
  • By default, AnkiDroid will continue to use V2 with a V3 collection. The V3 path is only enabled when legacy_schema=false
  • Tests pass, and in my brief tests in the emulator, reviewing/undoing seems to work.

I think this closes #7127, due to the backend code being much faster.
Closes #10694
Closes #10411

@mikehardy
Copy link
Member

🤯 😄

@mikehardy
Copy link
Member

Ready for rebase and hopefully I'll have time for a review tomorrow (nearly dinner time here in New Orleans 😁)

@dae
Copy link
Contributor Author

dae commented Jul 22, 2022

  • Have confirmed tests pass in each commit locally.
  • Have moved the commit from Use backend to calculate legacy timezone #11881 onto the tail of this one, so the final commit switches AD over to the latest backend. Can be kept in a separate PR if you'd prefer.

@dae
Copy link
Contributor Author

dae commented Jul 22, 2022

(made a trivial change to re-trigger the flaky emulator test)

Edit: new failure appears to have been introduced in the move to the latest backend. Investigating. I didn't catch this in local tests because I had an older API emulator active.

Edit 2: should be fixed now (though this code will presumably be dropped in favour of the backend importer anyway)

@mikehardy mikehardy mentioned this pull request Jul 22, 2022
5 tasks
@mikehardy mikehardy added Review High Priority Request for high priority review Needs Review labels Jul 22, 2022
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.

I only had enough time to look at the first commit 😩 - but had a couple things worth posting about it now

Comment on lines 133 to 134
// TODO: add translatable string
message: String = "Processing...",
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about both this string and relying on new backend's tr method below, we haven't (I haven't?) fully analyzed translation language sets to see how compatible they are etc, and I don't have the time to do that right now. I fear it'll hang up this PR unless we just use AnkiDroid-level translation strings for the moment or we (where "we" is someone other than me at the moment, and through mid-August) can harmonize the languages and update AnkiDroid docs to point at Anki's translation service as well for people that want to translate non-crowdin strings etc etc and kick off that whole process

Copy link
Contributor Author

@dae dae Jul 23, 2022

Choose a reason for hiding this comment

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

For the "processing..." string, I figured you'd want to do that in a follow-up commit after an i18n sync.

I've gone through and checked the languages mappings, and they should all be ok now. For any languages that the backend doesn't currently support, it will just fall back on the English strings like if it hadn't been translated yet. New languages can be added when users follow the instructions on https://translating.ankiweb.net/

Copy link
Member

Choose a reason for hiding this comment

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

For the "processing..." string, I figured you'd want to do that in a follow-up commit after an i18n sync.

Historically, user-visible strings are always added into strings file at time of first usage, and I just label the PR with the strings label so I remember to do the sync action / PR after - we never let user-visible stuff through with hard-coded strings in PRs, or at least not intentionally

I've gone through and checked the languages mappings, and they should all be ok now.

Thank you!

For any languages that the backend doesn't currently support, it will just fall back on the English strings like if it hadn't been translated yet. New languages can be added when users follow the instructions on https://translating.ankiweb.net/

I think this is a pretty solid start - and I am biased towards action, so let's go with it. I guess all that's left is an edit to the translation page to give people a handle to the new-to-us system you've got with Pontoon - I just did that https://github.com/ankidroid/Anki-Android/wiki/Translating-AnkiDroid/_compare/7c0ba5c393040654691f8ed44d2d44c3617d845e...002935cd2bc07effc17b8f3a409a846bde79e2be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String added

// Open collection
Context context = AnkiDroidApp.getInstance();
Copy link
Member

Choose a reason for hiding this comment

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

nice one - thanks - hopefully avoids the semi-false positives I was seeing with LeakCanary which were making me think things were crashing

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 all seems great to me - nothing seems to jump out and be obviously off. Just need the user-visible string extracted to something (03-dialogs maybe?) and I'm +1. V3 scheduler support, this is huge

@mikehardy
Copy link
Member

Oh, and it took a conflict but I am assuming (hopefully not naively) that it'll be trivial

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jul 24, 2022
@mikehardy
Copy link
Member

All other reviewers: I really want to maintain momentum in this connected chain of anki/Anki-Android-Backend/Anki-Android work, and it threatens to stall all the time because of my poor availability and the pushing it needs between the components. So I'd like to set a 1-2 day timer on this one before I merge by default. If you want to review and need more time just say so! No problem. Otherwise if it's quiet on this PR I'm going to merge it as soon as the string + de-conflict are in

Studying with the V1 scheduler enabled is no longer possible on recent
Anki, AnkiMobile or AnkiWeb versions, so AnkiDroid is the last one
still supporting it. Pushing users to upgrade will save them from some
of the footguns V1 had, and will allow AnkiDroid to cut out some code
and tests. Many users have likely already upgraded due to the use of
the other clients.

This commit adds support for the backend upgrade code, so that learning
cards will not be reset on upgrade. To make use of this, users will be
automatically updated to the latest schema version, the scheduler
upgrade will be performed, and then they're moved back to the legacy
schema.

I've also added a helper to more ergonomically deal with schema changes.
dae added 7 commits July 25, 2022 09:28
- Mirrors the code layout of the desktop version, where routines like
buryCard(), that are not specific to V2 or V3, are stored in a separate
file. This leaves AbstractSched containing only the methods that will
need to be implemented separately for V3.
- Moves some methods from SchedV2 into BaseSched, as V3 will want to
use them as well for now.
- Some of the routines in BaseSched will only work with a schema upgrade.
For now, SchedV2 overrides these so they continue to work with the
legacy schema.
Like the desktop, this works by using a single set of tests that
alter some of the checks depending on the active scheduler version,
so that code duplication is avoided.
I expect this fixes ankidroid#9368

Bumps the backend version, since the API changed.

Closes ankidroid#11881
- Move logic from backend into frontend, so a new backend release is
not required
- Fix mapping of Chinese languages and Hindi; check others
- Don't require a restart after changing language in prefs
After discarding the backend, Preferences was calling getCol and passing
itself in as the context. This caused the preferences screen to be retained
as the backend lives on. By switching to AnkiDroidApp, a leak can be
avoided.
Some were irrelevant; the ones in ContentProviderTest need investigating.
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.

timer expired + string extracted == let's go!

@mikehardy mikehardy merged commit d02093d into ankidroid:main Jul 27, 2022
@github-actions github-actions bot added this to the 2.16 release milestone Jul 27, 2022
@github-actions github-actions bot removed Review High Priority Request for high priority review Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Jul 27, 2022
@dae dae deleted the v3 branch July 28, 2022 04:20
@SnipeAT
Copy link

SnipeAT commented Aug 2, 2022

This is a feature I've been checking daily for. Will it will show up in the next alpha?

@dae
Copy link
Contributor Author

dae commented Aug 2, 2022

AFAIK it's already in the latest alpha, but it requires opting in to the new backend, which is a big change that has not received any user testing so far, so you may be exposing yourself to bugs in unrelated functionality. If you're happy to live life on the edge and make lots of backups, you can opt in for a session by tapping on the V16 option in the developer options in the preferences screen. Please report any issues you encounter.

@mikehardy
Copy link
Member

@dae unfortunately this is not 100% accurate:

the V16 option in the developer options in the preferences screen.

That "developer options" screen is dynamically added only to "debug" builds, that is when you build from source. So it's not generally accessible to users yet.

The next step in progression of V16+ schema testing is to make that specific option a toggle with state stored/used in/from preferences, and to move it to advanced menu. I'd love to see that soon as folks like @SnipeAT are (assuming they understand the risk) a really really really valuable source of testing since they are motivated to try it. Whatever we can do to get to this state where end users can get a warning but then try V16 would be a big step forward IMHO

@BrayanDSO
Copy link
Member

BrayanDSO commented Aug 6, 2022

@mikehardy, actually with the new About screen, users on RELEASE build can use developer options as well.

Tap the logo 6 times and a big warning will show asking if the user wants to enable it or not.
(6 is the number of letters of my name, now you won't forget it 😉)

And FWIW, I'm in favor of out adding the V16 option to the alpha/beta releases on Advanced, so anyone can find them.

@SnipeAT
Copy link

SnipeAT commented Aug 16, 2022

Just wanted to report that I've been using the new backend and the v3 scheduler for a week or so and it's been working out pretty great. The app might crash in a couple situations but reports seem to be being sent back home. Just updated to the alpha released today and will continue using (after making another backup of my collection).

@dae
Copy link
Contributor Author

dae commented Aug 16, 2022

Thanks for letting us know! Please note that there's currently a bug where review times are being saved incorrectly. Until the next alpha, I recommend you set your maximum answer time in the deck options to your average answer time, and/or adjust the previous review records using something like this in the debug console of the desktop version (5000 is 5 seconds), which will reset 60s answers in the last month:

mw.col.modSchema(check=True)
mw.col.db.execute("update revlog set time = 5000 where time = 60000 and id > 1658096427000")

https://docs.ankiweb.net/misc.html#debug-console

#12024

Also the bug reports are kind of hard to search, so if you are able to reproduce a bug, it would be helpful if you could describe the steps you're taking on a ticket here, and optionally include the acra ID if it's not easy to reproduce.

@BrayanDSO
Copy link
Member

complementing: You get the Acra ID together with the debug info on Settings > About > Copy Debug Info

@mikehardy
Copy link
Member

@dae follow-up on this one: I attempted to document it here for early testers, did I get all the information correct? #10411 (comment)

Secondly, seems we may need an "enable v3 scheduler" switch? Or do we have one already and I missed it? That's my main doubt but I could be wrong on any of it of course

@BrayanDSO
Copy link
Member

We don't have a v3 Scheduler switch. I have a draft branch for it here https://github.com/BrayanDSO/Anki-Android/tree/v3pref, but it has a bug that the preference is always re-enabled, both here and Anki desktop, after sync

@mikehardy
Copy link
Member

No worries - it will be needed in the future yes - but right now as a way to feed new testers into new backend / v3, having it be a requirement that the collection is v3-enabled on desktop perhaps feeds them in at a measured pace. Not a bad thing - I'm not looking for ease of use, just the possibility of use, and real testing. I think our current state fits that

@dae
Copy link
Contributor Author

dae commented Aug 24, 2022

LGTM. There's a bug where the scheduler version is not updated after a sync (will fix), so recommend you tell people to restart the app after they have synced the setting from the desktop.

@mikehardy
Copy link
Member

@dae thanks for the review, I updated the comment with a link the PR you just posted for the same. I see a fair bit of traffic today on V3/new-backend related items but it doesn't seem overwhelming? Do you get the same feeling? Like, there are issues but it is at capacity and not over-capacity? I ask because I'm gradually widening my audience where I publicize this and right now it feels like "no more new testers for a few days" but maybe by end of week I can start mentioning it's available again. Perhaps in a week or so we can make a real call for testers but that's not a decision now

@dae
Copy link
Contributor Author

dae commented Aug 25, 2022

#12157 should address the crashes I'm aware of. No particular preference on timing on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants