Skip to content

refactor: move custom DB methods out of libAnki#18676

Merged
BrayanDSO merged 6 commits intoankidroid:mainfrom
david-allison:libanki-db
Jul 4, 2025
Merged

refactor: move custom DB methods out of libAnki#18676
BrayanDSO merged 6 commits intoankidroid:mainfrom
david-allison:libanki-db

Conversation

@david-allison
Copy link
Member

@david-allison david-allison commented Jun 26, 2025

For us to remove the libAnki <-> AnkiDroid circular dependency

Would appreciate someone (@lukstbit / @Arthur-Milchior ?) going over the new class/package names, I'm not too happy, but don't know how you want to move forwards

I also feel these should be combined in a follow-on commit, but wanted a thought on placement/naming first

  • com.ichi2.anki.backend.AnkiDroidDB
  • com.ichi2.anki.backend.BackendDBUtils

Checklist

  • 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

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

I tihnk the commit message stating that you want to remove circular dependency is too vague. What you really do is reducing the dependency of anki to libanki, if I get it right.

You may want to add # before the issue number so that it's clickable

* this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.ichi2.anki.backend
Copy link
Member

Choose a reason for hiding this comment

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

I think what may be really useful is to have a small README.md in the folder, to explain what we can expect to find in this folder, what is the logic used to decide what to put in it.

The code is right since it's, as far as I understand, only moved code. But if we try to improve the architecture, it may be nice to simultaneously explain in the code tree the architecture choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you want in the readme?

My ping was for your comments regarding the package/naming.

I needed to extract this functionality, but it didn't fit well within the current package structure of the app.

If I were to write the readme, it would explain that the classes here are primarily adapters, extensions and utilities for libanki and the Rust-based backend (rsdroid). Mainly:

  • To extend python-based signatures to make them more idiomatic kotlin
  • Combining business logic with libanki
  • To better document libanki/rsdroid
  • Combining Android dependencies with libanki/rsdroid

But: I don't know if you like the concept of this package, and whether we should split it further.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really know what I'd want, because I didn't really understand what was there. And yes, the explanation you give seems fine.

I guess, if I get it right, something such as:

This folder contains part of the back-end that is Kotlin-specific. It adds various helper, allowing to access the rust-backend and libanki (a port of Anki's python as literal as possible) in a way that is Kotlin-like.

Copy link
Member Author

Choose a reason for hiding this comment

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

com.ichi2.anki.backend

This package makes the Anki backend (com.ichi2.libanki) more easily consumable by an Android app.
It does so by adding helper methods, extension methods, and functionality which depends on Android.

libanki is expected to match anki/pylib, with minimal Android dependencies and no Anki-Android
dependencies. As com.ichi2.anki.backend is in Anki-Android, these constraints do not apply.

@Arthur-Milchior
Copy link
Member

I'm having trouble to really get an opinion, because I've no idea even what anki is supposed to be. There are so many things in it. I know libanki is a translation of upstream back-end folder, that one is easy to understand.
But libanki seems very back-end ish and it seems surprising to me to have a back-end in anki.

If there is a document/issue that state the ultimate goal according to you that would help.

In the meantime, as a way to separate libanki, I'm fine with this PR

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Jun 27, 2025
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

LGTM, the issues below are not blocking more like notes.

Related to naming, I'm not able to come up with something better so they are fine from my point of view.

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jun 28, 2025
@david-allison
Copy link
Member Author

david-allison commented Jun 28, 2025

Thanks! This PR was a mix of 'later' and 'newer' commits from the libAnki split PR (and I think it's the last one before we start moving files)

The reviews here have been really useful to have a 'high level' view of where things move to, rather than the mechanical 'make things work'

* BackendDBUtils.withAndroidFramework

Removes a cyclic dependency: libAnki <-> AnkiDroid

Issue 18015
In order for us to remove the Android dependency

Issue 18015
For us to remove the libAnki <-> AnkiDroid circular dependency

Issue 18015
For us to remove the libAnki <-> AnkiDroid circular dependency

Issue 18015
@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs reviewer reply Waiting for a reply from another reviewer labels Jul 2, 2025
`BackendDBUtils.withAndroidFramework` =>
`createDatabaseUsingAndroidFramework`

`AnkiDroidDB.withRustBackend` =>
`createDatabaseUsingRustBackend`
@david-allison david-allison added Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Jul 2, 2025
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Nothing blocking. LGTM

@BrayanDSO BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge Needs reviewer reply Waiting for a reply from another reviewer labels Jul 4, 2025
@BrayanDSO BrayanDSO added this pull request to the merge queue Jul 4, 2025
Merged via the queue into ankidroid:main with commit ea1017f Jul 4, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Jul 4, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jul 4, 2025
@david-allison david-allison deleted the libanki-db branch July 14, 2025 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants