feat: Add email contact picker when sending by email#620
feat: Add email contact picker when sending by email#620
Conversation
PR Reviewer Guide 🔍(Review updated until commit a2b1da9)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit a604891 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @Composable | ||
| private fun TrailingButton(appIcon: ImageVector, onClick: () -> Unit) { | ||
| IconButton(onClick = onClick) { | ||
| val (contentDescription, icon) = stringResource(R.string.contentDescriptionButtonHidePassword) to appIcon | ||
|
|
||
| Icon(icon, contentDescription, Modifier.size(Dimens.SmallIconSize)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Replace the hardcoded contentDescriptionButtonHidePassword string resource with an appropriate content description for the contact picker button. Using a password-related description for a contact picker is confusing for accessibility users and indicates copy-paste error. [general, importance: 8]
| @Composable | |
| private fun TrailingButton(appIcon: ImageVector, onClick: () -> Unit) { | |
| IconButton(onClick = onClick) { | |
| val (contentDescription, icon) = stringResource(R.string.contentDescriptionButtonHidePassword) to appIcon | |
| Icon(icon, contentDescription, Modifier.size(Dimens.SmallIconSize)) | |
| } | |
| } | |
| @Composable | |
| private fun TrailingButton(appIcon: ImageVector, onClick: () -> Unit) { | |
| IconButton(onClick = onClick) { | |
| val contentDescription = stringResource(R.string.contentDescriptionButtonAddContact) // Use appropriate string resource | |
| val icon = appIcon | |
| Icon(icon, contentDescription, Modifier.size(Dimens.SmallIconSize)) | |
| } | |
| } |
| val iterator = contactsMap.entries.iterator() | ||
| while (iterator.hasNext()) { | ||
| val (key, value) = iterator.next() | ||
| validatedRecipientsEmails = validatedRecipientsEmails.plus(value.emails) | ||
|
|
||
| } |
There was a problem hiding this comment.
Suggestion: Avoid updating validatedRecipientsEmails state inside a loop, which can trigger multiple recompositions or state emissions. Collect all emails first, then update the state once with the complete set to improve performance. [general, importance: 6]
| val iterator = contactsMap.entries.iterator() | |
| while (iterator.hasNext()) { | |
| val (key, value) = iterator.next() | |
| validatedRecipientsEmails = validatedRecipientsEmails.plus(value.emails) | |
| } | |
| val allNewEmails = contactsMap.values.flatMap { it.emails }.toSet() | |
| validatedRecipientsEmails = validatedRecipientsEmails.plus(allNewEmails) |
| if (clipData != null) { | ||
|
|
||
| val length = clipData.itemCount | ||
| val result = mutableListOf<Uri>() | ||
| for (i in 0 until length) { | ||
| selectContact(clipData.getItemAt(i).uri, context) | ||
| } |
There was a problem hiding this comment.
Suggestion: Remove the unused result variable declaration to eliminate dead code and unnecessary object allocation. The variable is populated but never consumed. [general, importance: 5]
| if (clipData != null) { | |
| val length = clipData.itemCount | |
| val result = mutableListOf<Uri>() | |
| for (i in 0 until length) { | |
| selectContact(clipData.getItemAt(i).uri, context) | |
| } | |
| if (clipData != null) { | |
| val length = clipData.itemCount | |
| for (i in 0 until length) { | |
| selectContact(clipData.getItemAt(i).uri, context) | |
| } |
|
Persistent review updated to latest commit e08157e |
|
Persistent review updated to latest commit 2bb029a |
| val iterator = contactsMap.entries.iterator() | ||
| while (iterator.hasNext()) { | ||
| val (key, value) = iterator.next() | ||
| validatedRecipientsEmails = validatedRecipientsEmails.plus(value.emails) | ||
|
|
||
| } |
There was a problem hiding this comment.
Suggestion: Update validatedRecipientsEmails on the main thread to avoid threading issues with Compose State. The state update is currently performed on Dispatchers.IO, which can cause race conditions or "illegal state access" exceptions in Compose. [possible issue, importance: 9]
| val iterator = contactsMap.entries.iterator() | |
| while (iterator.hasNext()) { | |
| val (key, value) = iterator.next() | |
| validatedRecipientsEmails = validatedRecipientsEmails.plus(value.emails) | |
| } | |
| withContext(Dispatchers.Main) { | |
| contactsMap.values.forEach { contact -> | |
| validatedRecipientsEmails = validatedRecipientsEmails.plus(contact.emails) | |
| } | |
| } |
| if (clipData != null) { | ||
| val length = clipData.itemCount | ||
| for (i in 0 until length) { | ||
| selectContact(clipData.getItemAt(i).uri, context) | ||
| } |
There was a problem hiding this comment.
Suggestion: Add null safety check for clipData.getItemAt(i).uri before passing it to selectContact. The uri property can return null, which would cause a crash when passed to the non-null parameter of the callback function. [possible issue, importance: 8]
| if (clipData != null) { | |
| val length = clipData.itemCount | |
| for (i in 0 until length) { | |
| selectContact(clipData.getItemAt(i).uri, context) | |
| } | |
| if (clipData != null) { | |
| val length = clipData.itemCount | |
| for (i in 0 until length) { | |
| clipData.getItemAt(i).uri?.let { uri -> | |
| selectContact(uri, context) | |
| } | |
| } |
| fun processContactPickerResultUri( | ||
| sessionUris: Uri, | ||
| context: Context, | ||
| ) { | ||
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add exception handling to the coroutine to prevent crashes from content resolver errors (e.g., SecurityException, IllegalArgumentException) and ensure the app remains stable if contact access fails. [possible issue, importance: 6]
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | |
| } | |
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| viewModelScope.launch { | |
| try { | |
| contactPickLaunch(sessionUris, context) | |
| } catch (e: Exception) { | |
| // Log error or handle contact parsing failure gracefully | |
| } | |
| } | |
| } |
…y, improve performance and avoiding threading issues
|
Persistent review updated to latest commit 573e64e |
| fun processContactPickerResultUri( | ||
| sessionUris: Uri, | ||
| context: Context, | ||
| ) { | ||
| try { | ||
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | ||
| }catch (e: Exception){ | ||
| SentryLog.e(TAG, "Error while launching contact picker", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Move the try-catch block inside the coroutine to properly catch exceptions thrown during contact processing. The current implementation only catches exceptions from launching the coroutine, not from the actual query execution, which can crash the app if the content provider fails. [possible issue, importance: 8]
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| try { | |
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | |
| }catch (e: Exception){ | |
| SentryLog.e(TAG, "Error while launching contact picker", e) | |
| } | |
| } | |
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| viewModelScope.launch { | |
| try { | |
| contactPickLaunch(sessionUris, context) | |
| } catch (e: Exception) { | |
| SentryLog.e(TAG, "Error while processing contact picker result", e) | |
| } | |
| } | |
| } |
| context.contentResolver.query(sessionUris, projection, null, null, null)?.use { cursor -> | ||
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | ||
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | ||
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | ||
|
|
||
| while (cursor.moveToNext()) { | ||
| val lookupKey = cursor.getString(lookupKeyIdx) | ||
| val mimeType = cursor.getString(mimeTypeIdx) | ||
| val data1 = cursor.getString(data1Idx) ?: "" |
There was a problem hiding this comment.
Suggestion: Validate that getColumnIndex returns valid indices (>= 0) before using them in getString. If the content provider schema differs or columns are missing, getColumnIndex returns -1, which causes an IllegalStateException when passed to getString. [possible issue, importance: 6]
| context.contentResolver.query(sessionUris, projection, null, null, null)?.use { cursor -> | |
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | |
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | |
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | |
| while (cursor.moveToNext()) { | |
| val lookupKey = cursor.getString(lookupKeyIdx) | |
| val mimeType = cursor.getString(mimeTypeIdx) | |
| val data1 = cursor.getString(data1Idx) ?: "" | |
| context.contentResolver.query(sessionUris, projection, null, null, null)?.use { cursor -> | |
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | |
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | |
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | |
| if (lookupKeyIdx < 0 || mimeTypeIdx < 0 || data1Idx < 0) { | |
| SentryLog.e(TAG, "Required columns not found in contact cursor") | |
| return@use | |
| } | |
| while (cursor.moveToNext()) { | |
| val lookupKey = cursor.getString(lookupKeyIdx) | |
| val mimeType = cursor.getString(mimeTypeIdx) | |
| val data1 = cursor.getString(data1Idx) ?: "" |
|
Persistent review updated to latest commit a83cc96 |
| fun processContactPickerResultUri( | ||
| sessionUris: Uri, | ||
| context: Context, | ||
| ) { | ||
| try { | ||
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | ||
| }catch (e: Exception){ | ||
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Move the try-catch block inside the coroutine block. Exceptions thrown inside viewModelScope.launch occur asynchronously and will not be caught by the outer try-catch, causing crashes to go unreported. [possible issue, importance: 9]
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| try { | |
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | |
| }catch (e: Exception){ | |
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | |
| } | |
| } | |
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| viewModelScope.launch { | |
| try { | |
| contactPickLaunch(sessionUris, context) | |
| } catch (e: Exception) { | |
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | |
| } | |
| } | |
| } |
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | ||
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | ||
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | ||
|
|
||
| while (cursor.moveToNext()) { | ||
| val lookupKey = cursor.getString(lookupKeyIdx) | ||
| val mimeType = cursor.getString(mimeTypeIdx) | ||
| val data1 = cursor.getString(data1Idx) ?: "" |
There was a problem hiding this comment.
Suggestion: Validate that getColumnIndex returns valid indices (>= 0) before using them. If the content provider returns unexpected columns, getString(-1) will throw an exception and crash the app. [possible issue, importance: 6]
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | |
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | |
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | |
| while (cursor.moveToNext()) { | |
| val lookupKey = cursor.getString(lookupKeyIdx) | |
| val mimeType = cursor.getString(mimeTypeIdx) | |
| val data1 = cursor.getString(data1Idx) ?: "" | |
| val lookupKeyIdx = cursor.getColumnIndex(ContactsContract.Contacts.LOOKUP_KEY) | |
| val mimeTypeIdx = cursor.getColumnIndex(ContactsContract.Data.MIMETYPE) | |
| val data1Idx = cursor.getColumnIndex(ContactsContract.Data.DATA1) | |
| if (lookupKeyIdx == -1 || mimeTypeIdx == -1 || data1Idx == -1) { | |
| SentryLog.e(TAG, "Required contact columns not found") | |
| return@withContext | |
| } | |
| while (cursor.moveToNext()) { | |
| val lookupKey = cursor.getString(lookupKeyIdx) | |
| val mimeType = cursor.getString(mimeTypeIdx) | |
| val data1 = cursor.getString(data1Idx) ?: "" |
|
Persistent review updated to latest commit a2b1da9 |
|
| fun processContactPickerResultUri( | ||
| sessionUris: Uri, | ||
| context: Context, | ||
| ) { | ||
| try { | ||
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | ||
| }catch (e: Exception){ | ||
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Move the try-catch block inside the coroutine. The current implementation catches exceptions synchronously, but viewModelScope.launch is asynchronous, so exceptions thrown inside contactPickLaunch will not be caught and will crash the app instead of being logged to Sentry. [possible issue, importance: 9]
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| try { | |
| viewModelScope.launch { contactPickLaunch(sessionUris, context) } | |
| }catch (e: Exception){ | |
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | |
| } | |
| } | |
| fun processContactPickerResultUri( | |
| sessionUris: Uri, | |
| context: Context, | |
| ) { | |
| viewModelScope.launch { | |
| try { | |
| contactPickLaunch(sessionUris, context) | |
| } catch (e: Exception) { | |
| SentryLog.e(TAG, "Error while importing contacts from picker result", e) | |
| } | |
| } | |
| } |
| } catch (_: ActivityNotFoundException) { | ||
| coroutine.launch { | ||
| longToast(R.string.startActivityCantHandleAction) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add the context receiver to the longToast call. The longToast function from Splitties is an extension function on Context and requires a context receiver to compile and function correctly. [possible issue, importance: 9]
| } catch (_: ActivityNotFoundException) { | |
| coroutine.launch { | |
| longToast(R.string.startActivityCantHandleAction) | |
| } | |
| } | |
| } catch (_: ActivityNotFoundException) { | |
| coroutine.launch { | |
| context.longToast(R.string.startActivityCantHandleAction) | |
| } | |
| } |
2 participants
Ajout d'un bouton dans la zone de saisie pour l'envoi par email, pour ouvrir la sélection de contact native.
Note: Nouvelle méthode disponible pour android 17, à regarder à sa sortie.