CheckIn deletion (EXPOSUREAPP-5410) #2654
Conversation
d4rken
commented
Mar 19, 2021
d4rken
commented
- Wire up UI elements with delete confirmation and repository
- Add delete method to respository
- Refactore repository to offer blocking calls to the UI
d4rken
added
enhancement
# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckIn.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckInRepository.kt
...droidTest/java/de/rki/coronawarnapp/eventregistration/storage/TraceLocationCheckInDaoTest.kt
Show resolved
Hide resolved
| @@ -68,6 +76,22 @@ class CheckInsViewModel @AssistedInject constructor( | |||
| savedState.set(SKEY_LAST_DEEPLINK, deepLink) | |||
| } | |||
|
|
|||
| fun onRemoveCheckInConfirmed(checkIn: CheckIn?) { | |||
| Timber.d("removeCheckin(checkIn=%s)", checkIn) | |||
| launch(scope = appScope) { | |||
There was a problem hiding this comment.
Because of the withContext(NonCancellable) in the Repository, I think we don't need to use the appScope and can simply use the viewModelScope
There was a problem hiding this comment.
I think there might still be a racecondition.
Maybe, I'll think about this a bit more and fix it in a follow up PR if necessary.
launch(scope = appScope) {
if (checkIn == null) { // <------ If we cancel the scope here, is `clear` still called?
checkInsRepository.clear()
} else {
checkInsRepository.deleteCheckIns(listOf(checkIn))
}
}There was a problem hiding this comment.
Oh, good point. I am pretty sure clear() wouldn't be called in the situation you described, because the suspend function withContext() would immediately throw a CancellationException and the "NonCancellable" code block wouldn't be executed.
=> Leave it as it is :)
| checkInDao.deleteAll() | ||
| } | ||
| suspend fun updateCheckIn(checkInId: Long, update: (CheckIn) -> CheckIn) = withContext(NonCancellable) { | ||
| Timber.d("updateCheckIn(checkInId=%d, update=%s)", checkInId, update) |
There was a problem hiding this comment.
Aren't String templates nicer in Kotlin? 😀
There was a problem hiding this comment.
If we print plain values like checkInId, yes, because there won't be a huge impact, but if we print complex parameters like data classes or callbacks in this case, using formatting args is better for performance, if logging is disabled, as they will not be evaluated toString().
We could now put checkInId as template, and update as formatting arg, but.... would this be a blocker for you 🙂 ?
There was a problem hiding this comment.
Thanks for the explanation. Then its fine for me as it is.
|
Kudos, SonarCloud Quality Gate passed! |
