fix: Apply comments' suggestions on summary and translation PRs#2926
fix: Apply comments' suggestions on summary and translation PRs#2926Elouan1411 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Despite the title suggesting only a comment fix, this PR is a broader cleanup around the AI Summary/Translate flow. It removes the now-unused actionCloseSummaryWindow string in the base locale and 12 translations, renames the Matomo event Summary → Summarize (and updates its single call site), renames AiProcessState.Error.isRetry → isRetryAttempt, adds a GPL license header to ic_paragraph_shorten.xml, reorders operations in getCurrentLanguageCode, simplifies setTranslateUi/setSummaryUi away from with(...), and—most substantially—moves the AI summary/translate state machine (timers, API calls, body updates) from ThreadFragment into ThreadViewModel, surfacing UI updates via a new aiStateUpdates: SingleLiveEvent<AiStateUpdate> and new AiAction/AiBodyUpdate types.
Changes:
- Refactor AI summary/translate logic from
ThreadFragmentintoThreadViewModel, with retry timers, state map updates and a newAiStateUpdateevent consumed by the fragment. - Rename
AiProcessState.Error.isRetry→isRetryAttempt(with adapter updated) and MatomoSummary→Summarize. - Remove the unused
actionCloseSummaryWindowstring from base + 12 locale files; add license header toic_paragraph_shorten.xml; minor tweaks togetCurrentLanguageCodeandMailActionsBottomSheetDialogUI setters.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Removes unused actionCloseSummaryWindow base string. |
| app/src/main/res/values-{da,de,el,es,fi,fr,it,nb,nl,pl,pt,sv}/strings.xml | Removes the corresponding translated actionCloseSummaryWindow entries. |
| app/src/main/res/drawable/ic_paragraph_shorten.xml | Adds GPL license header. |
| app/src/main/java/com/infomaniak/mail/utils/extensions/ContextExt.kt | Reorders take(2).lowercase() and trims a trailing blank line. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/ThreadViewModel.kt | Adds AI action orchestration, retry timers, AiStateUpdate event and related enums. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/ThreadState.kt | Renames isRetry → isRetryAttempt on AiProcessState.Error. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/ThreadFragment.kt | Delegates AI flow to the view model and observes aiStateUpdates; drops local timers and helpers. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/ThreadAdapter.kt | Updates AiAction import to the view-model namespace, renames usage to isRetryAttempt, removes stray kotlin.context import. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MessageActionsBottomSheetDialog.kt | Tracks MatomoName.Summarize instead of Summary. |
| app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/MailActionsBottomSheetDialog.kt | Rewrites setTranslateUi/setSummaryUi without with(...). |
| app/src/main/java/com/infomaniak/mail/MatomoMail.kt | Renames Matomo enum entry Summary("summary") → Summarize("summarize"). |
Comments suppressed due to low confidence (1)
app/src/main/java/com/infomaniak/mail/ui/main/thread/ThreadFragment.kt:294
- After the refactor,
onDestroyViewno longer does any AI-related cleanup, but a stray blank line was left at the start of the method body. Please remove it to match the style of the surrounding overrides.
override fun onDestroyView() {
threadAdapter.resetCallbacks()
super.onDestroyView()
_binding = null
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| val result = when (aiAction) { | ||
| AiAction.SUMMARY -> ApiRepository.aiSummary(languageCode, content) | ||
| AiAction.TRANSLATE -> ApiRepository.aiTranslate(languageCode, mailbox().uuid, messageUid) |
fc7baa4 to
b36f3a3
Compare
b36f3a3 to
cb64a93
Compare
| AiAction.SUMMARY -> ThreadAdapter.NotifyType.AiSummaryStateChanged | ||
| AiAction.TRANSLATE -> ThreadAdapter.NotifyType.AiTranslateStateChanged |
There was a problem hiding this comment.
| AiAction.SUMMARY -> ThreadAdapter.NotifyType.AiSummaryStateChanged | |
| AiAction.TRANSLATE -> ThreadAdapter.NotifyType.AiTranslateStateChanged | |
| AiAction.SUMMARY -> NotifyType.AiSummaryStateChanged | |
| AiAction.TRANSLATE -> NotifyType.AiTranslateStateChanged |
FabianDevel
left a comment
There was a problem hiding this comment.
We'll need to extract the whole code of your actions to another viewModel 'AiActionsViewModel' or to a manager
3bb605b to
68b6fef
Compare
d50fbd7 to
972f513
Compare
|
| isVisible = true | ||
| fun setSummaryUi(isEnabled: Boolean) { | ||
| binding.summary.isEnabled = isEnabled | ||
| binding.summary.isVisible = true |
| } | ||
|
|
||
| val cleanCode = languageCode?.lowercase()?.take(2) ?: defaultLanguage | ||
| val cleanCode = languageCode?.take(2)?.lowercase() ?: defaultLanguage |
There was a problem hiding this comment.
It's the same ... right ?
| hideButtonProgress(R.string.aiButtonRetry) | ||
|
|
||
| if (state.isRetry && !state.wasLoaderShown) { | ||
| if (state.asAlreadyRetried && !state.wasLoaderShown) { |
There was a problem hiding this comment.
You mean hasAlreadyRetried ?



No description provided.