Get remaining unit tests passing when new backend code is enabled #11579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
From the SQLite docs: When synchronous is NORMAL (1), the SQLite database engine will still sync at the most critical moments, but less often than in FULL mode. There is a very small (though non-zero) chance that a power failure at just the wrong time could corrupt the database in journal_mode=DELETE on an older filesystem. WAL mode is safe from corruption with synchronous=NORMAL, and probably DELETE mode is safe too on modern filesystems.
mikehardy
approved these changes
Jun 10, 2022
There was a problem hiding this comment.
These all look fine to me, my only comment is a possibility of a RustCleanup annotation on first commit. I'd love for David to have a look but it all looks great, and that's not a blocker for me
Now I just fear what you'll see if you head over to Anki-Android-Backend, my first efforts in that repo have been just to update dependencies to more or less current but I'm only part way there and I fear it's a bit un-hygienic at the moment
Either way, rare treat to see a PR from you here, very much appreciated
mikehardy
added
the
Needs Second Approval
I wouldn't worry too much about it at the moment - if the old deps are pinned properly they should suffice until AnkiDroid has been updated to work with the 2.1.34 schema 16 code. Once that's done and there's no longer a need to support running with the v11 schema, updating to newer backend releases should be simpler (at least after the initial hurdle of moving from 2.1.34->latest; there were a number of infrastructure changes in the interim which have since settled down). |
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 11, 2022
mikehardy
added
the
Backend Related
|
Hey @dae - had a problem with this one Learnt today David will have really limited availability for a while I pulled this locally and ran it through our typical CI battery without the changes described above; all passed so it will work for non-V16 testers I made the changes mentioned above and got this, do you see this? Are you referencing a local build of Anki-Android-Backend for this, with something extra done (perhaps using a newer anki submodule ref?) |
mikehardy
added
Needs Author Reply
|
I note that Anki-Android-Backend has this:
While Anki-Android has this:
However, those aspects were not perturbed in the PR and non-V16 appears to work. The related method (according to github threads in related repos) indicate method was implemented in 3.17.1 so this is unexpected. |
|
The changes in this PR don't touch any protobuf-related code, so presumably you'd get the same failures if you switched to V16 on ankidroid main? I wrote these before looking into making changes to the backend, so I would have been using the standard release build from David in build.gradle. I've only done testing inside AndroidStudio on a sim; were you testing on a device? Some Googling of that error seems to point to it perhaps being connected to ProGuard? protocolbuffers/protobuf#8956 |
|
Yeah seems to work if you are V11 only, but once you touch a collection with V16, then it goes sideways every time. If you rm-rf /sdcard/AnkiDroid and go back to V11 it works. It seems like a multi-bug: 1- there's an unknown protobuf field, which is likely a problem 1 can likely be tackled separately by instrumenting the last stack frame under our control and dumping the protobuf fields and looking for unexpected things? 2 I dunno, the internet was slim pickings on information on this one. Elimination of proguard as a cause can be done by turning off minification everywhere and re-trying at least. Can also try bisecting back in time to see if this ever worked and if/when it broke - lots of dependencies have been moved around in both repos |
mikehardy
added
Needs Review
and removed
Needs Author Reply
|
Sounds like it may an existing bug on main? I'll try reproduce it when I have a chance. |
|
When I try to load a V16-upgraded collection with the stock main branch of AnkiDroid, it fails due to the previously-mentioned /tmp issue being triggered by the downgrade. If I apply the fix covered in #11600 to the top of getInstance() in DroidBackendFactory, the downgrade succeeds and I'm able to navigate in the app. I'll update that PR so it's no longer blocked on the Java change. |
david-allison
approved these changes
Jun 15, 2022
There was a problem hiding this comment.
Thanks so much for this & for the code contributions in general!
I'm both overcommitted in terms of obligations (until August), and a family member is terminally ill, so I'm not going to be regularly available.
I /really/ don't want to be a blocker for things here, please don't block things on me. Worst case is I'll catch up later
|
Really sorry to hear about the family member David. :-( |
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 16, 2022
Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
|
Closing in favor of #11644 |
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 21, 2022
Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 25, 2022
Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
|
I captured the comment about a need for a more specific error message in a separate issue and flagged it up for any interested person to work on. I think that harvests all the discussion here |
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 25, 2022
Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 28, 2022
Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 28, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
dae
added a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 28, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581
mikehardy
pushed a commit
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes #11579 and closes #11581
mikehardy
pushed a commit
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes #11579 and closes #11581 refactor: Rename Storage.java to .kt com.ichi2.libanki.Storage refactor: Convert Storage to Kotlin com.ichi2.libanki.Storage Remove the time argument to Storage.collection() Collection does not currently take a time argument, and relies on the global object instead, so this change brings Storage in line with it. Reuse the backend when closing+reopening a collection Avoids having to re-initialize the translations, and reduces leaks (the import + export code leaks backends still)
mikehardy
pushed a commit
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes #11579 and closes #11581
mikehardy
pushed a commit
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes #11579 and closes #11581 Remove the time argument to Storage.collection() Collection does not currently take a time argument, and relies on the global object instead, so this change brings Storage in line with it. Reuse the backend when closing+reopening a collection Avoids having to re-initialize the translations, and reduces leaks (the import + export code leaks backends still)
mikehardy
pushed a commit
to ankitects/Anki-Android
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581 Remove the time argument to Storage.collection() Collection does not currently take a time argument, and relies on the global object instead, so this change brings Storage in line with it. Reuse the backend when closing+reopening a collection Avoids having to re-initialize the translations, and reduces leaks (the import + export code leaks backends still)
mikehardy
pushed a commit
that referenced
this pull request
Jun 29, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes #11579 and closes #11581 Remove the time argument to Storage.collection() Collection does not currently take a time argument, and relies on the global object instead, so this change brings Storage in line with it. Reuse the backend when closing+reopening a collection Avoids having to re-initialize the translations, and reduces leaks (the import + export code leaks backends still)
dorrin-sot
pushed a commit
to dorrin-sot/Anki-Android
that referenced
this pull request
Jul 14, 2022
Squashes a few commits: Move the legacy schema toggle into BackendFactory Simplify backend handling; rework collection instantiation 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. Remove always-on isUsingRustBackend Drop the legacy upgrade/initialization code Schema 11 was introduced in 2012, and decks that still are <11 are very rare. The desktop dropped support for schema 10 back in early 2020. This also removes the need to modify SCHEMA_VERSION when switching between TESTING_USE_V16_BACKEND. Remove DOWNGRADE_REQUIRED and slightly simplify startup error handling - The backend automatically downgrades when required, and possible. No backup is required, as the downgrade happens in a single transaction, and the downgrade code has proven itself over time. - Store the type of failure in getColSafe(), so it can be checked later. Update to work with desktop 2.1.53 code Depends on ankidroid/Anki-Android-Backend#202 Due to the removal and change of a few backend methods, syncing, importing and the card templates screen will not work when the schema16 setting is active (actually schema18 now). To get them working again, those code paths will need to switch to the backend implementations. A few notes: - Downgrading happens automatically when loading the collection in schema11 mode, so the extra code dealing with downgrades & "can downgrade" reporting can be stripped. - Added the ability to run col.set_config("key", JSONObject.NULL), as unit tests were attempting to write the entire collection config, which is no longer supported. - All tests pass on both old and new backends, though the latter required disabling a few failed tests when running with the new schema (eg notetype updating). Integrates, and thus closes ankidroid#11579 and closes ankidroid#11581 Remove the time argument to Storage.collection() Collection does not currently take a time argument, and relies on the global object instead, so this change brings Storage in line with it. Reuse the backend when closing+reopening a collection Avoids having to re-initialize the translations, and reduces leaks (the import + export code leaks backends still)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@david-allison had done the vast majority of the work here already, and there were only 6 tests failing. With the exception of the exception mapping, these all turned out to be problems with the tests rather than the library code, and "fixing" here mainly involved disabling them after figuring out why they were failing.
The synchronous change was not required for the tests, and was just something I noticed while working through the code.
Tested by manually enabling the new code path: