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 option to enable the v3 scheduler #12178

Merged
merged 2 commits into from
Aug 27, 2022
Merged

Conversation

BrayanDSO
Copy link
Member

Pull Request template

Purpose / Description

Help people test, and test and test and test and test all night long

Approach

  • extract part of the logic of _loadScheduler() into CollectionHelper.isV3Enabled
  • add a new experimental preference called "Use the v3 scheduler"
    • it's dependent on the new backend, i.e. the user can only enable it if the new backend is being used

How Has This Been Tested?

  • Sync the collection
  • Enable v3 on AnkiDroid -> sync -> see if it was enabled on another device
  • Disable v3 on AnkiDroid -> sync -> see if it was disabled on another device
  • Enable v3 on another device -> sync -> see if it was enabled on AnkiDroid
  • Disable v3 on another device -> sync -> see if it was disabled on AnkiDroid

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • 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)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

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 like this a lot, and I think it's near-perfect, just want a second look at the null handling as commented

// v3 scheduler
@RustCleanup("move this to Reviewing > Scheduling once the new backend is the default")
val v3schedPref = requirePreference<SwitchPreference>(R.string.enable_v3_sched_key).apply {
isChecked = CollectionHelper.isV3Enabled(col!!)
Copy link
Member

Choose a reason for hiding this comment

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

should this be in a withCol { } ? just curious, it's col!! here, but below col? (but then col?.newBackend!! - the null handling just has me a bit worried

Copy link
Member Author

@BrayanDSO BrayanDSO Aug 26, 2022

Choose a reason for hiding this comment

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

Very good question and I can't answer for sure. I haven't thought much about withCol, neither I grasp completely where we should use it, but I suppose it's on wherever possible.

I've tried to adress this on the last two commits and I'm gonna ask @dae if they are fine or if he could give me a direction on this

I'm definitely not sure if the runBlockingCatching use was correct, as it is used to configure the UI. Probably not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, all new code committed to the repo would use withCol, and ideally doing so in a launched task, so it doesn't block the UI like runBlockingCatching does. Integrating a launched task into some code paths may be tricky though, and it can require additional changes to avoid breaking unit tests if that path is tested, so it may be worth being pragmatic at the moment - allowing blocking calls if required, and not blocking a PR if there's already a bunch of surrounding code accessing the collection directly.

Copy link
Member Author

@BrayanDSO BrayanDSO Aug 26, 2022

Choose a reason for hiding this comment

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

Okay, good to know. Used lanchCatchingTask. Thanks!

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 much better to me! Thank you

@mikehardy mikehardy requested a review from dae August 26, 2022 11:16
@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge Strings and removed Needs Review labels Aug 26, 2022
@mikehardy mikehardy merged commit 9f050a6 into ankidroid:main Aug 27, 2022
@github-actions github-actions bot added this to the 2.16 release milestone Aug 27, 2022
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Aug 27, 2022
@BrayanDSO BrayanDSO deleted the v3pref branch August 27, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants