From c8d9269d132c6bcf8686c36d51085ab056674899 Mon Sep 17 00:00:00 2001 From: Nicolas Bourdin Date: Fri, 22 Nov 2024 15:43:43 +0100 Subject: [PATCH] refractor: Apply suggestion from code review --- .../infomaniak/mail/data/api/ApiRepository.kt | 2 +- .../com/infomaniak/mail/ui/MainViewModel.kt | 9 ++++--- .../DownloadAttachmentProgressDialog.kt | 14 ++++++++-- .../actions/DownloadMessagesProgressDialog.kt | 15 ++++++----- .../actions/DownloadMessagesViewModel.kt | 26 ++++++++++++------- .../thread/actions/DownloadProgressDialog.kt | 14 +++------- .../MessageActionsBottomSheetDialog.kt | 6 ++--- .../actions/MultiSelectBottomSheetDialog.kt | 20 ++++++++++---- .../actions/ThreadActionsBottomSheetDialog.kt | 2 +- .../{KDriveUtils.kt => SaveOnKDriveUtils.kt} | 2 +- .../utils/extensions/AttachmentExtensions.kt | 6 ++--- .../utils/extensions/NavigationExtensions.kt | 4 +-- .../res/layout/dialog_download_progress.xml | 1 + .../main/res/navigation/main_navigation.xml | 3 +-- 14 files changed, 73 insertions(+), 51 deletions(-) rename app/src/main/java/com/infomaniak/mail/utils/{KDriveUtils.kt => SaveOnKDriveUtils.kt} (98%) diff --git a/app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt b/app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt index f48356e309..c0385bb7ed 100644 --- a/app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt +++ b/app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt @@ -437,7 +437,7 @@ object ApiRepository : ApiRepositoryCore() { fun getDownloadedMessage(mailboxUuid: String, folderId: String, shortUid: Int): Response { val request = Request.Builder().url(ApiRoutes.downloadMessage(mailboxUuid, folderId, shortUid)) - .headers(HttpUtils.getHeaders(null)) + .headers(HttpUtils.getHeaders()) .get() .build() diff --git a/app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt b/app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt index 1c9cebcfa0..283dd33a43 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt @@ -1195,17 +1195,18 @@ class MainViewModel @Inject constructor( } } - fun getMessagesUidsFromThreadUids(selectedThreadsUuids: List): List { + fun getMessagesUidsFromThreadUids(selectedThreadsUids: List): List { val messageUids = mutableListOf() - selectedThreadsUuids.forEach { threadUuid -> + selectedThreadsUids.forEach { threadUuid -> val thread = threadController.getThread(threadUuid) ?: return@forEach messageUids.addAll(thread.messages.map { it.uid }) } + return messageUids } - fun getSubject(threadUuid: String): String? { - return threadController.getThread(threadUuid)?.subject + fun getSubject(threadUid: String): String { + return threadController.getThread(threadUid)?.subject ?: "" } companion object { diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadAttachmentProgressDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadAttachmentProgressDialog.kt index f34d0744fa..1c0e40f487 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadAttachmentProgressDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadAttachmentProgressDialog.kt @@ -17,6 +17,12 @@ */ package com.infomaniak.mail.ui.main.thread.actions +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.appcompat.content.res.AppCompatResources +import androidx.core.view.isVisible import androidx.fragment.app.viewModels import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs @@ -25,13 +31,17 @@ import com.infomaniak.mail.utils.extensions.AttachmentExtensions import com.infomaniak.mail.utils.extensions.AttachmentExtensions.getIntentOrGoToPlayStore import dagger.hilt.android.AndroidEntryPoint -@AndroidEntryPoint class DownloadAttachmentProgressDialog : DownloadProgressDialog() { private val navigationArgs: DownloadAttachmentProgressDialogArgs by navArgs() private val downloadAttachmentViewModel: DownloadAttachmentViewModel by viewModels() override val dialogTitle: String? by lazy { navigationArgs.attachmentName } - override val dialogIconDrawableRes: Int? by lazy { navigationArgs.attachmentType.icon } + + override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? { + binding.icon.isVisible = true + binding.icon.setImageDrawable(AppCompatResources.getDrawable(requireContext(), navigationArgs.attachmentType.icon)) + return super.onCreateView(inflater, container, savedInstanceState) + } override fun download() { downloadAttachmentViewModel.downloadAttachment().observe(this) { cachedAttachment -> diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesProgressDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesProgressDialog.kt index 30f0ad574f..de5068dd36 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesProgressDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesProgressDialog.kt @@ -27,17 +27,14 @@ import androidx.navigation.fragment.navArgs import com.infomaniak.lib.core.utils.goToPlayStore import com.infomaniak.lib.core.utils.setBackNavigationResult import com.infomaniak.mail.R -import com.infomaniak.mail.utils.KDriveUtils.DRIVE_PACKAGE -import com.infomaniak.mail.utils.KDriveUtils.SAVE_EXTERNAL_ACTIVITY_CLASS -import com.infomaniak.mail.utils.KDriveUtils.canSaveOnKDrive -import dagger.hilt.android.AndroidEntryPoint +import com.infomaniak.mail.utils.SaveOnKDriveUtils.DRIVE_PACKAGE +import com.infomaniak.mail.utils.SaveOnKDriveUtils.SAVE_EXTERNAL_ACTIVITY_CLASS +import com.infomaniak.mail.utils.SaveOnKDriveUtils.canSaveOnKDrive -@AndroidEntryPoint class DownloadMessagesProgressDialog : DownloadProgressDialog() { private val downloadThreadsViewModel: DownloadMessagesViewModel by viewModels() private val navigationArgs: DownloadMessagesProgressDialogArgs by navArgs() override val dialogTitle: String? by lazy { getDialogTitleFromArgs() } - override val dialogIconDrawableRes: Int? by lazy { null } override fun download() { downloadThreadsViewModel.downloadThreads(mainViewModel.currentMailbox.value).observe(this) { threadUris -> @@ -54,7 +51,11 @@ class DownloadMessagesProgressDialog : DownloadProgressDialog() { private fun getDialogTitleFromArgs() = if (navigationArgs.messageUids.size == 1) { navigationArgs.nameFirstMessage } else { - requireContext().resources.getQuantityString(R.plurals.downloadingEmailsTitle, 1, navigationArgs.messageUids.size) + requireContext().resources.getQuantityString( + R.plurals.downloadingEmailsTitle, + navigationArgs.messageUids.size, + navigationArgs.messageUids.size + ) } private fun ArrayList.openKDriveOrPlayStore(context: Context): Intent? { diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesViewModel.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesViewModel.kt index b87e6c8a96..9152a93e45 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesViewModel.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadMessagesViewModel.kt @@ -26,6 +26,7 @@ import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.liveData import androidx.lifecycle.viewModelScope import com.infomaniak.lib.core.utils.DownloadManagerUtils +import com.infomaniak.mail.R import com.infomaniak.mail.data.api.ApiRepository import com.infomaniak.mail.data.cache.mailboxContent.MessageController import com.infomaniak.mail.data.models.mailbox.Mailbox @@ -47,23 +48,28 @@ class DownloadMessagesViewModel @Inject constructor( private val ioCoroutineContext = viewModelScope.coroutineContext(ioDispatcher) - private val messageLocalUids - inline get() = savedStateHandle.get>(DownloadMessagesProgressDialogArgs::messageUids.name)!! + private val messageLocalUids: Array + inline get() = savedStateHandle[DownloadMessagesProgressDialogArgs::messageUids.name]!! fun downloadThreads(currentMailbox: Mailbox?) = liveData(ioCoroutineContext) { val downloadedThreadUris: List? = runCatching { val mailbox = currentMailbox ?: return@runCatching null val listUri = mutableListOf() - val listFileName = mutableSetOf().also { it.addAll(getAllFileNameInExportEmlDir(appContext)) } + val listFileName = getAllFileNameInExportEmlDir(appContext).toMutableSet() messageLocalUids.forEach { messageUid -> val message = messageController.getMessage(messageUid) ?: return@runCatching null - val response = ApiRepository.getDownloadedMessage(mailbox.uuid, message.folderId, message.shortUid) + val response = ApiRepository.getDownloadedMessage( + mailboxUuid = mailbox.uuid, + folderId = message.folderId, + shortUid = message.shortUid, + contentType = EML_CONTENT_TYPE, + ) if (!response.isSuccessful || response.body == null) return@runCatching null val messageSubject: String = message.subject?.removeIllegalFileNameCharacter() ?: NO_SUBJECT_FILE - createOriginalFileName(messageSubject, listFileName.toList()).let { fileName -> + createUniqueFileName(messageSubject, listFileName.toList()).let { fileName -> listFileName.add(fileName) saveEmlToFile(appContext, response.body!!.bytes(), fileName)?.let { listUri.add(it) } } @@ -75,7 +81,7 @@ class DownloadMessagesViewModel @Inject constructor( } // TODO Extract this code in core2 - private fun createOriginalFileName(originalFileName: String, listFileName: List): String { + private fun createUniqueFileName(originalFileName: String, listFileName: List): String { var postfix = 1 var fileName = originalFileName @@ -85,21 +91,21 @@ class DownloadMessagesViewModel @Inject constructor( } private fun getAllFileNameInExportEmlDir(context: Context): List { - val fileDir = File(context.cacheDir, context.getString(com.infomaniak.mail.R.string.EXPOSED_EML_PATH)) + val fileDir = File(context.cacheDir, context.getString(R.string.EXPOSED_EML_PATH)) if (!fileDir.exists()) fileDir.mkdirs() return fileDir.listFiles()?.map { it.name.removeSuffix(".eml") } ?: emptyList() } private fun saveEmlToFile(context: Context, emlByteArray: ByteArray, fileName: String): Uri? { val fileNameWithExtension = "${fileName.removeIllegalFileNameCharacter()}.eml" - val fileDir = File(context.cacheDir, context.getString(com.infomaniak.mail.R.string.EXPOSED_EML_PATH)) + val fileDir = File(context.cacheDir, context.getString(R.string.EXPOSED_EML_PATH)) if (!fileDir.exists()) fileDir.mkdirs() runCatching { val file = File(fileDir, fileNameWithExtension) file.outputStream().use { it.write(emlByteArray) } - return FileProvider.getUriForFile(context, context.getString(com.infomaniak.mail.R.string.EML_AUTHORITY), file) + return FileProvider.getUriForFile(context, context.getString(R.string.EML_AUTHORITY), file) }.onFailure { exception -> exception.printStackTrace() } @@ -109,7 +115,7 @@ class DownloadMessagesViewModel @Inject constructor( private fun String.removeIllegalFileNameCharacter(): String = this.replace(DownloadManagerUtils.regexInvalidSystemChar, "") companion object { - private const val EML_CONTENT_TYPE = "message/rfc822" private const val NO_SUBJECT_FILE = "message" + private const val EML_CONTENT_TYPE = "message/rfc822" } } diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadProgressDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadProgressDialog.kt index 36880ea3a5..7334080963 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadProgressDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/DownloadProgressDialog.kt @@ -20,9 +20,6 @@ package com.infomaniak.mail.ui.main.thread.actions import android.app.Dialog import android.os.Bundle import android.view.KeyEvent -import androidx.annotation.DrawableRes -import androidx.appcompat.content.res.AppCompatResources -import androidx.core.view.isVisible import androidx.fragment.app.DialogFragment import androidx.fragment.app.activityViewModels import androidx.lifecycle.lifecycleScope @@ -38,10 +35,13 @@ import kotlinx.coroutines.launch @AndroidEntryPoint abstract class DownloadProgressDialog : DialogFragment() { + protected val binding by lazy { DialogDownloadProgressBinding.inflate(layoutInflater) } protected val mainViewModel: MainViewModel by activityViewModels() + abstract val dialogTitle: String? - @get:DrawableRes abstract val dialogIconDrawableRes: Int? + + protected abstract fun download() override fun onStart() { download() @@ -50,10 +50,6 @@ abstract class DownloadProgressDialog : DialogFragment() { override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { val context = requireContext() - - dialogIconDrawableRes?.let { binding.icon.setImageDrawable(AppCompatResources.getDrawable(requireContext(), it)) } - binding.icon.isVisible = dialogIconDrawableRes == null - isCancelable = false return MaterialAlertDialogBuilder(context) @@ -68,8 +64,6 @@ abstract class DownloadProgressDialog : DialogFragment() { .create() } - abstract fun download() - protected fun popBackStackWithError() { lifecycleScope.launch { mainViewModel.isNetworkAvailable.first { it != null }?.let { isNetworkAvailable -> diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MessageActionsBottomSheetDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MessageActionsBottomSheetDialog.kt index ba3eb5a302..8525235575 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MessageActionsBottomSheetDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MessageActionsBottomSheetDialog.kt @@ -178,9 +178,9 @@ class MessageActionsBottomSheetDialog : MailActionsBottomSheetDialog() { override fun onSaveKDrive() { trackBottomSheetThreadActionsEvent(ACTION_SAVE_KDRIVE_NAME) navigateToDownloadMessagesProgressDialog( - listOf(messageUid), - mainViewModel.getSubject(threadUid), - MessageActionsBottomSheetDialog::class.java.name + messageUids = listOf(messageUid), + nameFirstMessage = mainViewModel.getSubject(threadUid), + currentClassName = MessageActionsBottomSheetDialog::class.java.name, ) } diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MultiSelectBottomSheetDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MultiSelectBottomSheetDialog.kt index 5c2eb916ab..089c687d64 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MultiSelectBottomSheetDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MultiSelectBottomSheetDialog.kt @@ -22,6 +22,7 @@ import android.view.LayoutInflater import android.view.View import android.view.ViewGroup import androidx.fragment.app.activityViewModels +import com.infomaniak.lib.core.utils.SentryLog import com.infomaniak.lib.core.utils.safeBinding import com.infomaniak.mail.MatomoMail.ACTION_ARCHIVE_NAME import com.infomaniak.mail.MatomoMail.ACTION_DELETE_NAME @@ -118,13 +119,18 @@ class MultiSelectBottomSheetDialog : ActionsBottomSheetDialog() { toggleThreadsFavoriteStatus(selectedThreadsUids, shouldFavorite) isMultiSelectOn = false } + binding.saveKDrive.setClosingOnClickListener(shouldCloseMultiSelection = true) { trackMultiSelectActionEvent(ACTION_SAVE_KDRIVE_NAME, selectedThreadsCount, isFromBottomSheet = true) - navigateToDownloadMessagesProgressDialog( - messageUids = mainViewModel.getMessagesUidsFromThreadUids(selectedThreadsUids), - nameFirstMessage = selectedThreads.firstOrNull()?.subject, - MultiSelectBottomSheetDialog::class.java.name, - ) + runCatching { + navigateToDownloadMessagesProgressDialog( + messageUids = mainViewModel.getMessagesUidsFromThreadUids(selectedThreadsUids), + nameFirstMessage = mainViewModel.getSubject(selectedThreadsUids.first()), + currentClassName = MultiSelectBottomSheetDialog::class.java.name, + ) + }.onFailure { + SentryLog.e(TAG, "SelectedThreadUids is empty, it should not happened") + } isMultiSelectOn = false } } @@ -154,4 +160,8 @@ class MultiSelectBottomSheetDialog : ActionsBottomSheetDialog() { private fun getSpamIconAndText(isFromSpam: Boolean): Pair { return if (isFromSpam) R.drawable.ic_non_spam to R.string.actionNonSpam else R.drawable.ic_spam to R.string.actionSpam } + + companion object { + private val TAG = this::class.java.simpleName + } } diff --git a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ThreadActionsBottomSheetDialog.kt b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ThreadActionsBottomSheetDialog.kt index 1f2bd418d5..b94c8e9657 100644 --- a/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ThreadActionsBottomSheetDialog.kt +++ b/app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ThreadActionsBottomSheetDialog.kt @@ -200,7 +200,7 @@ class ThreadActionsBottomSheetDialog : MailActionsBottomSheetDialog() { trackBottomSheetThreadActionsEvent(ACTION_SAVE_KDRIVE_NAME) navigateToDownloadMessagesProgressDialog( messageUids = thread.messages.map { it.uid }, - nameFirstMessage = thread.subject, + nameFirstMessage = thread.subject ?: "", ThreadActionsBottomSheetDialog::class.java.name ) } diff --git a/app/src/main/java/com/infomaniak/mail/utils/KDriveUtils.kt b/app/src/main/java/com/infomaniak/mail/utils/SaveOnKDriveUtils.kt similarity index 98% rename from app/src/main/java/com/infomaniak/mail/utils/KDriveUtils.kt rename to app/src/main/java/com/infomaniak/mail/utils/SaveOnKDriveUtils.kt index 98029ef026..095b09e1cb 100644 --- a/app/src/main/java/com/infomaniak/mail/utils/KDriveUtils.kt +++ b/app/src/main/java/com/infomaniak/mail/utils/SaveOnKDriveUtils.kt @@ -21,7 +21,7 @@ import android.content.Context import android.content.pm.PackageManager import io.sentry.Sentry -object KDriveUtils { +object SaveOnKDriveUtils { const val DRIVE_PACKAGE = "com.infomaniak.drive" const val SAVE_EXTERNAL_ACTIVITY_CLASS = "com.infomaniak.drive.ui.SaveExternalFilesActivity" diff --git a/app/src/main/java/com/infomaniak/mail/utils/extensions/AttachmentExtensions.kt b/app/src/main/java/com/infomaniak/mail/utils/extensions/AttachmentExtensions.kt index f282c86331..896cdfb6c4 100644 --- a/app/src/main/java/com/infomaniak/mail/utils/extensions/AttachmentExtensions.kt +++ b/app/src/main/java/com/infomaniak/mail/utils/extensions/AttachmentExtensions.kt @@ -37,9 +37,9 @@ import com.infomaniak.mail.data.models.mailbox.Mailbox import com.infomaniak.mail.ui.main.SnackbarManager import com.infomaniak.mail.ui.main.thread.actions.DownloadAttachmentProgressDialogArgs import com.infomaniak.mail.utils.AccountUtils -import com.infomaniak.mail.utils.KDriveUtils.DRIVE_PACKAGE -import com.infomaniak.mail.utils.KDriveUtils.SAVE_EXTERNAL_ACTIVITY_CLASS -import com.infomaniak.mail.utils.KDriveUtils.canSaveOnKDrive +import com.infomaniak.mail.utils.SaveOnKDriveUtils.DRIVE_PACKAGE +import com.infomaniak.mail.utils.SaveOnKDriveUtils.SAVE_EXTERNAL_ACTIVITY_CLASS +import com.infomaniak.mail.utils.SaveOnKDriveUtils.canSaveOnKDrive import com.infomaniak.mail.utils.SentryDebug import com.infomaniak.mail.utils.WorkerUtils.UploadMissingLocalFileException import com.infomaniak.mail.utils.extensions.AttachmentExtensions.AttachmentIntentType.OPEN_WITH diff --git a/app/src/main/java/com/infomaniak/mail/utils/extensions/NavigationExtensions.kt b/app/src/main/java/com/infomaniak/mail/utils/extensions/NavigationExtensions.kt index 95659dd45b..6f84e84da9 100644 --- a/app/src/main/java/com/infomaniak/mail/utils/extensions/NavigationExtensions.kt +++ b/app/src/main/java/com/infomaniak/mail/utils/extensions/NavigationExtensions.kt @@ -93,8 +93,8 @@ fun Fragment.navigateToDownloadProgressDialog( fun Fragment.navigateToDownloadMessagesProgressDialog( messageUids: List, - nameFirstMessage: String?, - currentClassName: String + nameFirstMessage: String, + currentClassName: String, ) { safeNavigate( resId = R.id.downloadMessagesProgressDialog, diff --git a/app/src/main/res/layout/dialog_download_progress.xml b/app/src/main/res/layout/dialog_download_progress.xml index a6e6ebcc20..d496f61e0e 100644 --- a/app/src/main/res/layout/dialog_download_progress.xml +++ b/app/src/main/res/layout/dialog_download_progress.xml @@ -29,6 +29,7 @@ android:layout_marginStart="@dimen/marginStandardMedium" android:importantForAccessibility="no" android:src="@drawable/ic_file_unknown" + android:visibility="gone" app:layout_constraintBottom_toBottomOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toTopOf="parent" /> diff --git a/app/src/main/res/navigation/main_navigation.xml b/app/src/main/res/navigation/main_navigation.xml index a8a76ef0cf..83445dd994 100644 --- a/app/src/main/res/navigation/main_navigation.xml +++ b/app/src/main/res/navigation/main_navigation.xml @@ -583,8 +583,7 @@ app:argType="string[]" /> + app:argType="string" />