Remove launchWithCol()

Code inside a withCol() block should not modify the UI, as it runs on a background thread. withCol() should fetch and/or mutate collection data, and then UI operations/program state changes should be performed outside the withCol block, so they're serialized on the main thread and the risk of race conditions is minimized. Recently mentioned on #13886 (comment) launchWithCol() makes it a bit too easy to accidentally run UI code in the background thread, as it provides both an async context and a collection at the same time, and the user will be tempted to do both fetch-from-col and update-UI steps inside the block. Removing it means a few extra characters at each call site, but I think it's a bit less error-prone.
ankidroid · Jun 4, 2023 · 52a3ea9 · 52a3ea9
1 parent d39436d
commit 52a3ea9
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 25 deletions.
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt
@@ -139,20 +139,6 @@ fun Fragment.launchCatchingTask(
     }
 }
 
-/** Launches a [CollectionManager.withCol] job while catching its errors with [launchCatchingTask] */
-fun <T> FragmentActivity.launchWithCol(block: Collection.() -> T): Job {
-    return launchCatchingTask {
-        withCol { block() }
-    }
-}
-
-/** See [FragmentActivity.launchWithCol] */
-fun <T> Fragment.launchWithCol(block: Collection.() -> T): Job {
-    return launchCatchingTask {
-        withCol { block() }
-    }
-}
-
 private fun showError(context: Context, msg: String, exception: Throwable) {
     try {
         AlertDialog.Builder(context).show {

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/AppearanceSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/AppearanceSettingsFragment.kt
@@ -136,7 +136,7 @@ class AppearanceSettingsFragment : SettingsFragment() {
         requirePreference<SwitchPreference>(R.string.show_estimates_preference).apply {
             launchCatchingTask { isChecked = withCol { get_config_boolean("estTimes") } }
             setOnPreferenceChangeListener { newETA ->
-                launchWithCol { set_config("estTimes", newETA) }
+                launchCatchingTask { withCol { set_config("estTimes", newETA) } }
             }
         }
         // Show progress
@@ -145,7 +145,7 @@ class AppearanceSettingsFragment : SettingsFragment() {
         requirePreference<SwitchPreference>(R.string.show_progress_preference).apply {
             launchCatchingTask { isChecked = withCol { get_config_boolean("dueCounts") } }
             setOnPreferenceChangeListener { newDueCountsValue ->
-                launchWithCol { set_config("dueCounts", newDueCountsValue) }
+                launchCatchingTask { withCol { set_config("dueCounts", newDueCountsValue) } }
             }
         }
     }

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt
@@ -48,7 +48,7 @@ class GeneralSettingsFragment : SettingsFragment() {
                 setValueIndex(valueIndex)
             }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config("addToCur", "0" == newValue) }
+                launchCatchingTask { withCol { set_config("addToCur", "0" == newValue) } }
             }
         }
         // Paste PNG
@@ -57,7 +57,7 @@ class GeneralSettingsFragment : SettingsFragment() {
         requirePreference<SwitchPreference>(R.string.paste_png_key).apply {
             launchCatchingTask { isChecked = withCol { get_config("pastePNG", false)!! } }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config("pastePNG", newValue) }
+                launchCatchingTask { withCol { set_config("pastePNG", newValue) } }
             }
         }
         // Error reporting mode

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ReviewingSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ReviewingSettingsFragment.kt
@@ -20,7 +20,6 @@ import androidx.preference.SwitchPreference
 import com.ichi2.anki.CollectionManager.withCol
 import com.ichi2.anki.R
 import com.ichi2.anki.launchCatchingTask
-import com.ichi2.anki.launchWithCol
 import com.ichi2.anki.preferences.Preferences.Companion.getDayOffset
 import com.ichi2.anki.preferences.Preferences.Companion.setDayOffset
 import com.ichi2.anki.reviewer.AutomaticAnswerAction
@@ -40,7 +39,7 @@ class ReviewingSettingsFragment : SettingsFragment() {
         requirePreference<ListPreference>(R.string.new_spread_preference).apply {
             launchCatchingTask { setValueIndex(withCol { get_config_int("newSpread") }) }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config("newSpread", (newValue as String).toInt()) }
+                launchCatchingTask { withCol { set_config("newSpread", (newValue as String).toInt()) } }
             }
         }
 
@@ -51,7 +50,7 @@ class ReviewingSettingsFragment : SettingsFragment() {
         requirePreference<NumberRangePreferenceCompat>(R.string.learn_cutoff_preference).apply {
             launchCatchingTask { setValue(withCol { get_config_int("collapseTime") / 60 }) }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config("collapseTime", (newValue as Int * 60)) }
+                launchCatchingTask { withCol { set_config("collapseTime", (newValue as Int * 60)) } }
             }
         }
         // Timebox time limit
@@ -61,7 +60,7 @@ class ReviewingSettingsFragment : SettingsFragment() {
         requirePreference<NumberRangePreferenceCompat>(R.string.time_limit_preference).apply {
             launchCatchingTask { setValue(withCol { get_config_int("timeLim") / 60 }) }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config("timeLim", (newValue as Int * 60)) }
+                launchCatchingTask { withCol { set_config("timeLim", (newValue as Int * 60)) } }
             }
         }
         // Start of next day
@@ -92,7 +91,7 @@ class ReviewingSettingsFragment : SettingsFragment() {
         requirePreference<ListPreference>(R.string.automatic_answer_action_preference).apply {
             launchCatchingTask { setValueIndex(withCol { get_config(AutomaticAnswerAction.CONFIG_KEY, 0.toInt())!! }) }
             setOnPreferenceChangeListener { newValue ->
-                launchWithCol { set_config(AutomaticAnswerAction.CONFIG_KEY, (newValue as String).toInt()) }
+                launchCatchingTask { withCol { set_config(AutomaticAnswerAction.CONFIG_KEY, (newValue as String).toInt()) } }
             }
         }
         // New timezone handling
@@ -103,9 +102,9 @@ class ReviewingSettingsFragment : SettingsFragment() {
             }
             setOnPreferenceChangeListener { newValue ->
                 if (newValue == true) {
-                    launchWithCol { sched.set_creation_offset() }
+                    launchCatchingTask { withCol { sched.set_creation_offset() } }
                 } else {
-                    launchWithCol { sched.clear_creation_offset() }
+                    launchCatchingTask { withCol { sched.clear_creation_offset() } }
                 }
             }
         }