Skip to content

Commit 88e00df

Browse files
criticalAYcriticalAY
committed
refactor: extract crash reporting interface to :common
As we move towards a multi-module architecture, feature modules need crash reporting without depending on the app module. Without this, each feature module would need its own bridge interface (e.g. WidgetCrashReporter, BrowserCrashReporter) duplicating the same pattern everywhere.
1 parent 46f0a91 commit 88e00df

6 files changed

Lines changed: 263 additions & 82 deletions

File tree

‎AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ACRATest.kt‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,17 @@ import androidx.test.annotation.UiThreadTest
2323
import androidx.test.ext.junit.runners.AndroidJUnit4
2424
import com.ichi2.anki.AnkiDroidApp
2525
import com.ichi2.anki.CrashReportService
26-
import com.ichi2.anki.CrashReportService.FEEDBACK_REPORT_ALWAYS
27-
import com.ichi2.anki.CrashReportService.FEEDBACK_REPORT_ASK
26+
import com.ichi2.anki.CrashReporter
27+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_ALWAYS
28+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_ASK
2829
import com.ichi2.anki.R
30+
import com.ichi2.anki.acraCoreConfigBuilder
2931
import com.ichi2.anki.analytics.UsageAnalytics
3032
import com.ichi2.anki.logging.ProductionCrashReportingTree
3133
import com.ichi2.anki.preferences.sharedPrefs
3234
import com.ichi2.anki.servicelayer.ThrowableFilterService
35+
import com.ichi2.anki.setDebugACRAConfig
36+
import com.ichi2.anki.setProductionACRAConfig
3337
import com.ichi2.anki.testutil.GrantStoragePermission
3438
import org.acra.ACRA
3539
import org.acra.builder.ReportBuilder
@@ -70,7 +74,7 @@ class ACRATest : InstrumentedTest() {
7074
@Throws(Exception::class)
7175
fun testDebugConfiguration() {
7276
// Debug mode overrides all saved state so no setup needed
73-
CrashReportService.setDebugACRAConfig(sharedPrefs)
77+
setDebugACRAConfig(sharedPrefs)
7478
assertArrayEquals(
7579
"Debug logcat arguments not set correctly",
7680
CrashReportService.acraCoreConfigBuilder
@@ -90,21 +94,21 @@ class ACRATest : InstrumentedTest() {
9094
)
9195
assertEquals(
9296
"ACRA feedback was not turned off correctly",
93-
CrashReportService.FEEDBACK_REPORT_NEVER,
97+
CrashReporter.FEEDBACK_REPORT_NEVER,
9498
sharedPrefs
95-
.getString(CrashReportService.FEEDBACK_REPORT_KEY, "undefined"),
99+
.getString(CrashReporter.FEEDBACK_REPORT_KEY, "undefined"),
96100
)
97101
}
98102

99103
@Test
100104
@Throws(Exception::class)
101105
fun testProductionConfigurationUserDisabled() {
102106
// set up as if the user had prefs saved to disable completely
103-
setReportConfig(CrashReportService.FEEDBACK_REPORT_NEVER)
107+
setReportConfig(CrashReporter.FEEDBACK_REPORT_NEVER)
104108

105109
// ACRA initializes production logcat via annotation and we can't mock Build.DEBUG
106110
// That means we are restricted from verifying production logcat args and this is the debug case again
107-
CrashReportService.setProductionACRAConfig(sharedPrefs)
111+
setProductionACRAConfig(sharedPrefs)
108112
verifyDebugACRAPreferences()
109113
}
110114

@@ -115,7 +119,7 @@ class ACRATest : InstrumentedTest() {
115119
setReportConfig(FEEDBACK_REPORT_ASK)
116120

117121
// If the user is set to ask, then it's production, with interaction mode dialog
118-
CrashReportService.setProductionACRAConfig(sharedPrefs)
122+
setProductionACRAConfig(sharedPrefs)
119123
verifyACRANotDisabled()
120124

121125
assertToastMessage(R.string.feedback_for_manual_toast_text)
@@ -134,7 +138,7 @@ class ACRATest : InstrumentedTest() {
134138

135139
// If the user is set to always, then it's production, with interaction mode toast
136140
// will be useful with ACRA 5.2.0
137-
CrashReportService.setProductionACRAConfig(sharedPrefs)
141+
setProductionACRAConfig(sharedPrefs)
138142

139143
// The same class/method combo is only sent once, so we face a new method each time (should test that system later)
140144
val crash = Exception("testCrashReportSend at " + System.currentTimeMillis())
@@ -173,7 +177,7 @@ class ACRATest : InstrumentedTest() {
173177
)
174178

175179
// Now let's clear data
176-
CrashReportService.deleteACRALimiterData(testContext)
180+
CrashReportService.deleteLimiterData(testContext)
177181

178182
// A third send should work again
179183
assertTrue(
@@ -192,7 +196,7 @@ class ACRATest : InstrumentedTest() {
192196
setReportConfig(FEEDBACK_REPORT_ALWAYS)
193197

194198
// If the user is set to always, then it's production, with interaction mode toast
195-
CrashReportService.setProductionACRAConfig(sharedPrefs)
199+
setProductionACRAConfig(sharedPrefs)
196200
verifyACRANotDisabled()
197201

198202
assertToastMessage(R.string.feedback_auto_toast_text)
@@ -207,7 +211,7 @@ class ACRATest : InstrumentedTest() {
207211
setReportConfig(FEEDBACK_REPORT_ALWAYS)
208212

209213
// If the user is set to ask, then it's production, with interaction mode dialog
210-
CrashReportService.setProductionACRAConfig(sharedPrefs)
214+
setProductionACRAConfig(sharedPrefs)
211215
verifyACRANotDisabled()
212216

213217
assertDialogEnabledStatus("dialog should be disabled when status is ALWAYS", false)
@@ -226,7 +230,7 @@ class ACRATest : InstrumentedTest() {
226230
setReportConfig(FEEDBACK_REPORT_ASK)
227231

228232
// If the user is set to ask, then it's production, with interaction mode dialog
229-
CrashReportService.setProductionACRAConfig(sharedPrefs)
233+
setProductionACRAConfig(sharedPrefs)
230234
verifyACRANotDisabled()
231235

232236
assertToastMessage(R.string.feedback_for_manual_toast_text)
@@ -282,7 +286,7 @@ class ACRATest : InstrumentedTest() {
282286
}
283287

284288
private fun setAcraReportingMode(feedbackReportAlways: String) {
285-
CrashReportService.setAcraReportingMode(feedbackReportAlways)
289+
CrashReportService.setReportingMode(feedbackReportAlways)
286290
}
287291

288292
@Throws(ACRAConfigurationException::class)
@@ -333,7 +337,7 @@ class ACRATest : InstrumentedTest() {
333337
}
334338

335339
private fun setReportConfig(feedbackReportAsk: String) {
336-
sharedPrefs.edit { putString(CrashReportService.FEEDBACK_REPORT_KEY, feedbackReportAsk) }
340+
sharedPrefs.edit { putString(CrashReporter.FEEDBACK_REPORT_KEY, feedbackReportAsk) }
337341
}
338342

339343
private val sharedPrefs: SharedPreferences

AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt renamed to AnkiDroid/src/main/java/com/ichi2/anki/AcraCrashReporter.kt

Lines changed: 45 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ import androidx.annotation.VisibleForTesting
2323
import androidx.core.content.edit
2424
import androidx.core.content.pm.PackageInfoCompat
2525
import androidx.webkit.WebViewCompat
26+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_ALWAYS
27+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_ASK
28+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_KEY
29+
import com.ichi2.anki.CrashReporter.Companion.FEEDBACK_REPORT_NEVER
2630
import com.ichi2.anki.analytics.AnkiDroidCrashReportDialog
2731
import com.ichi2.anki.analytics.UsageAnalytics
2832
import com.ichi2.anki.analytics.UsageAnalytics.sendAnalyticsException
@@ -43,13 +47,7 @@ import org.acra.config.ToastConfigurationBuilder
4347
import org.acra.sender.HttpSender
4448
import timber.log.Timber
4549

46-
object CrashReportService {
47-
// ACRA constants used for stored preferences
48-
const val FEEDBACK_REPORT_KEY = "reportErrorMode"
49-
const val FEEDBACK_REPORT_ASK = "2"
50-
const val FEEDBACK_REPORT_NEVER = "1"
51-
const val FEEDBACK_REPORT_ALWAYS = "0"
52-
50+
private object AcraCrashReporter : CrashReporter {
5351
/** Our ACRA configurations, initialized during Application.onCreate() */
5452
@JvmStatic
5553
private var logcatArgs =
@@ -166,15 +164,15 @@ object CrashReportService {
166164
*/
167165
@JvmStatic
168166
fun initialize(application: Application) {
169-
CrashReportService.application = application
167+
this.application = application
170168
// FIXME ACRA needs to reinitialize after language is changed, but with the new language
171169
// this is difficult because the Application (AnkiDroidApp) does not change it's baseContext
172170
// perhaps a solution could be to change AnkiDroidApp to have a context wrapper that it sets
173171
// as baseContext, and that wrapper allows a resources/configuration update, then
174172
// in GeneralSettingsFragment for the language dialog change listener, the context wrapper
175173
// could be updated directly with the new locale code so that calling getString on would fetch
176174
// the new language string ?
177-
toastText = ToastType.AUTO_TOAST.getToastMessage(CrashReportService.application)
175+
toastText = ToastType.AUTO_TOAST.getToastMessage(application)
178176

179177
// Setup logging and crash reporting
180178
if (BuildConfig.DEBUG) {
@@ -195,7 +193,7 @@ object CrashReportService {
195193
* Set the reporting mode for ACRA based on the value of the FEEDBACK_REPORT_KEY preference
196194
* @param value value of FEEDBACK_REPORT_KEY preference
197195
*/
198-
fun setAcraReportingMode(value: String) {
196+
override fun setReportingMode(value: String) {
199197
application.sharedPrefs().edit {
200198
// Set the ACRA disable value
201199
if (value == FEEDBACK_REPORT_NEVER) {
@@ -225,7 +223,7 @@ object CrashReportService {
225223
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
226224
fun setDebugACRAConfig(prefs: SharedPreferences) {
227225
// Disable crash reporting
228-
setAcraReportingMode(FEEDBACK_REPORT_NEVER)
226+
setReportingMode(FEEDBACK_REPORT_NEVER)
229227
prefs.edit { putString(FEEDBACK_REPORT_KEY, FEEDBACK_REPORT_NEVER) }
230228
// Use a wider logcat filter in case crash reporting manually re-enabled
231229
logcatArgs = arrayOf("-t", "1500", "-v", "long", "ACRA:S")
@@ -240,7 +238,7 @@ object CrashReportService {
240238
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
241239
fun setProductionACRAConfig(prefs: SharedPreferences) {
242240
// Enable or disable crash reporting based on user setting
243-
setAcraReportingMode(prefs.getString(FEEDBACK_REPORT_KEY, FEEDBACK_REPORT_ASK)!!)
241+
setReportingMode(prefs.getString(FEEDBACK_REPORT_KEY, FEEDBACK_REPORT_ASK)!!)
244242
}
245243

246244
private fun fetchWebViewInformation(): HashMap<String, String> {
@@ -262,17 +260,24 @@ object CrashReportService {
262260
}
263261

264262
/** Used when we don't have an exception to throw, but we know something is wrong and want to diagnose it */
265-
fun sendExceptionReport(
263+
override fun sendExceptionReport(
266264
message: String?,
267265
origin: String?,
268266
) = sendExceptionReport(ManuallyReportedException(message), origin)
269267

270-
fun sendExceptionReport(
268+
override fun sendExceptionReport(
271269
e: Throwable,
272270
origin: String?,
273-
additionalInfo: String? = null,
274-
onlyIfSilent: Boolean = false,
275-
context: Context = application.applicationContext,
271+
additionalInfo: String?,
272+
onlyIfSilent: Boolean,
273+
) = sendExceptionReport(e, origin, additionalInfo, onlyIfSilent, application.applicationContext)
274+
275+
override fun sendExceptionReport(
276+
e: Throwable,
277+
origin: String?,
278+
additionalInfo: String?,
279+
onlyIfSilent: Boolean,
280+
context: Context,
276281
) {
277282
sendAnalyticsException(e, false)
278283
AnkiDroidApp.sentExceptionReportHack = true
@@ -297,7 +302,7 @@ object CrashReportService {
297302

298303
fun isProperServiceProcess(): Boolean = ACRA.isACRASenderServiceProcess()
299304

300-
fun isAcraEnabled(
305+
override fun isEnabled(
301306
context: Context,
302307
defaultValue: Boolean,
303308
): Boolean {
@@ -314,21 +319,21 @@ object CrashReportService {
314319
*
315320
* @param context the context leading to the directory with ACRA limiter data
316321
*/
317-
fun deleteACRALimiterData(context: Context) {
322+
override fun deleteLimiterData(context: Context) {
318323
try {
319324
LimiterData().store(context)
320325
} catch (e: Exception) {
321326
Timber.w(e, "Unable to clear ACRA limiter data")
322327
}
323328
}
324329

325-
fun onPreferenceChanged(
330+
override fun onPreferenceChanged(
326331
ctx: Context,
327332
newValue: String,
328333
) {
329-
setAcraReportingMode(newValue)
334+
setReportingMode(newValue)
330335
// If the user changed error reporting, make sure future reports have a chance to post
331-
deleteACRALimiterData(ctx)
336+
deleteLimiterData(ctx)
332337
// We also need to re-chain our UncaughtExceptionHandlers
333338
UsageAnalytics.reInitialize()
334339
ThrowableFilterService.reInitialize()
@@ -338,7 +343,8 @@ object CrashReportService {
338343
* @return the status of the report, true if the report was sent, false if the report is already
339344
* submitted
340345
*/
341-
fun sendReport(ankiActivity: AnkiActivity): Boolean {
346+
override fun sendReport(activity: android.app.Activity): Boolean {
347+
val ankiActivity = activity as AnkiActivity
342348
val preferences = ankiActivity.sharedPrefs()
343349
val reportMode = preferences.getString(FEEDBACK_REPORT_KEY, "")
344350
return if (FEEDBACK_REPORT_NEVER == reportMode) {
@@ -359,7 +365,7 @@ object CrashReportService {
359365
val currentTimestamp = TimeManager.time.intTimeMS()
360366
val lastReportTimestamp = getTimestampOfLastReport(activity)
361367
return if (currentTimestamp - lastReportTimestamp > MIN_INTERVAL_MS) {
362-
deleteACRALimiterData(activity)
368+
deleteLimiterData(activity)
363369
sendExceptionReport(
364370
UserSubmittedException(EXCEPTION_MESSAGE),
365371
"AnkiDroidApp.HelpDialog",
@@ -385,45 +391,23 @@ object CrashReportService {
385391
}
386392

387393
/**
388-
* Runs the provided block, catching [Exception], logging it and reporting it to [CrashReportService]
389-
*
390-
* **Example**
391-
* ```
392-
* runCatchingWithReport("callingMethod", onlyIfSilent = true) {
393-
* doSomethingRisky()
394-
* }
395-
* ```
396-
*
397-
* **Note**: This differs from [runCatching] - `Error` is thrown
398-
*
399-
* @param origin Data logged to Timber, and provided as the 'origin' field in the error report
400-
* @param onlyIfSilent Skip crash report if the crash reporting service is not 'always accept'
401-
* @param block Code to execute
402-
*
403-
* @throws Error If raised, this will be reported and rethrown
404-
*
405-
* @return A Result containing either the successful result of [block] or the [Exception] thrown
406-
*/
407-
fun <T> runCatchingWithReport(
408-
origin: String?,
409-
onlyIfSilent: Boolean = false,
410-
block: () -> T,
411-
): Result<T> =
412-
try {
413-
Result.success(block())
414-
} catch (e: Throwable) {
415-
Timber.w(e, origin)
416-
CrashReportService.sendExceptionReport(e, origin, onlyIfSilent = onlyIfSilent)
417-
if (e is Error) throw e
418-
Result.failure(e)
419-
}
420-
421-
/**
422-
* Initializes ACRA crash reporting.
394+
* Initializes ACRA crash reporting and wires it up as the
395+
* global [CrashReportService] reporter.
423396
*/
424397
context(application: Application)
425398
fun initializeAcraCrashReporter() {
426-
CrashReportService.initialize(application)
399+
AcraCrashReporter.initialize(application)
400+
CrashReportService.setReporter(AcraCrashReporter)
427401
}
428402

429-
fun isAcraSenderProcess(): Boolean = CrashReportService.isProperServiceProcess()
403+
fun isAcraSenderProcess(): Boolean = AcraCrashReporter.isProperServiceProcess()
404+
405+
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
406+
val CrashReportService.acraCoreConfigBuilder: CoreConfigurationBuilder
407+
get() = AcraCrashReporter.acraCoreConfigBuilder
408+
409+
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
410+
fun setDebugACRAConfig(sharedPrefs: SharedPreferences) = AcraCrashReporter.setDebugACRAConfig(sharedPrefs)
411+
412+
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
413+
fun setProductionACRAConfig(sharedPrefs: SharedPreferences) = AcraCrashReporter.setProductionACRAConfig(sharedPrefs)

‎AnkiDroid/src/main/java/com/ichi2/anki/analytics/AnkiDroidCrashReportDialog.kt‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import android.os.Bundle
2323
import android.view.View
2424
import androidx.core.content.edit
2525
import com.ichi2.anki.CrashReportService
26+
import com.ichi2.anki.CrashReporter
2627
import com.ichi2.anki.R
2728
import com.ichi2.anki.databinding.DialogFeedbackBinding
2829
import com.ichi2.anki.preferences.sharedPrefs
@@ -87,11 +88,11 @@ class AnkiDroidCrashReportDialog :
8788
if (autoReport) {
8889
preferences.edit {
8990
putString(
90-
CrashReportService.FEEDBACK_REPORT_KEY,
91-
CrashReportService.FEEDBACK_REPORT_ALWAYS,
91+
CrashReporter.FEEDBACK_REPORT_KEY,
92+
CrashReporter.FEEDBACK_REPORT_ALWAYS,
9293
)
9394
}
94-
CrashReportService.setAcraReportingMode(CrashReportService.FEEDBACK_REPORT_ALWAYS)
95+
CrashReportService.setReportingMode(CrashReporter.FEEDBACK_REPORT_ALWAYS)
9596
}
9697
// Send the crash report
9798
helper!!.sendCrash(binding.userComment.text.toString(), "")
@@ -100,7 +101,7 @@ class AnkiDroidCrashReportDialog :
100101
// The limiter persists it's limit info *before* the user cancels.
101102
// Therefore, on cancel, purge limits to make sure the user may actually send in future.
102103
// Better to maybe send to many reports than definitely too few.
103-
CrashReportService.deleteACRALimiterData(this)
104+
CrashReportService.deleteLimiterData(this)
104105
helper!!.cancelReports()
105106
}
106107
finish()

‎AnkiDroid/src/main/java/com/ichi2/anki/preferences/DeveloperOptionsFragment.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class DeveloperOptionsFragment : SettingsFragment() {
6060
requirePreference<Preference>(R.string.pref_trigger_crash_key).setOnPreferenceClickListener {
6161
// If we don't delete the limiter data, our test crash may not go through,
6262
// but we are triggering it very much on purpose, we want to see the crash in ACRA
63-
this.context?.let { c -> CrashReportService.deleteACRALimiterData(c) }
63+
this.context?.let { c -> CrashReportService.deleteLimiterData(c) }
6464

6565
Timber.w("Crash triggered on purpose from advanced preferences in debug mode")
6666
throw RuntimeException("This is a test crash")

‎AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/DebugInfoService.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ object DebugInfoService {
5353
.replace("\n", " \n")
5454
}
5555

56-
private fun isSendingCrashReports(context: Context): Boolean = CrashReportService.isAcraEnabled(context, false)
56+
private fun isSendingCrashReports(context: Context): Boolean = CrashReportService.isEnabled(context, false)
5757
}
5858

5959
/**

0 commit comments

Comments
 (0)