feat: Remove junk menu duplicate #2770
Conversation
solrubado
force-pushed
the
remove-junk-menu-duplicate
branch
from
ace22c9 to
07985ea
Compare
app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MailActionsBottomSheetDialog.kt
Outdated
Show resolved
Hide resolved
solrubado
force-pushed
the
remove-junk-menu-duplicate
branch
2 times, most recently
from
e19c156 to
246d695
Compare
LunarX
left a comment
LunarX
left a comment
There was a problem hiding this comment.
Somes first change requests
| fun hideReportJunkButtons() = with(binding) { | ||
| spam.isGone = true | ||
| phishing.isGone = true | ||
| blockSender.isGone = true | ||
| } |
There was a problem hiding this comment.
If this method is only used by MessageActionsBottomSheetDialog, can probably keep it private and define it inside the only class that uses it
| fun onSpam() = Unit | ||
| fun onPhishing() = Unit | ||
| fun onBlockSender() = Unit |
There was a problem hiding this comment.
No need for default implementations here
| val (text, icon) = if (isFromSpam) { | ||
| R.string.actionNonSpam to R.drawable.ic_non_spam | ||
| } else { | ||
| R.string.actionSpam to R.drawable.ic_report_junk |
There was a problem hiding this comment.
The icon is not correct. You are using the report junk icon instead of the spam icon. Also if the report_junk icon is not used anymore in the app we can remove it altogether
| val (text, icon) = if (isFromSpam) { | ||
| R.string.actionNonSpam to R.drawable.ic_non_spam | ||
| } else { | ||
| R.string.actionSpam to R.drawable.ic_report_junk | ||
| } |
There was a problem hiding this comment.
I see we have to setup the ui dynamically for all three junk items in two different places. We can extract a setJunkUi method and reuse it in both places
| } | ||
|
|
||
| phishing.isVisible = !isFromSpam | ||
| blockSender.isVisible = !isFromSpam |
There was a problem hiding this comment.
There used to be other conditions to hide/show the block sender. For exemple the option is not visible when the only email address that can be blocked is yourself. Now you show this button regardless of this condition
| blockSender.isVisible = !isFromSpam | ||
| } | ||
|
|
||
| private fun observeReportPhishingResult() { | ||
| mainViewModel.reportPhishingTrigger.observe(viewLifecycleOwner) { | ||
| descriptionDialog.resetLoadingAndDismiss() | ||
| findNavController().popBackStack() | ||
| } | ||
| } | ||
|
|
||
| private fun setBlockUserUi(potentialUsersToBlock: Map<Recipient, Message>) { | ||
| if (potentialUsersToBlock.count() == 0) binding.blockSender.isGone = true | ||
| } |
There was a problem hiding this comment.
Here you compute the blockSender visibility in two different spots which is error prone and could easily lead to hidden bugs when refactoring the code or moving the code around.
Also the 1st spot computes a condition A and the 2nd spot computes a condition B but the 1st spot doesn't care about condition B and the 2nd spot doesn't care about condition A. So only one of the two condition is effective
| // Check the first message, because it is not possible to select messages from multiple folders, | ||
| // so you won't have both SPAM and non-SPAM messages. | ||
| trackBottomSheetThreadActionsEvent( | ||
| MatomoName.Spam, | ||
| value = thread.messages.firstOrNull()?.folder?.role == FolderRole.SPAM |
There was a problem hiding this comment.
Can the condition for the matomo event be hardcoded to false here or is it different?
|
|
||
| descriptionDialog.show( | ||
| title = getString(R.string.reportPhishingTitle), | ||
| description = resources.getQuantityString(R.plurals.reportPhishingDescription, threadsCount), |
There was a problem hiding this comment.
I see that the quantity to select the singular or plural form of the string used to be computed based on the number of messages. Here you've changed it to the number of threads which is not the same. If you have a single thread with 2 messages, the string should be in plural to match the previous logic
| onPositiveButtonClicked = { | ||
| mainViewModel.reportPhishing( | ||
| threadsUids, | ||
| messages | ||
| ) | ||
| }, |
There was a problem hiding this comment.
This is more of a detail on the style of writing but we usually tend to one-line lines like those. If you look at what was there previously it's actually how it used to be written.
| onPositiveButtonClicked = { | |
| mainViewModel.reportPhishing( | |
| threadsUids, | |
| messages | |
| ) | |
| }, | |
| onPositiveButtonClicked = { mainViewModel.reportPhishing(threadsUids, messages) }, |
|
|
||
|
|
There was a problem hiding this comment.
Here we usually avoid double new lines in this situation
| --> | ||
| <resources> | ||
| <string name="errorPotentialUsersToBlockNull" translatable="false">Potential users to block is null</string> | ||
| <string name="errorPhishingMessagesNull" translatable="false">Phishing messages report failed because the list is empty</string> |
There was a problem hiding this comment.
Could we maybe name all sentry related strings by prefixing them with sentry... to better group them and distinguish them from other non-sentry string that have translatable="false" ?
|
|
||
| if (messages.isEmpty()) { | ||
| //An error will be shown to the user in the reportPhishing function | ||
| SentryLog.e(TAG, getString(R.string.errorPhishingMessagesNull)) |
There was a problem hiding this comment.
Should we not avoid showing the descriptionDialog if we detect that messages are empty and we already know that the dialog can never succeed?
|
|
||
| if (messages.isEmpty()) { | ||
| //An error will be shown to the user in the reportPhishing function | ||
| SentryLog.e(TAG, getString(R.string.errorPhishingMessagesNull)) |
There was a problem hiding this comment.
The string id mentions that list is "null" but the list is not null, it's empty. Which is also what the string says.
Maybe we can rename this string id
|
No description provided.