From a4bba93f8c6ba2f81141272215f6726abee57f1f Mon Sep 17 00:00:00 2001 From: Artem Chepurnyi Date: Thu, 8 Aug 2024 07:19:41 +0300 Subject: [PATCH] improvement: Proper export Cancellation and better notification handling --- .../receiver/VaultExportActionReceiver.kt | 15 +---- .../android/downloader/worker/ExportWorker.kt | 64 ++++++++++--------- .../common/service/export/ExportManager.kt | 8 ++- .../service/export/impl/ExportManagerImpl.kt | 29 ++++----- .../impl/DownloadAttachmentMetadataImpl.kt | 5 ++ .../state/RememberStateFlowScope.kt | 21 ++++++ .../state/RememberStateFlowScopeImpl.kt | 16 +---- .../keyguard/copy/ExportManagerImpl.kt | 43 +++++++------ 8 files changed, 109 insertions(+), 92 deletions(-) diff --git a/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/receiver/VaultExportActionReceiver.kt b/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/receiver/VaultExportActionReceiver.kt index 93a26bf..7dc1d19 100644 --- a/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/receiver/VaultExportActionReceiver.kt +++ b/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/receiver/VaultExportActionReceiver.kt @@ -4,13 +4,9 @@ import android.content.BroadcastReceiver import android.content.ComponentName import android.content.Context import android.content.Intent -import androidx.work.ListenableWorker.Result -import com.artemchep.keyguard.common.io.launchIn import com.artemchep.keyguard.common.model.MasterSession -import com.artemchep.keyguard.common.model.RemoveAttachmentRequest import com.artemchep.keyguard.common.service.export.ExportManager import com.artemchep.keyguard.common.usecase.GetVaultSession -import com.artemchep.keyguard.common.usecase.RemoveAttachment import com.artemchep.keyguard.common.usecase.WindowCoroutineScope import org.kodein.di.android.closestDI import org.kodein.di.direct @@ -66,16 +62,7 @@ class VaultExportActionReceiver : BroadcastReceiver() { ?: return@run null s.di.direct.instance() } ?: return - - // TODO: - val removeIo = kotlin.run { - val request = RemoveAttachmentRequest.ByDownloadId( - downloadId = exportId, - ) - val removeAttachment: RemoveAttachment by di.instance() - removeAttachment(listOf(request)) - } - removeIo.launchIn(windowCoroutineScope) + exportManager.cancel(exportId) } } } diff --git a/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/worker/ExportWorker.kt b/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/worker/ExportWorker.kt index 5ea918b..41d5c5d 100644 --- a/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/worker/ExportWorker.kt +++ b/common/src/androidMain/kotlin/com/artemchep/keyguard/android/downloader/worker/ExportWorker.kt @@ -19,18 +19,19 @@ import androidx.work.WorkerParameters import com.artemchep.keyguard.android.Notifications import com.artemchep.keyguard.android.downloader.receiver.VaultExportActionReceiver import com.artemchep.keyguard.common.R -import com.artemchep.keyguard.common.io.attempt -import com.artemchep.keyguard.common.io.bind -import com.artemchep.keyguard.common.io.timeout -import com.artemchep.keyguard.common.io.toIO import com.artemchep.keyguard.common.model.MasterSession import com.artemchep.keyguard.common.service.download.DownloadProgress import com.artemchep.keyguard.common.service.export.ExportManager import com.artemchep.keyguard.common.usecase.GetVaultSession import com.artemchep.keyguard.feature.filepicker.humanReadableByteCountSI +import com.artemchep.keyguard.feature.loading.getErrorReadableMessage +import com.artemchep.keyguard.feature.navigation.state.TranslatorScope +import com.artemchep.keyguard.platform.LeContext import com.artemchep.keyguard.res.Res import com.artemchep.keyguard.res.* -import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flatMapConcat +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.last import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.onStart @@ -90,6 +91,11 @@ class ExportWorker( override val di by closestDI { applicationContext } + private val translator by lazy { + val ctx = LeContext(applicationContext) + TranslatorScope.of(ctx) + } + private val notificationManager = context.getSystemService()!! private var notificationId = Notifications.export.obtainId() @@ -102,6 +108,7 @@ class ExportWorker( ) } + @OptIn(ExperimentalCoroutinesApi::class) private suspend fun internalDoWork( notificationId: Int, args: Args, @@ -111,23 +118,17 @@ class ExportWorker( ?: return Result.success() val exportManager: ExportManager by s.di.instance() - val exportStatusFlow = exportManager - .statusByExportId(exportId = args.exportId) - kotlin.run { - // ...check if the status is other then None. - val result = exportStatusFlow - .filter { it !is DownloadProgress.None } - .toIO() - .timeout(500L) - .attempt() - .bind() - if (result.isLeft()) { - return Result.success() + val exportProgressFlow = exportManager + .getProgressFlowByExportId(exportId = args.exportId) + // Return None if the progress flow doesn't + // exist anymore. Notice how we use the concat here, + // this is intended. + .flatMapConcat { flow -> + flow ?: flowOf(DownloadProgress.None) } - } val title = applicationContext.getString(R.string.notification_vault_export_title) - val result = exportStatusFlow + val result = exportProgressFlow .onStart { val foregroundInfo = createForegroundInfo( id = notificationId, @@ -140,13 +141,7 @@ class ExportWorker( .onEach { progress -> when (progress) { is DownloadProgress.None -> { - val foregroundInfo = createForegroundInfo( - id = notificationId, - exportId = args.exportId, - name = title, - progress = null, - ) - setForeground(foregroundInfo) + // Do nothing } is DownloadProgress.Loading -> { @@ -169,22 +164,30 @@ class ExportWorker( is DownloadProgress.Complete -> { // Do nothing - return@onEach } } } // complete once we finish the download .transformWhile { progress -> emit(progress) // always emit progress - progress !is DownloadProgress.Complete + progress !is DownloadProgress.Complete && + progress !is DownloadProgress.None } .last() + // None means that the progress flow doesn't exist + // anymore. This is most likely to happen because + // someone has cancelled the export. + if (result is DownloadProgress.None) { + return Result.success() + } + require(result is DownloadProgress.Complete) // Send a complete notification. result.result.fold( ifLeft = { e -> sendFailureNotification( exportId = args.exportId, + reason = e, ) }, ifRight = { @@ -195,7 +198,7 @@ class ExportWorker( ) return result.result .fold( - ifLeft = { e -> + ifLeft = { // We don't want to automatically retry exporting a // vault, just notify a user and bail out. Result.success() @@ -214,10 +217,13 @@ class ExportWorker( private suspend fun sendFailureNotification( exportId: String, + reason: Throwable, ) = sendCompleteNotification(exportId) { builder -> val name = getString(Res.string.exportaccount_export_failure) + val message = getErrorReadableMessage(reason, translator) builder .setContentTitle(name) + .setContentText(message.text ?: message.title) .setTicker(name) .setSmallIcon(android.R.drawable.stat_sys_warning) } diff --git a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/ExportManager.kt b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/ExportManager.kt index e502322..a0419f4 100644 --- a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/ExportManager.kt +++ b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/ExportManager.kt @@ -5,7 +5,13 @@ import com.artemchep.keyguard.common.service.export.model.ExportRequest import kotlinx.coroutines.flow.Flow interface ExportManager { - fun statusByExportId(exportId: String): Flow + /** + * Returns currently a progress flow for a given [exportId], + * returns `null` if the export is not active. + */ + fun getProgressFlowByExportId(exportId: String): Flow?> + + fun cancel(exportId: String) class QueueResult( val exportId: String, diff --git a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/impl/ExportManagerImpl.kt b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/impl/ExportManagerImpl.kt index 8732d7f..52573c4 100644 --- a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/impl/ExportManagerImpl.kt +++ b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/service/export/impl/ExportManagerImpl.kt @@ -36,6 +36,7 @@ import kotlinx.collections.immutable.persistentMapOf import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.channels.ProducerScope import kotlinx.coroutines.coroutineScope @@ -44,12 +45,9 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.channelFlow import kotlinx.coroutines.flow.collect -import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.emitAll import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flow -import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.last import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach @@ -96,8 +94,6 @@ open class ExportManagerBase( private val mutex = Mutex() - private val flowOfNone = flowOf(DownloadProgress.None) - constructor( directDI: DirectDI, onLaunch: ExportManager.(String) -> Unit, @@ -119,18 +115,19 @@ open class ExportManagerBase( onLaunch = onLaunch, ) - private fun fileStatusBy(predicate: (PoolEntry) -> Boolean) = sink - .map { state -> - val entryOrNull = state.values.firstOrNull(predicate) - entryOrNull?.flow - ?: flowOfNone - } - .distinctUntilChanged() - .flatMapLatest { it } - - override fun statusByExportId( + override fun getProgressFlowByExportId( exportId: String, - ): Flow = fileStatusBy { it.id == exportId } + ): Flow?> = sink + .map { state -> + state.values + .firstOrNull { it.id == exportId }?.flow + } + + override fun cancel(exportId: String) { + val entry = sink.value.values + .firstOrNull { it.id == exportId } + entry?.scope?.cancel() + } override suspend fun queue( request: ExportRequest, diff --git a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/usecase/impl/DownloadAttachmentMetadataImpl.kt b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/usecase/impl/DownloadAttachmentMetadataImpl.kt index ea8107e..30c77c6 100644 --- a/common/src/commonMain/kotlin/com/artemchep/keyguard/common/usecase/impl/DownloadAttachmentMetadataImpl.kt +++ b/common/src/commonMain/kotlin/com/artemchep/keyguard/common/usecase/impl/DownloadAttachmentMetadataImpl.kt @@ -33,6 +33,7 @@ import io.ktor.client.HttpClient import kotlinx.serialization.json.Json import org.kodein.di.DirectDI import org.kodein.di.instance +import java.net.UnknownHostException /** * @author Artem Chepurnyi @@ -192,6 +193,10 @@ class DownloadAttachmentMetadataImpl2( encryptionKey = base64Service.decode(model.keyBase64), ) }.getOrElse { + // TODO: Throw properly! + if (it is UnknownHostException) { + throw it + } it.printStackTrace() AttachmentData( url = requireNotNull(attachment.url), diff --git a/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScope.kt b/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScope.kt index e831460..d07cf66 100644 --- a/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScope.kt +++ b/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScope.kt @@ -4,6 +4,7 @@ import androidx.compose.material3.ColorScheme import com.artemchep.keyguard.common.model.ToastMessage import com.artemchep.keyguard.feature.loading.LoadingTask import com.artemchep.keyguard.feature.localization.TextHolder +import com.artemchep.keyguard.feature.localization.textResource import com.artemchep.keyguard.feature.navigation.NavigationIntent import com.artemchep.keyguard.platform.LeContext import org.jetbrains.compose.resources.StringResource @@ -14,6 +15,10 @@ import kotlinx.coroutines.flow.shareIn import org.jetbrains.compose.resources.PluralStringResource interface TranslatorScope { + companion object { + fun of(context: LeContext) = TranslatorScopeContext(context) + } + suspend fun translate( res: StringResource, ): String @@ -30,6 +35,22 @@ interface TranslatorScope { ): String } +class TranslatorScopeContext( + private val context: LeContext, +) : TranslatorScope { + override suspend fun translate(res: StringResource): String = + textResource(res, context) + + override suspend fun translate(res: StringResource, vararg args: Any): String = + textResource(res, context, *args) + + override suspend fun translate( + res: PluralStringResource, + quantity: Int, + vararg args: Any, + ): String = textResource(res, context, quantity, *args) +} + suspend fun TranslatorScope.translate(text: TextHolder) = when (text) { is TextHolder.Res -> translate(text.data) is TextHolder.Value -> text.data diff --git a/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScopeImpl.kt b/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScopeImpl.kt index bbdc4ae..20f12b2 100644 --- a/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScopeImpl.kt +++ b/common/src/commonMain/kotlin/com/artemchep/keyguard/feature/navigation/state/RememberStateFlowScopeImpl.kt @@ -58,7 +58,9 @@ class RememberStateFlowScopeImpl( private val colorSchemeState: State, override val screenName: String, override val context: LeContext, -) : RememberStateFlowScopeZygote, CoroutineScope by scope { +) : RememberStateFlowScopeZygote, + TranslatorScope by TranslatorScope.of(context), + CoroutineScope by scope { private val registry = mutableMapOf>() override val colorScheme get() = colorSchemeState.value @@ -149,18 +151,6 @@ class RememberStateFlowScopeImpl( } } - override suspend fun translate(res: StringResource): String = - textResource(res, context) - - override suspend fun translate(res: StringResource, vararg args: Any): String = - textResource(res, context, *args) - - override suspend fun translate( - res: PluralStringResource, - quantity: Int, - vararg args: Any, - ): String = textResource(res, context, quantity, *args) - override fun screenExecutor(): LoadingTask { val executor = LoadingTask(this, appScope) executor diff --git a/common/src/desktopMain/kotlin/com/artemchep/keyguard/copy/ExportManagerImpl.kt b/common/src/desktopMain/kotlin/com/artemchep/keyguard/copy/ExportManagerImpl.kt index 6146db7..0ca9167 100644 --- a/common/src/desktopMain/kotlin/com/artemchep/keyguard/copy/ExportManagerImpl.kt +++ b/common/src/desktopMain/kotlin/com/artemchep/keyguard/copy/ExportManagerImpl.kt @@ -1,25 +1,26 @@ package com.artemchep.keyguard.copy -import com.artemchep.keyguard.common.io.attempt -import com.artemchep.keyguard.common.io.bind -import com.artemchep.keyguard.common.io.timeout -import com.artemchep.keyguard.common.io.toIO import com.artemchep.keyguard.common.model.ToastMessage import com.artemchep.keyguard.common.service.download.DownloadProgress import com.artemchep.keyguard.common.service.export.impl.ExportManagerBase import com.artemchep.keyguard.common.usecase.ShowMessage +import com.artemchep.keyguard.feature.loading.getErrorReadableMessage import com.artemchep.keyguard.feature.localization.textResource +import com.artemchep.keyguard.feature.navigation.state.TranslatorScope import com.artemchep.keyguard.platform.LeContext import com.artemchep.keyguard.res.Res import com.artemchep.keyguard.res.* +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.GlobalScope -import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.flatMapConcat +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.last import kotlinx.coroutines.flow.transformWhile import kotlinx.coroutines.launch import org.kodein.di.DirectDI import org.kodein.di.instance +@OptIn(ExperimentalCoroutinesApi::class) class ExportManagerImpl( private val directDI: DirectDI, private val showMessage: ShowMessage, @@ -28,32 +29,36 @@ class ExportManagerImpl( directDI = directDI, onLaunch = { exportId -> GlobalScope.launch { - val exportStatusFlow = statusByExportId(exportId = exportId) - kotlin.run { - // ...check if the status is other then None. - val result = exportStatusFlow - .filter { it !is DownloadProgress.None } - .toIO() - .timeout(500L) - .attempt() - .bind() - if (result.isLeft()) { - return@launch + val exportStatusFlow = getProgressFlowByExportId(exportId = exportId) + // Return None if the progress flow doesn't + // exist anymore. Notice how we use the concat here, + // this is intended. + .flatMapConcat { flow -> + flow ?: flowOf(DownloadProgress.None) } - } val result = exportStatusFlow // complete once we finish the download .transformWhile { progress -> emit(progress) // always emit progress - progress !is DownloadProgress.Complete + progress !is DownloadProgress.Complete && + progress !is DownloadProgress.None } .last() + // None means that the progress flow doesn't exist + // anymore. This is most likely to happen because + // someone has cancelled the export. + if (result is DownloadProgress.None) { + return@launch + } + require(result is DownloadProgress.Complete) result.result.fold( - ifLeft = { + ifLeft = { e -> + val translator = TranslatorScope.of(context) val message = ToastMessage( title = textResource(Res.string.exportaccount_export_failure, context), + text = getErrorReadableMessage(e, translator).run { text ?: title }, type = ToastMessage.Type.ERROR, ) showMessage.copy(message)