Value class deck #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

github-actions · 2025-02-09T14:36:21Z

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

It's been a while, but I don't feel my comment introducing a regression test for bugs that this PR previously had was merged:

#17718 (review)

david-allison · 2025-02-12T09:08:58Z

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

+        deck = if (extras?.containsKey("did") == true) {
            col.decks.get(extras.getLong("did"))
        } else {
            null
        } ?: col.decks.current()


deck = extras?.getNullableLong("did")?.let { col.decks.get(it) } ?: col.decks.current()

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

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

Arthur-Milchior · 2025-02-14T00:45:29Z

Sorry for missing your test. It's done

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.

Arthur-Milchior · 2025-02-14T03:13:42Z

I removed your checkResult on startRegularActivity. It caused an unrelated lint failure. I don't understand enough and don't want to fix it in this PR

Initial implementation by David This test ensures that every changes possibly done in the deck options are correctly applied Co-authored-by: David Allison <[email protected]>

david-allison

Only a few string fixes, doesn't block reviewer 2

david-allison · 2025-02-14T09:01:25Z

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

                } catch (e: RuntimeException) {
-                    Timber.e(e, "RuntimeException on saving deck")
+                    Timber.e(e, "RuntimeException on saving filteredDeck")


david-allison · 2025-02-14T09:01:30Z

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

                                commit()
                            }
                        }
                    }
                }

-                // save deck
+                // save filteredDeck


david-allison · 2025-02-14T09:01:49Z

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

        registerExternalStorageListener()
        if (deck.isNormal) {
-            Timber.w("Deck is not a dyn deck")
+            Timber.w("Deck is not a dyn filteredDeck")


Suggested change

Timber.w("Deck is not a dyn filteredDeck")

Timber.w("Deck is not filtered")

david-allison · 2025-02-14T09:01:59Z

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

                }
            }
            addPreferences(col)
            buildLists()
            updateSummaries()
        }

-        // Set the activity title to include the name of the deck
+        // Set the activity title to include the name of the filteredDeck


Suggested change

// Set the activity title to include the name of the filteredDeck

// Set the activity title to include the name of the deck

david-allison · 2025-02-14T09:02:07Z

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

@@ -255,13 +286,13 @@ class FilteredDeckOptions :
    private fun addPreferences(col: Collection) {
        addPreferencesFromResource(R.xml.cram_deck_options)
        if (col.schedVer() != 1) {
-            Timber.d("sched v2: removing filtered deck custom study steps")
+            Timber.d("sched v2: removing filtered filteredDeck custom study steps")


Suggested change

Timber.d("sched v2: removing filtered filteredDeck custom study steps")

Timber.d("sched v2: removing filtered deck custom study steps")

david-allison · 2025-02-14T09:02:16Z

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

@@ -280,9 +311,9 @@ class FilteredDeckOptions :

    override fun closeWithResult() {
        if (prefChanged) {
-            // Rebuild the filtered deck if a setting has changed
+            // Rebuild the filtered filteredDeck if a setting has changed


Suggested change

// Rebuild the filtered filteredDeck if a setting has changed

// Rebuild the filtered deck if a setting has changed

david-allison · 2025-02-14T09:03:58Z

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

+
+        fun isFiltered(jsonObject: JSONObject) = jsonObject.getInt(DYN) != 0
+
+        fun factory(jsonObject: JSONObject): Deck = if (isFiltered(jsonObject)) FilteredDeck(jsonObject) else RegularDeck(jsonObject)


doccomment on these factories

david-allison · 2025-02-14T09:05:18Z

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

-            if (filtered) {
+    private fun newDeckLegacy(filtered: Boolean) =
+        Deck.factory(BackendUtils.fromJsonBytes(col.backend.newDeckLegacy(filtered))).apply {
+            if (this is FilteredDeck) {


hmm... I'd use filtered as the boolean, then require(this is Filtered) { "error" }

david-allison · 2025-02-14T09:06:48Z

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

@@ -378,7 +373,13 @@ class Decks(
    /** Falls back on default config if deck or config missing */
    @LibAnkiAlias("config_dict_for_deck_id")
    fun configDictForDeckId(did: DeckId): DeckConfig {
-        val conf = get(did)?.conf ?: 1
+        val deck = get(did)


Does this work:

val conf = (get(did) as? RegularDeck?)?.conf ?: 1

david-allison · 2025-02-14T09:08:42Z

AnkiDroid/src/main/java/com/ichi2/libanki/utils/PythonExtensions.kt

+ *
+ * This better approximates `JSON.get` in the Python
+ */
+fun JSONObject.getLongOrNull(key: String): Long? {


Very similar to a method in java/com/ichi2/anki/utils/ext/JSONObject.kt

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 Needs Author Reply Waiting for a reply from the original author label Jan 18, 2025

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

github-actions bot added Stale and removed Stale labels Feb 9, 2025

Arthur-Milchior force-pushed the value_class_deck branch 4 times, most recently from d4d5bac to b397440 Compare February 11, 2025 17:54

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

david-allison requested changes Feb 12, 2025

View reviewed changes

Arthur-Milchior force-pushed the value_class_deck branch from b397440 to 03db006 Compare February 14, 2025 00:44

Arthur-Milchior force-pushed the value_class_deck branch from 03db006 to cd6cff9 Compare February 14, 2025 00:55

Arthur-Milchior added 4 commits February 14, 2025 02:25

NF: Split Deck into NormalDeck and Filtered Deck

3f08469

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: Add constants for the decks' key

36b8480

NF: rewrite dynExamples as more readable

b55a074

Arthur-Milchior force-pushed the value_class_deck branch 2 times, most recently from 00541f5 to 6e2969e Compare February 14, 2025 03:13

NF: integration test - Filtered Deck Options

3a14dab

Initial implementation by David This test ensures that every changes possibly done in the deck options are correctly applied Co-authored-by: David Allison <[email protected]>

david-allison force-pushed the value_class_deck branch from 6e2969e to 3a14dab Compare February 14, 2025 08:59

david-allison requested changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value class deck #17718

Value class deck #17718

Arthur-Milchior commented Jan 4, 2025

david-allison left a comment •

edited

Loading

Arthur-Milchior commented Jan 16, 2025

david-allison left a comment •

edited

Loading

david-allison Jan 16, 2025

Arthur-Milchior Feb 10, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

github-actions bot commented Feb 9, 2025

david-allison left a comment

david-allison Feb 12, 2025

Arthur-Milchior commented Feb 14, 2025

Arthur-Milchior commented Feb 14, 2025

david-allison left a comment

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

david-allison Feb 14, 2025

	Timber.w("Deck is not a dyn filteredDeck")
	Timber.w("Deck is not filtered")

	// Set the activity title to include the name of the filteredDeck
	// Set the activity title to include the name of the deck

	Timber.d("sched v2: removing filtered filteredDeck custom study steps")
	Timber.d("sched v2: removing filtered deck custom study steps")

	// Rebuild the filtered filteredDeck if a setting has changed
	// Rebuild the filtered deck if a setting has changed


		fun isFiltered(jsonObject: JSONObject) = jsonObject.getInt(DYN) != 0

		fun factory(jsonObject: JSONObject): Deck = if (isFiltered(jsonObject)) FilteredDeck(jsonObject) else RegularDeck(jsonObject)

Value class deck #17718

Are you sure you want to change the base?

Value class deck #17718

Conversation

Arthur-Milchior commented Jan 4, 2025

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

Arthur-Milchior commented Jan 16, 2025

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthur-Milchior Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

github-actions bot commented Feb 9, 2025

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthur-Milchior commented Feb 14, 2025

Arthur-Milchior commented Feb 14, 2025

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment •

edited

Loading

david-allison left a comment •

edited

Loading

Arthur-Milchior Feb 10, 2025 •

edited

Loading