Skip to content

Commit

Permalink
Simplify backend handling; rework collection instantiation
Browse files Browse the repository at this point in the history
When the Rust code was initially introduced, it was not clear whether
it would be usable on all devices, or whether it would need to be removed
for some reason. This no doubt influenced the design of the existing API,
which tries to make it easy to swap the Rust code out with something else.

Unfortunately this approach has some downsides:

- It makes it somewhat harder to follow, as method calls jump through
multiple interfaces before they're actually sent to the backend.
- It makes utilizing new methods considerably more cumbersome.

For example, take the extract_av_tags() call. It follows the following
path:

collection method or method in related helper class:
https://github.com/ankidroid/Anki-Android/blob/cea79e1b077bc30e7eed8f37529002aae416d34d/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt#L242

to generic interface:
https://github.com/ankidroid/Anki-Android/blob/cea79e1b077bc30e7eed8f37529002aae416d34d/AnkiDroid/src/main/java/com/ichi2/libanki/backend/DroidBackend.kt#L83

to specific implementation:
https://github.com/ankidroid/Anki-Android/blob/cea79e1b077bc30e7eed8f37529002aae416d34d/AnkiDroid/src/main/java/com/ichi2/libanki/backend/RustDroidV16Backend.kt#L57

and if it's unusable with the legacy schema (which I don't believe is
actually true in this case), it also needs to be added to the other
implementation:
https://github.com/ankidroid/Anki-Android/blob/cea79e1b077bc30e7eed8f37529002aae416d34d/AnkiDroid/src/main/java/com/ichi2/libanki/backend/RustDroidBackend.kt#L87

and then finally, a method in the backend module is invoked.

The backend module has code generation so that invoking a backend method
is as simple as making a method call, but currently you have to weave
the call through 3 or so levels of indirection before you can actually
use it. With something like 170 available methods, that's a fair amount
of extra work required.

Rather than trying to insulate libanki from the backend code, this PR
drops some of the indirection in favour of the approach the desktop takes:
libanki is the insulation layer; it can call freely into the backend methods,
but consumers (eg the GUI code) are expected to only call methods on the
collection, and not access the backend directly.

In addition to the above, collection initialization has been reworked
to be more similar to the computer version. Instead of the collection
being created from a database object, a backend is passed into the
collection creation, and the collection takes care of creating a DB
instance that wraps the backend.
  • Loading branch information
dae committed Jun 18, 2022
1 parent 2ddbe7d commit b02515b
Show file tree
Hide file tree
Showing 28 changed files with 228 additions and 546 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import com.ichi2.anki.tests.InstrumentedTest;
import com.ichi2.libanki.DB;

import net.ankiweb.rsdroid.database.AnkiSupportSQLiteDatabase;

import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -34,7 +36,9 @@
import java.io.FileOutputStream;
import java.util.Random;

import androidx.annotation.NonNull;
import androidx.sqlite.db.SupportSQLiteDatabase;
import androidx.sqlite.db.SupportSQLiteOpenHelper;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import androidx.test.rule.GrantPermissionRule;

Expand All @@ -55,8 +59,10 @@ public void testDBCorruption() throws Exception {
SQLiteDatabase.deleteDatabase(illFatedDBFile);
Assert.assertFalse("database exists already", illFatedDBFile.exists());

TestDB illFatedDB = new TestDB(illFatedDBFile.getCanonicalPath());
Assert.assertFalse("database should not be corrupt yet", illFatedDB.mDatabaseIsCorrupt);
TestCallback callback = new TestCallback(1);
DB illFatedDB = new DB(AnkiSupportSQLiteDatabase.withFramework(getTestContext(), illFatedDBFile.getCanonicalPath(), callback));

Assert.assertFalse("database should not be corrupt yet", callback.mDatabaseIsCorrupt);

// Scribble in it
byte[] b = new byte[1024];
Expand All @@ -75,7 +81,7 @@ public void testDBCorruption() throws Exception {
// do nothing, it is expected
}

Assert.assertTrue("database corruption not detected", illFatedDB.mDatabaseIsCorrupt);
Assert.assertTrue("database corruption not detected", callback.mDatabaseIsCorrupt);

// our handler avoids deleting databases, in contrast with default handler
Assert.assertTrue("database incorrectly deleted on corruption", illFatedDBFile.exists());
Expand All @@ -87,29 +93,17 @@ public void testDBCorruption() throws Exception {


// Test fixture that lets us inspect corruption handler status
public static class TestDB extends DB {

public class TestCallback extends AnkiSupportSQLiteDatabase.DefaultDbCallback {
private boolean mDatabaseIsCorrupt = false;

private TestDB(String ankiFilename) {
super(ankiFilename);
public TestCallback(int version) {
super(version);
}

@Override
protected SupportSQLiteOpenHelperCallback getDBCallback() {
return new TestSupportSQLiteOpenHelperCallback(1);
}

public class TestSupportSQLiteOpenHelperCallback extends SupportSQLiteOpenHelperCallback {
private TestSupportSQLiteOpenHelperCallback(int version) {
super(version);
}

@Override
public void onCorruption(SupportSQLiteDatabase db) {
mDatabaseIsCorrupt = true;
super.onCorruption(db);
}
public void onCorruption(SupportSQLiteDatabase db) {
mDatabaseIsCorrupt = true;
super.onCorruption(db);
}
}
}
19 changes: 0 additions & 19 deletions AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@
import com.ichi2.anki.analytics.UsageAnalytics;
import com.ichi2.utils.Permissions;

import net.ankiweb.rsdroid.BackendFactory;
import net.ankiweb.rsdroid.BackendV11Factory;
import net.ankiweb.rsdroid.BackendVNextFactory;
import net.ankiweb.rsdroid.RustCleanup;

import java.io.InputStream;
import java.util.Locale;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -496,20 +491,6 @@ protected void log(int priority, String tag, @NonNull String message, Throwable
}
}


/**
* Creates a backend instance using the currently configured backend implementation.
* @return
*/
@RustCleanup("Can remove after migrating to VNext")
public static BackendFactory currentBackendFactory() {
if (AnkiDroidApp.TESTING_USE_V16_BACKEND) {
return new BackendVNextFactory();
} else {
return new BackendV11Factory();
}
}

public static int activeSchemaVersion() {
if (TESTING_USE_V16_BACKEND) {
return Consts.BACKEND_SCHEMA_VERSION;
Expand Down
7 changes: 3 additions & 4 deletions AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.ichi2.anki.exception.StorageAccessException;
import com.ichi2.libanki.Collection;
import com.ichi2.libanki.DB;
import com.ichi2.libanki.Storage;
import com.ichi2.libanki.exception.UnknownDatabaseVersionException;
import com.ichi2.libanki.utils.SystemTime;
Expand Down Expand Up @@ -240,8 +241,7 @@ public synchronized void closeCollection(boolean save, String reason) {
* @return Whether or not {@link Collection} and its child database are open.
*/
public boolean colIsOpen() {
return mCollection != null && !mCollection.isDbClosed() &&
mCollection.getDb().getDatabase() != null && mCollection.getDb().getDatabase().isOpen();
return mCollection != null && !mCollection.isDbClosed();
}

/**
Expand Down Expand Up @@ -425,7 +425,6 @@ public static String getAppSpecificInternalAnkiDroidDirectory(@NonNull Context c
return new File(getCurrentAnkiDroidDirectory(context), COLLECTION_FILENAME).getAbsolutePath();
}


/**
* @return the absolute path to the AnkiDroid directory.
*/
Expand Down Expand Up @@ -564,7 +563,7 @@ public static void loadCollectionComplete(Collection col) {

public static int getDatabaseVersion(Context context) throws UnknownDatabaseVersionException {
// backend can't open a schema version outside range, so fall back to a pure DB implementation
return Storage.getDatabaseVersion(getCollectionPath(context));
return Storage.getDatabaseVersion(context, getCollectionPath(context));
}

public enum DatabaseVersion {
Expand Down
87 changes: 58 additions & 29 deletions AnkiDroid/src/main/java/com/ichi2/libanki/Collection.kt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import com.ichi2.async.CollectionTask.PartialSearch
import com.ichi2.async.ProgressSender
import com.ichi2.async.TaskManager
import com.ichi2.libanki.TemplateManager.TemplateRenderContext.TemplateRenderOutput
import com.ichi2.libanki.backend.DroidBackend
import com.ichi2.libanki.backend.exception.BackendNotSupportedException
import com.ichi2.libanki.exception.NoSuchDeckException
import com.ichi2.libanki.exception.UnknownDatabaseVersionException
Expand All @@ -53,11 +52,11 @@ import com.ichi2.libanki.utils.Time
import com.ichi2.libanki.utils.TimeManager
import com.ichi2.upgrade.Upgrade
import com.ichi2.utils.*
import net.ankiweb.rsdroid.Backend
import net.ankiweb.rsdroid.RustCleanup
import org.jetbrains.annotations.Contract
import timber.log.Timber
import java.io.*
import java.lang.NullPointerException
import java.util.*
import java.util.concurrent.LinkedBlockingDeque
import java.util.function.Consumer
Expand All @@ -73,35 +72,57 @@ import java.util.regex.Pattern
@KotlinCleanup("TextUtils -> Kotlin isNotEmpty()")
@KotlinCleanup("inline function in init { } so we don't need to init `crt` etc... at the definition")
@KotlinCleanup("ids.size != 0")
open class Collection @VisibleForTesting constructor(
open class Collection constructor(
/**
* @return The context that created this Collection.
*/
val context: Context,
db: DB,
val path: String,
var server: Boolean,
private var debugLog: Boolean, // Not in libAnki.
protected val droidBackend: DroidBackend
/**
* Do not access the backend outside of libanki.
*
* Ideally this would be declared as internal, but submodules such as com.ichi2.libanki.sched
* need to access it. If those submodules were merged into com.ichi2.libanki, then visibility
* could be changed.
*/
val backend: Backend
) : CollectionGetter {

@get:JvmName("isDbClosed")
var dbClosed = false
private set

/** Allows a mock db to be inserted for testing */
@set:VisibleForTesting
var db: DB = db
val dbClosed: Boolean
get() {
if (dbClosed) {
throw NullPointerException("DB Closed")
}
return field
return dbInternal == null
}
set(value) {
dbClosed = false
field = value

/** * Run the provided closure with the backend.
* Throws if backend.legacySchema == true
* * This should not be used outside libanki; see
* visibility comment for `backend` property.
*/
fun<T> legacyBackendThrows(func: (Backend) -> T): T {
if (backend.legacySchema) {
throw BackendNotSupportedException()
}
return func(backend)
}

@VisibleForTesting(otherwise = VisibleForTesting.NONE)
fun debugEnsureNoOpenPointers() {
val result = backend.getActiveSequenceNumbers()
if (result.isNotEmpty()) {
val numbers = result.toString()
throw IllegalStateException("Contained unclosed sequence numbers: $numbers")
}
}

// a lot of legacy code does not check for nullability
val db: DB
get() = dbInternal!!

var dbInternal: DB? = null

/**
* Getters/Setters ********************************************************** *************************************
*/
Expand Down Expand Up @@ -149,11 +170,11 @@ open class Collection @VisibleForTesting constructor(
private var mLogHnd: PrintWriter? = null

init {
_openLog()
media = Media(this, server)
val created = reopen()
log(path, VersionUtils.pkgVersionName)
// mLastSave = getTime().now(); // assigned but never accessed - only leaving in for upstream comparison
clearUndo()
media = Media(this, server)
tags = initTags()
load()
if (crt == 0L) {
Expand All @@ -165,6 +186,11 @@ open class Collection @VisibleForTesting constructor(
if (!get_config("newBury", false)!!) {
set_config("newBury", true)
}
if (created) {
Storage.addNoteTypes(col, backend)
col.onCreate()
col.save()
}
}

@KotlinCleanup("remove :DeckManager, remove ? on return value")
Expand Down Expand Up @@ -427,20 +453,26 @@ open class Collection @VisibleForTesting constructor(
if (!server) {
db.database.disableWriteAheadLogging()
}
droidBackend.closeCollection(db, downgrade)
dbClosed = true
backend.closeCollection(downgrade)
dbInternal = null
media.close()
_closeLog()
Timber.i("Collection closed")
}
}

fun reopen() {
Timber.i("Reopening Database")
/** True if DB was created */
fun reopen(): Boolean {
Timber.i("(Re)opening Database: %s", path)
if (dbClosed) {
db = droidBackend.openCollectionDatabase(path)
// fixme: pass in time
val (db_, created) = Storage.openDB(path, backend, TimeManager.time)
dbInternal = db_
media.connect()
_openLog()
return created
} else {
return false
}
}

Expand Down Expand Up @@ -1397,7 +1429,7 @@ open class Collection @VisibleForTesting constructor(
}

open fun onCreate() {
droidBackend.useNewTimezoneCode(this)
sched.useNewTimezoneCode()
set_config("schedVer", 2)
// we need to reload the scheduler: this was previously loaded as V1
_loadScheduler()
Expand Down Expand Up @@ -2496,9 +2528,6 @@ open class Collection @VisibleForTesting constructor(
return sched
}

open val backend: DroidBackend
get() = droidBackend

class CheckDatabaseResult(private val oldSize: Long) {
private val mProblems: MutableList<String?> = ArrayList()
var cardsWithFixedHomeDeckCount = 0
Expand Down
20 changes: 8 additions & 12 deletions AnkiDroid/src/main/java/com/ichi2/libanki/CollectionV16.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,36 +20,32 @@ import com.ichi2.async.CollectionTask
import com.ichi2.libanki.backend.*
import com.ichi2.libanki.backend.model.toProtoBuf
import com.ichi2.libanki.exception.InvalidSearchException
import net.ankiweb.rsdroid.Backend
import net.ankiweb.rsdroid.RustCleanup
import net.ankiweb.rsdroid.exceptions.BackendInvalidInputException

class CollectionV16(
context: Context,
db: DB,
path: String,
server: Boolean,
log: Boolean,
backend: RustDroidV16Backend
) : Collection(context, db, path, server, log, backend) {

/** Workaround as we shouldn't be overriding members which are used in the constructor */
override val backend: RustDroidV16Backend
get() = super.backend as RustDroidV16Backend
backend: Backend
) : Collection(context, path, server, log, backend) {

override fun initTags(): TagManager {
return TagsV16(this, RustTagsBackend(backend.backend))
return TagsV16(this, RustTagsBackend(backend))
}

override fun initDecks(deckConf: String?): DeckManager {
return DecksV16(this, RustDroidDeckBackend(backend.backend))
return DecksV16(this, RustDroidDeckBackend(backend))
}

override fun initModels(): ModelManager {
return ModelsV16(this, backend.backend)
return ModelsV16(this, backend)
}

override fun initConf(conf: String?): ConfigManager {
return ConfigV16(RustConfigBackend(backend.backend))
return ConfigV16(RustConfigBackend(backend))
}

/** col.conf is now unused, handled by [ConfigV16] which has a separate table */
Expand Down Expand Up @@ -92,7 +88,7 @@ class CollectionV16(
order
}
val cardIdsList = try {
backend.backend.searchCards(search, adjustedOrder.toProtoBuf())
backend.searchCards(search, adjustedOrder.toProtoBuf())
} catch (e: BackendInvalidInputException) {
throw InvalidSearchException(e)
}
Expand Down
Loading

0 comments on commit b02515b

Please sign in to comment.