[GSoC] Add Use new backend (persistent) preference

[BrayanDSO]

Purpose / Description

• The V16 pref is useful to testing in case the dev forgot to set legacy_schema=false on local.properties while not having to re-enable every time the app is reopened.

Fixes

A tiny bit of #6772

Approach

On the commits

How Has This Been Tested?

Emulator SDK 25

untitled.webm

(not so sure why Android Studio recording is getting dark, maybe I should buy a faster computer)

Checklist

Please, go through these checks before submitting the PR.

• 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)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[dae]

Would it be worth moving these into the Advanced section so alpha testers can try too? #11808 (comment)

[dae]

Also, I don't really see the benefit of the separate csv pref. The csv importer will require v16 to be enabled, so it's going to be hidden by default even without a separate preference.

[BrayanDSO]

Would it be worth moving these into the Advanced section so alpha testers can try too? #11808 (comment)

I'm particularly quite in favor of releasing it as an Advanced preference. If I get a green light of the maintainers, I'll do it.

Also, I don't really see the benefit of the separate csv pref. The csv importer will require v16 to be enabled, so it's going to be hidden by default even without a separate preference.

I actually prefer enabling the CSV importer if the new backend is active instead of a preference as well. Good to hear that another person find this reasonable too. Better than taking the "safe" approach (the CSV importer screen is self contained, so shouldn't break the rest of the app). edit: done this

[BrayanDSO]

Noticed that I should refactor some of the file selection code, so I'll keep this PR only for the V16 option

edit: moved to the Advanced category anyway, that's what I interpreted #11808 (comment) wants, and sooner or later it will have do be done

[Arthur-Milchior]

you have a merge conflict

[lukstbit]

I reviewed the changes and I'm questioning if we really need the temporary backend change. The permanent backend change preference has several advantages:

• offers the user full control about the backend choice
• it has a status indicator which shows what backend implementation we use

The temporary backend change:

• it doesn't have a a status indicator to show what backend implementation we use. See below why this might be a problem
• it doesn't work as its description mentions as Android doesn't really have the concept of "app close"(apps just come and go as the user does he's thing). Example:
  I'm clicking the temporary backend preference -> I'm being taken to the DeckPicker -> I press BACK -> reopen the app x minutes later.

At this point did I close the app? I would say yes, and others might as well
Is the backend reverted ? No, it isn't.

At this point, although I left the app, the backend wasn't changed back. Can I see/realize this by looking at the preferences? No, as the temporary backend change doesn't offer any visual feedback.

So the text would need to be changed to mention that the user has to kill the app from the task manager(which indeed reverts the backend). At this point, if we require the user to go to the task manager and kill the app, we might as well just use the permanent backend preference.

Also, I really think that changing any of these preferences should bring up a confirmation dialog. When I was first checking the preferences, as I scrolled, I accidentally touched the permanent backend change preference and I enabled it. A simple misplaced touch shouldn't automatically enable such preferences.

[dae]

Fair points about visibility and confirmation. I think the reason David originally added the temporary toggle is out of concern that the user might turn it on, find it crashes the app on startup due to some code path/condition we didn't anticipate, and then will be unable to revert to the old setting. That's a legitimate concern while this code is so fresh, and I actually encountered such a situation today - if the collection has the current deck id stored incorrectly as a string (which happened with old AnkiDroid versions IIRC), the new schema path currently crashes AnkiDroid on startup.

[BrayanDSO]

Added confirmation and visual indication if temporary pref is enabled or not

2022-08-08.08-43-49.mp4

[mikehardy]

Sorry to drop in here semi-uninformed and kind of tardy, but I just got a network time slice and my thoughts were/are:

• change should be permanent. Yes, there is some risk, string description should contain serious risk warning, then go for it
• should be in advanced section, entire goal here (for me) is to say to V3 scheduler requesters (a motivated pool of testers!) "there is some risk here, but try the new alpha and do this..."

Also, it allows developers here to more easily test their GSoC stuff (because it's persistent) and for people doing QA on GSoC to test it more easily (because it's persistent...)

Hoping to review this (if someone doesn't beat me to it) shortly but just in case I get pulled away again for a week or whatever (like I did last week) I at least stated my vision for it. Done traveling on Aug 18 so I'm about to be able to really help push things forward again. Cheers

[BrayanDSO]

Okey dokey. No temporary preference it is. Make your backups and have fun testing, people! Cheers.

[david-allison]

I don't think this should be offered as an option.

The preference existed specifically for developers to test v16 only if the app was built under DEBUG. This was with the expectation that the code was unstable (despite unit tests passing)

The preference specifically reverts after an 'app close' to allow the app to be usable in the case of catastrophic failure.

Instead of implementing this, we should focus on getting the new backend stable, and then switching to it permanently.

If we do not feel that the new code is stable enough to offer to users, users should not be able to enable it.

[BrayanDSO]

FWIW, my opinion is:

1. The temporary preference should be removed or kept only at "Developer options"
   • It is useful only for developers. Non-devs shouldn't/wouldn't use it. And it's only useful for the specific case that something makes the app crash before the developer can go disable the setting. Even if that happens, the developer should just fix the problem or clear the app data.
2. We should have a persistent option (at least on "Developer options")
   • It's more useful than the temporary preference, as it isn't necessary to do the burden of re-enabling the preference on every app startup
   • It's still useful for motivated alpha users that aren't on DEBUG builds and want to test it can do it be enabling Dev options on the About screen secret
3. Advanced X Developer options
   • I lean more to putting it on "Advanced"
   • I don't want it, or any experimental preference, to go to the play store release versions once we are able to, but I still want more people to be able to test it. Many of the bugs that appear on main are only discovered by chance. The same will happen with the V16 changes, so we need more testers before releasing it to the general public.
   • So, I'm personally in favor of putting it, along with any other experimental preference, as a alpha-only Advanced option.
   • I don't know how AnkiDroid's beta program works, as the app releases were already blocked when I started to contribute, so I can't give an opinion if beta users should be able or not to use it.

I'm strong on the first two points. I've stated my opinion about the third point, but I'm happy with any path taken.

[mikehardy]

I still want this. There's a chicken-and-egg problem of getting enough testing to feel that it's stable enough to offer to "users", and "users" is not just two classes ("people with debug builds from source" and "people using alphas / betas / etc"). There is at the moment one specific group of people - our alpha users - that are installing manually a thing labeled "alpha" and if they want to test, they would specifically tap a thing that warns them about the danger. At that point it could eat the family pet and we have clean conscience I think.

In the meantime a subset of these people (v3 scheduler requesters) is motivated to try it, and we'll start getting some real usage of the backend. Realistically, I think this will net us probably 4-5 testers max, 2 alpha / non-developer v3 people, and 2-3 of "us" (people that regular test / PR things)

Sync works, review works, undo works, those are the basic use cases. And for problems, we've got Damien and myself ready for fixes.

My vision for duration of v3 stabilization is just a couple weeks and I'm happy to re-hide this if there are problems and/or if we're about to go to beta, if for some reason it takes a lot longer or it becomes a problem.

Where to put it? Definitely "advanced" but it can't be in "developer options" since that is for debug-builds only

Note that I have still not reviewed the text - when I do so I'll make sure it says something about "There is a chance this could corrupt your installation completely, requiring app uninstall and deletion of /sdcard/AnkiDroid, and has a chance of propagating corruption via sync to ankiweb. We welcome the testing, but please make sure you have a valid non-AnkiWeb backup." (or similar)

[BrayanDSO]

I'm happy with any text. When you do review it, if text changes are required, please suggest the one you'd like 🙏 for 1. the summary and 2. the dialog message

edit: current dialog message and summary:

[mikehardy]

With a warning like that, I believe any "reasonable person" (yes, that's vague, but it's a US legal standard at least, I'm being sincere) can agree that if something goes wrong, you were warned......

[mikehardy]

This works for me, I personally appreciate the change. I also respect the legitimate worries about this and will own (via user support, which I provide on mailing list, play store, and issue tracker...) issues if they pop up. I hope on balance it does help testing...I think it will 🤞

[mikehardy]

maintainer note: doing i18n sync prior to merging this, so the string change from this does not mix with unrelated translations (a real string review velocity killer...)