[#117] Send Sentry report if file path given for logs is not valid #122
Conversation
| logInternal(LogLevel.Warn, sentryErrorTitle) | ||
| } | ||
| } catch (e: Exception) { | ||
| logInternal(LogLevel.Info, "Could not read local DataStore") |
There was a problem hiding this comment.
Safeguarding against any error that might occur with DataStore; since this is the first I've worked with it I want to make sure that failures with it do cause the app to crash.
| // call the datastore methods; we need to use this with caution, but | ||
| // https://developer.android.com/topic/libraries/architecture/datastore#synchronous | ||
| val dataStore = SClogDataStore(context) | ||
| val alreadyReportedFailure = runBlocking { dataStore.getHasReportedFilepathError.first() } |
There was a problem hiding this comment.
This will cause whoever calls initWith() to be blocked.
Can we instead create a new coroutine scope (eg. something like SupervisorJob() + Dispatchers.IO) and wrap the whole try-catch inside a suspend function that can be called inside a coroutine launched by the newly created coroutine scope?
eg.
newCoroutineScope.launch { doInitWith() }
....
private suspend fun doInitWith() { try-catch etc.... }
There was a problem hiding this comment.
Good call. I didn't make the entire method suspend (Timber documentation for planting/updating trees has historically been called from the main thread I believe, at least it's been that way since first implementation)... but I did move the code that checked for file access errors and called runBlocking on the dataStore to be in a new coroutine scope.
| launch { | ||
| // Doesn't need to be lifecycle aware for this test | ||
| val timestamp = | ||
| AppDataStore(baseContext).apply { |
There was a problem hiding this comment.
curious to know why baseContext is passed in here here, but applicationContext is passed into SClogDataStore
There was a problem hiding this comment.
Hrm. Honestly I think that was a discrepancy I missed. Because we're using the context to lookup the datastore instance I don't think it matters which we use (although I could be wrong), but you're right, to be safe we should probably ensure we are using the same context. The examples I have found all use Context, but seem to be passing in the application context. To make this completely explicit I have changed my code to request the Application instead of the Context (Application is a ContextWrapper); I don't know if there's ramifications to this, but it seems to behave the same.
There was a problem hiding this comment.
passing in Application is the right call. Activity Context is tied to the Activity lifecycle, so we don't want any potential issues from carrying out operations tied to an Activity lifecycle.
|
@phileo Hopefully I've addressed your comments. I've been playing around with DataStore a little more this afternoon and there's something I still don't like about having to pass in the Application/Context into the containing class, but I all of the examples I have found that create a |
Related Issue: #117
Summary of Problem:
We are seeing issues where some of the standard file paths are not actually valid on some devices - and as such writes to these file locations are failing. We would like Steamclog to report when this is occurring for it's log files.
Proposed Solution:
Note, #121 was created so that we can investigate further error handling around User Reports not being sent.
Testing Steps: