Drag and drop files into Socialite #154
Conversation
There was a problem hiding this comment.
Hello @sashalukin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request implements drag and drop functionality for files into the Socialite application. Specifically, it allows users to drag and drop image files into the chat screen to attach them as media items. The implementation involves adding a dragAndDropTarget modifier to the MessageList composable, which handles the drag and drop events. When a file is dropped, the application requests drag and drop permissions, extracts the URI of the file, and attaches it as a media item to the chat. A border is also added to visually indicate when the user is dragging a file over the chat area.
Highlights
- Drag and Drop Implementation: Implements drag and drop functionality for attaching media files to the chat.
- UI Enhancement: Adds a visual border to indicate when a file is being dragged over the chat area.
- Permission Handling: Requests drag and drop permissions when a file is dropped.
Changelog
- app/src/main/java/com/google/android/samples/socialite/ui/chat/ChatScreen.kt
- Added necessary imports for drag and drop functionality, including
DragAndDropTarget,DragAndDropEvent, andmimeTypes. - Introduced a
isDraggedOverstate variable to track whether a file is being dragged over the chat area. - Implemented a
DragAndDropTargetcallback to handle drag and drop events. - In the
onDropmethod, the code requests drag and drop permissions, extracts the URI of the dropped file, and attaches it as a media item. - Added a border to the
MessageListcomposable to visually indicate when a file is being dragged over it. - Added
dragAndDropTargetmodifier to theMessageListcomposable to enable drag and drop functionality.
- Added necessary imports for drag and drop functionality, including
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A file dragged, a message sent,
With Socialite, intent.
Images fly with ease,
A modern digital breeze,
Sharing moments heaven-sent.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces drag and drop functionality for attaching media items to the chat screen. The implementation appears to be well-structured and integrates smoothly with the existing UI components. However, there are a few areas that could be improved for better performance, error handling, and code clarity.
Summary of Findings
- DragAndDropTarget Implementation: The anonymous
DragAndDropTargetimplementation could be extracted into a named class or object for better readability and reusability. - MIME Type Filtering: The
shouldStartDragAndDroplambda only checks forimage/*mime types. Consider expanding this to includevideo/*or other relevant types. - API Level Check: The
onDropfunction is annotated with@RequiresApi(Build.VERSION_CODES.N). Add a check to ensure that the code doesn't run on older devices, or provide a fallback mechanism. - Error Handling: The
try-catchblock only logs the exception. Consider providing more user-friendly feedback, such as displaying an error message in the UI. - ClipData Iteration: The code iterates through all items in the
ClipData. If a large number of items are present, this could potentially lead to performance issues. Consider adding a limit to the number of items processed. - MIME Type Default Value: The
mimeTypeis defaulted to an empty string if the content resolver returns null. Consider defaulting toapplication/octet-streamor another more appropriate default.
Merge Readiness
The pull request introduces a valuable feature and is generally well-implemented. However, addressing the identified issues would improve the code's robustness, performance, and user experience. I recommend addressing these points before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| val callback = remember { | ||
| object : DragAndDropTarget { | ||
| @RequiresApi(Build.VERSION_CODES.N) | ||
| override fun onDrop(event: DragAndDropEvent): Boolean { | ||
| isDraggedOver = false | ||
| activity.requestDragAndDropPermissions(event.toAndroidDragEvent()) | ||
| val clipData = event.toAndroidDragEvent().clipData | ||
| if (clipData != null && clipData.itemCount > 0) { | ||
| for (i in 0 until clipData.itemCount) { | ||
| val item = clipData.getItemAt(i) | ||
| val passedUri = item.uri?.toString() | ||
| if (!passedUri.isNullOrEmpty()) { | ||
| try { | ||
| val mimeType = context.contentResolver | ||
| .getType(passedUri.toUri()) ?: "" | ||
| onMediaItemAttached(MediaItem(passedUri, mimeType)) | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Can't access URI: $passedUri", e) | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| override fun onEntered(event: DragAndDropEvent) { | ||
| super.onEntered(event) | ||
| isDraggedOver = true | ||
| } | ||
|
|
||
| override fun onExited(event: DragAndDropEvent) { | ||
| super.onExited(event) | ||
| isDraggedOver = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider extracting this anonymous DragAndDropTarget implementation into a named class or object. This would improve readability and potentially reusability. Also, the callback variable is only used once, so it could be inlined directly into the dragAndDropTarget modifier.
| .then(dragAndDropBorderModifier) | ||
| .dragAndDropTarget( | ||
| shouldStartDragAndDrop = { event -> | ||
| event.mimeTypes().any { it.startsWith("image/") } |
There was a problem hiding this comment.
The shouldStartDragAndDrop lambda only checks for image/* mime types. Consider expanding this to include video/* or other relevant types to allow for more flexible drag and drop functionality. It might be good to add a comment explaining why this check is here.
| @RequiresApi(Build.VERSION_CODES.N) | ||
| override fun onDrop(event: DragAndDropEvent): Boolean { |
There was a problem hiding this comment.
The onDrop function is annotated with @RequiresApi(Build.VERSION_CODES.N). It would be good to add a check to ensure that the code doesn't run on older devices, or provide a fallback mechanism. Consider using a helper function to encapsulate this check.
| } catch (e: Exception) { | ||
| Log.e(TAG, "Can't access URI: $passedUri", e) | ||
| } |
There was a problem hiding this comment.
The try-catch block only logs the exception. Consider providing more user-friendly feedback, such as displaying an error message in the UI, to inform the user that the file could not be attached. Also, it would be helpful to include the exception message in the log for debugging purposes.
} catch (e: Exception) {
Log.e(TAG, "Can't access URI: $passedUri, ${e.message}", e)
// Optionally, show a Toast or Snackbar to the user
// Toast.makeText(context, "Failed to attach media", Toast.LENGTH_SHORT).show()
}| for (i in 0 until clipData.itemCount) { | ||
| val item = clipData.getItemAt(i) |
There was a problem hiding this comment.
The code iterates through all items in the ClipData. If a large number of items are present, this could potentially lead to performance issues. Consider adding a limit to the number of items processed or using a more efficient data structure if possible.
| val mimeType = context.contentResolver | ||
| .getType(passedUri.toUri()) ?: "" |
There was a problem hiding this comment.
The mimeType is retrieved using context.contentResolver.getType(passedUri.toUri()) ?: "". If the content resolver returns null, the mime type is defaulted to an empty string. This could cause issues later on. Consider defaulting to application/octet-stream or another more appropriate default.
val mimeType = context.contentResolver
.getType(passedUri.toUri()) ?: "application/octet-stream"
No description provided.