convert Deck to strongly typed value class #17718

Arthur-Milchior · 2025-01-04T20:01:24Z

I loved David's recent work in using value class. And thought that we could finally kill the overhead cost of DeepClone in JSONObject. So I transformed Deck into a value class, and added the needed properties so that the other part of the code can use Deck without considering it as a JSONObject.

AnkiDroid/src/main/java/com/ichi2/libanki/Deck.kt

AnkiDroid/src/main/java/com/ichi2/libanki/Decks.kt

AnkiDroid/src/main/java/com/ichi2/libanki/backend/BackendUtils.kt

AnkiDroid/src/main/java/com/ichi2/ui/AppCompatPreferenceActivity.kt

AnkiDroid/src/test/java/com/ichi2/testutils/JsonUtils.kt

david-allison

One question on the 1 and 2 discrepancy

AnkiDroid/src/main/java/com/ichi2/libanki/Deck.kt

AnkiDroid/src/main/java/com/ichi2/anki/FilteredDeckOptions.kt

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/EditDeckDescriptionDialog.kt

Arthur-Milchior · 2025-01-16T23:12:06Z

Test added for each property

david-allison

The typings for a number of properties are incorrect, and I'm concerned.

previewGoodSecs is an Int. if you insert it into JSON as a String, it's quoted

david-allison · 2025-01-16T23:56:24Z

AnkiDroid/src/main/java/com/ichi2/anki/FilteredDeckOptions.kt


                        "limit" -> {
-                            ar.getJSONArray(0).put(1, value)
+                            deck.firstFilter.limit = value as String


Why is this a string?

So the reason is that I misunderstood the way this part of the code worked. I thought all values were strings unless converted to other values. I'll update all of that

AnkiDroid/src/main/java/com/ichi2/libanki/Deck.kt

Arthur-Milchior · 2025-06-15T11:19:35Z

Sorry but I fail to understand what you mean by "break compat".
Anki don't have any file similar to our Deck.kt, so I don't think you are telling me that this code is too different from upstream code. And using constant or literal is a implementation details which is invisible to everything outside this file, so it's not breaking anything else I can see

david-allison · 2025-06-25T10:15:56Z

@ankidroid/core Thoughts? I'm fine with light wrappers. I'm a medium-strength 'no' here, but want to check for consensus

libAnki should track pylib upstream in almost all cases. This makes it very simple for us to ingest upstream changes from pylib.

When we deviate, it should be for a good reason (ideally, the changes should be pushed upstream, then ingested downstream, or added as extension methods/helper classes outside libAnki). As a few examples:

When we were doing the Rust backend conversion, it was a source of pain each time the code deviated. It deviated a LOT
Splitting out libAnki into a separate module is harder work due to additions which add circular dependencies between AnkiDroid and libAnki
Whenever we take the time to bring libAnki up to date with pylib, these deviations from pylib mean that it's not a 1:1 conversion

Making the code nicer to read/better factored is probably slowing us down here in the long run. Maybe the documentation is worthwhile, but this change goes much deeper than adding a wrapper over existing functionality.

BrayanDSO · 2025-06-25T14:25:28Z

I prefer the alternative that makes me think less, which in this case is copying libanki as much as possible.

ideally, the changes should be pushed upstream, then ingested downstream, or added as extension methods/helper classes outside libAnki

Completely agreed with that.

github-actions · 2025-07-09T14:40:51Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

david-allison · 2025-07-09T14:44:14Z

I'm happy to take on splitting out the first few commits which are non-controversial

Would be useful to wait until #18015 is in. Most of the conflicts are done, but there will be a little more on the test side.

Split libanki & tests into a separate module #18015

github-actions · 2025-07-31T17:41:04Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

david-allison · 2025-08-07T17:54:16Z

Happy to close if I haven't split this off & merged within 2 months, overall change is worthwhile

The first interest of this change is that we are not deep cloning all decks when creating a Deck object which should save a little bit of time, especially loading time on collection with a lot of decks. Instead, we have a better typing at zero runtime cost. The second interest is that we are certain that the fact that the Deck is implemented as a JSONObject is hidden from the developer using this class. Instead, they can only use the provided extension, or add the one they needs. While most changes are relatively trivial, moving the implementation from everywhere in the code to Deck.kt, there are some extra changes that were needed. I moved `getLongOrNull` to PythonExtension given that it implements the behavior of a Python function we want to reuse, this seems more consistent. As value class can't be lateinit, I hardcoded AppCompatPreferenceActivity as being a getter for the backing field `_deck: Deck?`. I added in testutils/JsonUtils helper method that accepts a json holder, and a string representing the expected json, to simplify the use of those methods. The code used sometime `optLong("id")` and sometime `getLong("id")`. There seems to be no reason for the difference between both use case, and just be random choice from the developer. Anyway, if a deck lacks an id, any place using `getLong("id")` would have caused the code to crash, so replacing the `optLong` by `getLong` would, in the worst case, move the time at which the user is asked to clean their database. There seems to be a single key we sometime set to JSONObject.NULL, it was `delays`. For the sake of typing, the property `delays` in `deck` is thus of type `JSONArray?`. When the value `null` is assigned to this property, `NULL` is saved in the object, and reciprocally. There is no need to check whether `deck` has "empty" key before removing it. It's not even more efficient.

Deck is now declared as an interface so that it can be inherited. `Deck` contains a facotry to decide whether to return a normal deck or a filtered one.

This test ensures that every changes possibly done in the deck options are correctly applied

github-actions · 2025-09-01T10:40:49Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

david-allison · 2025-09-09T03:47:06Z

I'll fixup

blocked on: improvement(libanki): sync Decks .ktwith upstream #19200

Arthur-Milchior force-pushed the value_class_deck branch from b3b8b54 to 3dec54c Compare January 4, 2025 20:05

david-allison reviewed Jan 4, 2025

View reviewed changes

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jan 6, 2025

github-actions bot added the Has Conflicts label Jan 6, 2025

Arthur-Milchior force-pushed the value_class_deck branch from 3dec54c to 0f7f2df Compare January 8, 2025 22:21

Arthur-Milchior added Needs Review and removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Jan 8, 2025

david-allison requested changes Jan 8, 2025

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/libanki/Deck.kt Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/FilteredDeckOptions.kt Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/EditDeckDescriptionDialog.kt Outdated Show resolved Hide resolved

github-actions bot added the Has Conflicts label Jan 8, 2025

david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jan 16, 2025

Arthur-Milchior force-pushed the value_class_deck branch from 0f7f2df to b5d4aa3 Compare January 16, 2025 23:11

Arthur-Milchior removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Jan 16, 2025

Arthur-Milchior force-pushed the value_class_deck branch 2 times, most recently from 292c595 to 4f95714 Compare January 16, 2025 23:52

david-allison requested changes Jan 16, 2025

View reviewed changes

Arthur-Milchior force-pushed the value_class_deck branch 3 times, most recently from c9e9db4 to b7ba1ae Compare January 17, 2025 03:12

This comment was marked as resolved.

Sign in to view

david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 17, 2025

david-allison reviewed Jan 17, 2025

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/libanki/Deck.kt Show resolved Hide resolved

david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jan 18, 2025

david-allison self-requested a review January 18, 2025 03:12

This comment was marked as resolved.

Sign in to view

david-allison removed the Needs Review label Jan 18, 2025

david-allison added the cleanup Non functional change that would improve the code readability label Jun 3, 2025

david-allison changed the title ~~Value class deck~~ convert Deck to strongly typed value class Jun 3, 2025

github-actions bot added the Has Conflicts label Jun 4, 2025

Arthur-Milchior force-pushed the value_class_deck branch from 92ec6e9 to f87527a Compare June 15, 2025 11:06

Arthur-Milchior force-pushed the value_class_deck branch from f87527a to e9928af Compare June 15, 2025 11:20

david-allison added the Needs reviewer reply Waiting for a reply from another reviewer label Jun 25, 2025

github-actions bot added the Stale label Jul 9, 2025

github-actions bot removed the Stale label Jul 9, 2025

github-actions bot added the Stale label Jul 31, 2025

github-actions bot closed this Aug 7, 2025

david-allison added the Needs a new dev For PR that were good start but the original dev' left. Any dev can take over and finish it. label Aug 7, 2025

david-allison reopened this Aug 7, 2025

github-actions bot removed the Stale label Aug 7, 2025

Arthur-Milchior force-pushed the value_class_deck branch from e9928af to 593bea9 Compare August 17, 2025 21:44

Arthur-Milchior added 3 commits August 18, 2025 11:47

NF: Split Deck into NormalDeck and Filtered Deck

dc29920

Deck is now declared as an interface so that it can be inherited. `Deck` contains a facotry to decide whether to return a normal deck or a filtered one.

NF: rewrite dynExamples as more readable

3a7a785

NF: Adding David's tests.

d59bd33

This test ensures that every changes possibly done in the deck options are correctly applied

Arthur-Milchior force-pushed the value_class_deck branch from 593bea9 to d59bd33 Compare August 18, 2025 09:48

github-actions bot added the Stale label Sep 1, 2025

github-actions bot closed this Sep 8, 2025

Uh oh!

convert Deck to strongly typed value class #17718

convert Deck to strongly typed value class #17718

Uh oh!

Conversation

Arthur-Milchior commented Jan 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arthur-Milchior commented Jan 16, 2025

Uh oh!

david-allison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Arthur-Milchior commented Jun 15, 2025

Uh oh!

david-allison commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrayanDSO commented Jun 25, 2025

Uh oh!

github-actions bot commented Jul 9, 2025

Uh oh!

david-allison commented Jul 9, 2025

Uh oh!

github-actions bot commented Jul 31, 2025

Uh oh!

david-allison commented Aug 7, 2025

Uh oh!

github-actions bot commented Sep 1, 2025

Uh oh!

david-allison commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-allison left a comment •

edited

Loading

david-allison left a comment •

edited

Loading

Arthur-Milchior Feb 10, 2025 •

edited

Loading

david-allison commented Jun 25, 2025 •

edited

Loading