[criticalAY]

Note

Assisted-by: Opus 5.5

Purpose / Description

After a night mode switch, confirming a change in the note type field editor did nothing. An open field menu also acted on the first field, so Delete could remove the wrong one.

• First step of ConfirmationDialog loses callbacks on activity recreation #22714, covering the field editor. The other ConfirmationDialog callers follow.

Fixes

• Part of ConfirmationDialog loses callbacks on activity recreation #22714

Approach

The editor saves the selected field, and ConfirmationDialog now returns a fragment result that the editor listens for in onCreate.

How Has This Been Tested?

test (fails on main).

Learning (optional, can help others)

No activity handles uiMode, so a night mode switch recreates the screen while rotation doesn't.

Checklist

Please, go through these checks before submitting the PR.

• 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

[mikehardy]

Assisted-by: Grok 4.7

Thank you — splitting “which field” from “the confirm click still does the edit” makes the failure mode easy to follow, and the Robolectric tests recreate the activity before the click.

Rebased two commits onto current main locally (no push). Ran lint (lintAll, ktlintCheck, lint-rules:test), unit (jacocoUnitTestReport), and release emulator tests on emulator-5556 / API35_GAPI; all passed.

Item click shows NoteTypeFieldEditorContextMenu with the field label, then sets currentPos. After recreation the restored menu’s item click uses the new activity. Delete, the field-delete warning, and the full-sync delete confirm all call deleteField(), which removes noteFields[currentPos]. The delete listener also calls modSchema(check = false) and popBackStackImmediate of the whole back stack, which is the old confirm body.

Add’s name dialog is still an AlertDialog. On ConfirmModSchemaException it shows a ConfirmationDialog whose result bundle holds field_name. The listener calls addField(name, false), which skips the schema check and calls modSchema(check = false). Reposition stores newPosition - 1 as field_index and, on confirm, modSchema(check = false) then moveFieldLegacy of noteFields[currentPos] to that index. Sort does not put an index in the bundle; on confirm it modSchema(check = false) and changeSortField(notetype, currentPos).

NoteTypeFieldEditorRecreationTest launches the Basic note type, opens the menu on Back (position 1), and uses ActivityController.configurationChange() after +night, asserting a new activity. One test recreates before choosing Delete. The others recreate with the confirm already showing, including the synced-collection full-sync path (ls = scm) for delete, add (Extra), reposition (input 1 → Back, Front), and sort (sortf == 1).

some acceptable findings, no action required, just leaving for posterity: rename’s full-sync ConfirmationDialog in renameFieldDialog still uses setConfirm; ConfirmationDialog’s class KDoc still says to call setArgs and setConfirm only though newInstance is what survives recreation.

[david-allison]

LGTM, thanks so much!!!

[david-allison]

nit/style: I like these as extension methods

[david-allison]

nit/style: I like these as extension methods

[david-allison]

nit/style: could do with a method on ConfirmationDialog

[david-allison]

I would expect this on ConfirmationDialog and handling ARG_REQUEST_KEY inside the method

[mikehardy]

Assisted-by: Grok 4.7

Thank you — saving the field name and pending action through recreation fixes the wrong-field deletes and the startup race David flagged.

Locally I rebased onto main without force-pushing (prep stayed at the PR tip). I ran lint (lintAll, ktlintCheck, lint-rules:test), unit tests (jacocoUnitTestReport), and release emulator tests (TEST_RELEASE_BUILD=true via run_emulator_tests.py on emulator-5556 / API35_GAPI); all passed.

Reviewed ConfirmationDialog result bundles for delete/sort/reposition/add, selectedFieldName lookup after restore, and tests that block collection load until confirm plus schema reorder before restore.

some acceptable findings, no action required, just leaving for posterity: rename full-sync confirm still uses in-memory setConfirm; KDoc still mentions setConfirm only.

[david-allison]

legacy database resurrection after deletion

diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseDeletionRegressionTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseDeletionRegressionTest.kt
new file mode 100644
index 0000000000..e581bbe02c
--- /dev/null
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseDeletionRegressionTest.kt
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+@file:Suppress("DEPRECATION")
+
+package com.ichi2.anki.multiprofile
+
+import android.app.Application
+import android.content.Context
+import android.database.sqlite.SQLiteDatabase
+import androidx.test.core.app.ApplicationProvider
+import com.ichi2.testutils.common.OS
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Assume.assumeFalse
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import org.robolectric.annotation.SQLiteMode
+import java.io.File
+
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [27], application = Application::class)
+@SQLiteMode(SQLiteMode.Mode.NATIVE)
+class ProfileDatabaseDeletionRegressionTest {
+    @Test
+    fun `deleting a database does not restore a conflicting legacy copy`() {
+        assumeFalse("Windows paths cannot hold the nested copy", OS.WINDOWS.isRunning())
+        val base = ApplicationProvider.getApplicationContext<Context>()
+        val root = File(base.filesDir.parentFile, "p_delete_regression")
+        val profile = ProfileContextWrapper.create(base, ProfileId("p_delete_regression"), root)
+        val database = File(profile.noBackupFilesDir, "androidx.work.workdb")
+        val legacy = File(File(root, "databases"), database.absolutePath)
+        seed(database, 99)
+        seed(legacy, 73)
+
+        // The existing destination wins when both locations contain a database.
+        profile.openOrCreateDatabase(database.path, Context.MODE_PRIVATE, null).use { db ->
+            db.rawQuery("SELECT value FROM marker", null).use { cursor ->
+                assertTrue(cursor.moveToFirst())
+                assertEquals(99, cursor.getInt(0))
+            }
+        }
+        assertTrue(profile.deleteDatabase(database.path))
+
+        profile.openOrCreateDatabase(database.path, Context.MODE_PRIVATE, null).use { db ->
+            db.rawQuery("SELECT count(*) FROM sqlite_master WHERE type='table' AND name='marker'", null).use { cursor ->
+                assertTrue(cursor.moveToFirst())
+                assertEquals("Deletion must not resurrect the legacy database", 0, cursor.getInt(0))
+            }
+        }
+    }
+
+    private fun seed(
+        file: File,
+        value: Int,
+    ) {
+        file.parentFile!!.mkdirs()
+        SQLiteDatabase.openOrCreateDatabase(file, null).use { db ->
+            db.execSQL("CREATE TABLE marker (value INTEGER)")
+            db.execSQL("INSERT INTO marker VALUES ($value)")
+        }
+    }
+}

Data loss after a failed WAL move

diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseJournalMoveFailureRegressionTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseJournalMoveFailureRegressionTest.kt
new file mode 100644
index 0000000000..f8db6e08de
--- /dev/null
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/multiprofile/ProfileDatabaseJournalMoveFailureRegressionTest.kt
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+@file:Suppress("DEPRECATION")
+
+package com.ichi2.anki.multiprofile
+
+import android.app.Application
+import android.content.Context
+import android.database.sqlite.SQLiteDatabase
+import androidx.test.core.app.ApplicationProvider
+import com.ichi2.testutils.common.OS
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Assume.assumeFalse
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import org.robolectric.annotation.SQLiteMode
+import java.io.File
+import java.io.IOException
+
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [27], application = Application::class)
+@SQLiteMode(SQLiteMode.Mode.NATIVE)
+class ProfileDatabaseJournalMoveFailureRegressionTest {
+    @Test
+    fun `failed journal move does not hide a committed transaction`() {
+        assumeFalse("Windows paths cannot hold the nested copy", OS.WINDOWS.isRunning())
+        val base = ApplicationProvider.getApplicationContext<Context>()
+        val root = File(base.filesDir.parentFile, "p_journal_move_failure")
+        val profile = ProfileContextWrapper.create(base, ProfileId("p_journal_move_failure"), root)
+        val destination = File(profile.noBackupFilesDir, "androidx.work.workdb")
+        val nested = File(File(root, "databases"), destination.absolutePath)
+        val seed = File(base.cacheDir, "journal-move-seed.db")
+        nested.parentFile!!.mkdirs()
+        SQLiteDatabase.openOrCreateDatabase(seed, null).use { db ->
+            db.execSQL("CREATE TABLE marker (value INTEGER)")
+            assertTrue(db.enableWriteAheadLogging())
+            db.rawQuery("PRAGMA wal_autocheckpoint=0", null).close()
+            db.execSQL("INSERT INTO marker VALUES (73)")
+            val wal = File(seed.path + "-wal")
+            assertTrue("The committed insert must still be in WAL", wal.length() > 32)
+            seed.copyTo(nested)
+            wal.copyTo(File(nested.path + "-wal"))
+        }
+        // Resume a previous migration interrupted after moving only the main database.
+        assertTrue(nested.renameTo(destination))
+        val sourceParent = nested.parentFile!!
+        assertTrue(sourceParent.setWritable(false, false))
+        try {
+            try {
+                profile.openOrCreateDatabase(destination.path, Context.MODE_PRIVATE, null).use(::assertMarker)
+            } catch (expected: IOException) {
+                // Refusing to open is safe while the committed journal cannot be moved.
+                assertTrue(File(nested.path + "-wal").length() > 32)
+            }
+        } finally {
+            sourceParent.setWritable(true, true)
+        }
+        // An explicit refusal must still leave the committed data recoverable.
+        profile.openOrCreateDatabase(destination.path, Context.MODE_PRIVATE, null).use(::assertMarker)
+    }
+
+    private fun assertMarker(database: SQLiteDatabase) {
+        database.rawQuery("SELECT value FROM marker", null).use { cursor ->
+            assertTrue("Opening without the stranded WAL hides committed data", cursor.moveToFirst())
+            assertEquals(73, cursor.getInt(0))
+        }
+    }
+}

Unresolved candidate and validation limits

// SPDX-License-Identifier: GPL-3.0-or-later

@file:Suppress("DEPRECATION")

package com.ichi2.anki.multiprofile

import android.app.Application
import android.content.Context
import android.database.sqlite.SQLiteDatabase
import androidx.test.core.app.ApplicationProvider
import com.ichi2.testutils.common.OS
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assume.assumeFalse
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.annotation.Config
import org.robolectric.annotation.SQLiteMode
import java.io.File

@RunWith(RobolectricTestRunner::class)
@Config(sdk = [27], application = Application::class)
@SQLiteMode(SQLiteMode.Mode.NATIVE)
class ProfileDatabaseOrphanWalRegressionTest {
    @Test
    fun `an orphan WAL cannot replace a newly created database`() {
        assumeFalse("Windows paths cannot hold the nested copy", OS.WINDOWS.isRunning())
        val base = ApplicationProvider.getApplicationContext<Context>()
        val root = File(base.filesDir.parentFile, "p_orphan_wal_regression")
        val profile = ProfileContextWrapper.create(base, ProfileId("p_orphan_wal_regression"), root)
        val destination = File(profile.noBackupFilesDir, "androidx.work.workdb")
        val nested = File(File(root, "databases"), destination.absolutePath)
        val seed = File(base.cacheDir, "orphan-wal-seed.db")
        seed(seed, 10)
        SQLiteDatabase.openOrCreateDatabase(seed, null).use { db ->
            assertTrue(db.enableWriteAheadLogging())
            db.execSQL("UPDATE marker SET value=73")
            nested.parentFile!!.mkdirs()
            File(seed.path + "-wal").copyTo(File(nested.path + "-wal"))
        }
        // The old database is absent; a helper now creates a fresh destination.
        assertEquals(destination, profile.getDatabasePath(destination.path))
        seed(destination, 99)
        SQLiteDatabase.openOrCreateDatabase(destination, null).use { db ->
            assertTrue(db.enableWriteAheadLogging())
        }
        profile.openOrCreateDatabase(destination.path, Context.MODE_PRIVATE, null).use { db ->
            db.rawQuery("SELECT value FROM marker", null).use { cursor ->
                assertTrue(cursor.moveToFirst())
                assertEquals("A leftover WAL belongs to the old database", 99, cursor.getInt(0))
            }
        }
    }

    private fun seed(
        file: File,
        value: Int,
    ) {
        file.parentFile!!.mkdirs()
        SQLiteDatabase.openOrCreateDatabase(file, null).use { db ->
            db.execSQL("CREATE TABLE marker (value INTEGER)")
            db.execSQL("INSERT INTO marker VALUES ($value)")
        }
    }
}