Repository navigation
feat(chat): replace the attachment menu with a media bottom sheet - #6819
Open
ToteMeiSter wants to merge 18 commits into
Open
ToteMeiSter wants to merge 18 commits into
ToteMeiSter wants to merge 18 commits into
Conversation
The observer of the recording state is registered again when the chat activity is recreated, for example on a screen rotation. LiveData then delivers the current value to it and the device vibrated as if a recording had just started or ended. Vibrate only when the state differs from the one the activity already knew. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
A rotation stops the chat activity. ChatViewModel.onStop then stopped the MediaRecorder, while the locked and in-progress state of the recording stayed. The screen still showed a running recording and a truncated file was sent. The view model now keeps the recorder running when the stopping activity is changing its configuration. It reads this from the lifecycle owner: ON_STOP reaches the observer before the body of Activity.onStop runs (API 29+), so a flag set by the activity in onStop would come too late. The recorder is still stopped when the user leaves the chat. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
A short tap on the record button switches between voice and video mode. The mode is stored in AppPreferences and shown by the button icon and a snackbar hint. The result is a regular video attachment. In video mode holding the button records with CameraX (front camera, 720p with fallback, about 2.5 Mbit/s, at most 120 s, then it stops and is sent). Sliding left cancels, sliding up locks. The gestures, the lock state, the timer and the locked recording view are shared with the voice recording. The preview is shown above the input and the camera can be switched during the recording. An unfinished recording is cancelled and the camera released when the chat is paused. Too short recordings show a hint, camera errors a message. The finished mp4 goes through the regular file upload. Refs nextcloud#6812 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
… popup The record button used to start a recording on the first touch, so a short tap that should switch the mode started a recording first. Recording now starts only when the button is held. A tap shows the record hint as a popup above the button instead of a snackbar, placed in the window of the button and kept above it after layout changes. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
The activity is recreated on rotation while the video recorder belonged to it: onPause cancelled the recording and deleted its file, and binding the camera to the next activity made the CameraX recorder configure itself in a state where that is not allowed. - The video recorder lives in ChatViewModel like the voice recorder. The activity attaches its preview and callback, and the recorder lets go of them when the activity is destroyed. A result that arrives without an activity is delivered to the next one. - The camera is bound to a lifecycle of the recorder itself, not to the one of the activity, so it stays bound while the activity is recreated. - A new activity picks up the recording and locks it when it was held, so the timer, stop, send and cancel are shown. - The recording is cancelled when the user leaves the chat, not on a configuration change. - A recording cut off by the camera is not lost but shown in the attachment preview. - The video is recorded in the rotation of the sensor. - A recording is never stopped while a switched camera settles. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
The container of the video recording preview did not consume touches, so taps on it reached the chat below. Make it consume them and hide it from TalkBack, where it is only decoration. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
Show recent photos and videos from MediaStore in a 3-column grid with a live camera tile and multi-select, and keep every former menu entry in a bar below with unchanged visibility rules. Selected media open the existing attachment preview with caption. Supports Android 14 partial media access. Removes AttachmentDialog and dialog_attachment.xml. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
READ_MEDIA_VISUAL_USER_SELECTED turns off the Android 14 compatibility mode, so isFilesPermissionGranted() must accept the partial grant, otherwise voice and video messages, camera photos, local files and share-to-Talk ask for permissions again. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Compose attachment sheet with recent image and video selection, media permission handling, and conversation-specific actions. It also adds voice and video input modes and a hold gesture for recording. Video messages use CameraX and support camera switching, activity recreation, and distinct completion outcomes. Unit tests cover attachment rules, media access and selection, recording gestures, and video recorder behavior. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The media sheet’s permission handling preserves the expected request and access behavior. No concrete merge-blocking risk is established in the supplied change context. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Media access remains subject to Android permissions, and selected attachments still pass through the existing preview. Camera cleanup and recording cancellation have explicit controls. No security weakness was established, but overlapping camera use and lifecycle recovery retain some uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
cad5ab06-8c7d-4c28-b0e2-80f15e84101f
📒 Files selected for processing (43)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentAction.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentActionResources.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentSheet.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/CameraTile.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/MediaAccess.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/MediaSelection.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/RecentMedia.ktapp/src/main/java/com/nextcloud/talk/attachmentsheet/RecentMediaLoader.ktapp/src/main/java/com/nextcloud/talk/chat/CameraLens.ktapp/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/MessageInputFragment.ktapp/src/main/java/com/nextcloud/talk/chat/MessageInputVoiceRecordingFragment.ktapp/src/main/java/com/nextcloud/talk/chat/RecordButtonGesture.ktapp/src/main/java/com/nextcloud/talk/chat/RecordHintPopup.ktapp/src/main/java/com/nextcloud/talk/chat/RecordInputMode.ktapp/src/main/java/com/nextcloud/talk/chat/RecordingStopCoordinator.ktapp/src/main/java/com/nextcloud/talk/chat/VideoMessageRecorder.ktapp/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/main/java/com/nextcloud/talk/ui/dialog/AttachmentDialog.ktapp/src/main/java/com/nextcloud/talk/utils/permissions/PlatformPermissionUtilImpl.ktapp/src/main/java/com/nextcloud/talk/utils/preferences/AppPreferences.javaapp/src/main/java/com/nextcloud/talk/utils/preferences/AppPreferencesImpl.ktapp/src/main/res/drawable/bg_record_hint.xmlapp/src/main/res/drawable/ic_record_hint_arrow.xmlapp/src/main/res/layout/activity_chat.xmlapp/src/main/res/layout/dialog_attachment.xmlapp/src/main/res/layout/view_message_input.xmlapp/src/main/res/layout/view_record_hint.xmlapp/src/main/res/values/dimens.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/nextcloud/talk/attachmentsheet/AttachmentActionTest.ktapp/src/test/java/com/nextcloud/talk/attachmentsheet/MediaAccessTest.ktapp/src/test/java/com/nextcloud/talk/attachmentsheet/MediaSelectionTest.ktapp/src/test/java/com/nextcloud/talk/attachmentsheet/RecentMediaTest.ktapp/src/test/java/com/nextcloud/talk/chat/RecordButtonGestureTest.ktapp/src/test/java/com/nextcloud/talk/chat/RecordHintPopupPlacementTest.ktapp/src/test/java/com/nextcloud/talk/chat/RecordingStopCoordinatorTest.ktapp/src/test/java/com/nextcloud/talk/chat/ShouldVibrateOnRecordingChangeTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoMessageRecordingTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoRecorderLifecycleTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoRecordingRecreationTest.kt
💤 Files with no reviewable changes (2)
- app/src/main/res/layout/dialog_attachment.xml
- app/src/main/java/com/nextcloud/talk/ui/dialog/AttachmentDialog.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
Collaborator
|
Hi @ToteMeiSter thank you for your contributions. |
…g start The start time was taken on ACTION_DOWN, but the recording starts only after the 400-600 ms hold threshold, so a voice message was judged too short (or long enough) by a duration which included the hold. Take the time in beginRecording() after the recording has started and use the monotonic clock. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
…alized it Send or delete of a locked video recording cleared the lock and the in-progress state at once, while CameraX was still finalizing the file. A new recording started in that window was silently dropped in beginRecording(). For video the state is now released in onVideoRecordingFinished(), which CameraX reaches for every outcome (send, cancel, error, too short). Audio, and a video recorder which is already idle, are still cleared at once. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
…rded frame The locked video recording preview was a fixed 180x240dp box floating over the message bubbles, squashed to about 2:1 in landscape. Now a scrim dims the chat pane (it blocks touches and is hidden from TalkBack), and the preview is centered in it in the aspect of the recorded frame, fitted with margins, at most 75% of the width and 480dp on the longest side. The placement is a pure function (videoPreviewPlacement) with unit tests; it is recomputed when the pane changes size and when the recorder learns the frame aspect from CameraX. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
…entation, hide chat from TalkBack - Center by Gravity.CENTER and change only width and height: the margin comparison never settled in RTL and re-laid out the chat every frame. - The frame aspect is turned into screen coordinates by the difference of the recording and the display rotation (screenFrameAspect), so the frame matches the picture with auto-rotate off and after a turn during the recording. - While recording, the siblings of the scrim are hidden from TalkBack and get their previous values back afterwards. - The preview size is recalculated after a camera switch. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
In video mode the locked recording panel is one row: delete, red dot in a progress ring of the 120 s limit, timer, send. The voice panel is unchanged. The preview keeps a smaller gap on a short area. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
- Hide the whole recording indicator from TalkBack, so the progress ring does not announce its percentage every second. - Relative padding of the timer, so the gap stays between dot and timer in RTL. - No inset around the progress ring, so it fills its 24dp. - The ring scale is set in code only (PROGRESS_MAX). Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
…t by the effective state requestReadFilesPermissions() did not request READ_MEDIA_VISUAL_USER_SELECTED on Android 14+, although the manifest declares it, and the result handler looked only at grantResults[0]. With "selected photos" the first permission is denied, so a partial access showed a false refusal. The request is now the set of the attachment sheet (mediaPermissionsToRequest) plus READ_MEDIA_AUDIO from Android 13 on, as before. The result allows the file picker if any requested permission was granted or the effective access (permissionUtil.isFilesPermissionGranted()) is given: before Android 10 the granted READ permission differs from the WRITE one that check looks at. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
5 of 7 tasks
A video recording result that waited for a new activity was delivered from onCreate, before currentConversation and spreedCapabilities were set: the send path and the preview path could throw. The pending result is now delivered once the initial capabilities state has initialized both; a running recording still reattaches at once. The delete and send buttons of the compact video row announced the voice recording labels to TalkBack. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Keep reselection available for mixed Android 14 grants. · MediaAccess.kt:18-39
app/src/main/java/com/nextcloud/talk/attachmentsheet/MediaAccess.kt:18-39
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep reselection available for mixed Android 14 grants.
When image access is full, video access is selected-only, and
READ_MEDIA_VISUAL_USER_SELECTEDis granted,resolveMediaAccessreturnsFULL.AttachmentSheetBodythen omits the “Select more” callback, so users cannot expand the selected videos from the sheet. The existinggrantedMediaPermissions(context)helper can supply the per-type grants for this check; keepFULLunchanged for media loading.Suggested fix
- val access = remember(refreshKey) { currentMediaAccess(context) } + val grantedPermissions = remember(refreshKey) { grantedMediaPermissions(context) } + val access = remember(refreshKey) { + resolveMediaAccess(Build.VERSION.SDK_INT, grantedPermissions) + } + val canSelectMore = access == MediaAccess.PARTIAL || + (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE && + READ_MEDIA_VISUAL_USER_SELECTED in grantedPermissions && + (android.Manifest.permission.READ_MEDIA_IMAGES !in grantedPermissions || + android.Manifest.permission.READ_MEDIA_VIDEO !in grantedPermissions)) @@ - onSelectMore = permissionRequest.takeIf { access == MediaAccess.PARTIAL } + onSelectMore = permissionRequest.takeIf { canSelectMore }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
f2b0ae11-95a7-42a9-8de4-3692cf68f739
📒 Files selected for processing (19)
app/src/main/java/com/nextcloud/talk/attachmentsheet/MediaAccess.ktapp/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/MessageInputFragment.ktapp/src/main/java/com/nextcloud/talk/chat/MessageInputVoiceRecordingFragment.ktapp/src/main/java/com/nextcloud/talk/chat/RecordButtonGesture.ktapp/src/main/java/com/nextcloud/talk/chat/VideoMessageRecorder.ktapp/src/main/java/com/nextcloud/talk/chat/VideoPreviewPlacement.ktapp/src/main/res/drawable/video_recording_dot.xmlapp/src/main/res/layout/activity_chat.xmlapp/src/main/res/layout/fragment_message_input_voice_recording.xmlapp/src/main/res/values/colors.xmlapp/src/main/res/values/dimens.xmlapp/src/test/java/com/nextcloud/talk/attachmentsheet/MediaAccessTest.ktapp/src/test/java/com/nextcloud/talk/attachmentsheet/ShareFilePermissionTest.ktapp/src/test/java/com/nextcloud/talk/chat/RecordButtonGestureTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoFrameAspectTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoPreviewPlacementCompactPanelTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoPreviewPlacementTest.ktapp/src/test/java/com/nextcloud/talk/chat/VideoRecordingRecreationTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/test/java/com/nextcloud/talk/attachmentsheet/MediaAccessTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
…Android 14 With READ_MEDIA_VISUAL_USER_SELECTED and only one of READ_MEDIA_IMAGES/VIDEO granted, the access resolves to FULL and the sheet hid the action. canSelectMoreMedia also covers that mix; media loading is unchanged. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
Contributor
Author
|
@coderabbitai Re the outside-diff finding on Confirmed and fixed in b8eaef7: the new This reply and the fix were prepared with the help of an AI assistant (Claude Code) and reviewed before posting. |
5 of 7 tasks
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd '^(MediaAccess|CanSelectMoreMediaTest|AttachmentSheet)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/attachmentsheet/MediaAccess.kt
ast-grep outline app/src/test/java/com/nextcloud/talk/attachmentsheet/CanSelectMoreMediaTest.kt
ast-grep outline app/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentSheet.kt
cat -n app/src/main/java/com/nextcloud/talk/attachmentsheet/MediaAccess.kt
cat -n app/src/test/java/com/nextcloud/talk/attachmentsheet/CanSelectMoreMediaTest.kt
rg -n -C5 'canSelectMoreMedia\(|resolveMediaAccess\(|RecentMediaLoader\.load\(|MediaAccess\.(FULL|PARTIAL|NONE)' app/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentSheet.ktLength of output: 13523 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '155,190p' app/src/main/java/com/nextcloud/talk/attachmentsheet/AttachmentSheet.ktLength of output: 1871
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
…st returns When the system grants without a dialog there is no pause/resume, so the sheet kept the old grants and the placeholder. The request result now bumps a key that the grants and the media list depend on. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #6814
This is a feature PR; the issue is not approved yet, the PR is opened for discussion.
Depends on #6818
(the stack also contains the two voice recording fixes it depends on; the lens helpers in
CameraLens.ktcome from it)🖼️ Screenshots
Feature
The attachment dialog is replaced by a Compose
ModalBottomSheet:AttachmentDialoganddialog_attachment.xmlare removed.Media access:
READ_MEDIA_IMAGES,READ_MEDIA_VIDEOandREAD_MEDIA_VISUAL_USER_SELECTED. With partial access the grid shows the selected items and a "Select more photos and videos" entry.READ_MEDIA_VISUAL_USER_SELECTEDturns off the Android 14 compatibility mode for the whole app, soPlatformPermissionUtilImpl.isFilesPermissionGranted()accepts partial access (second commit). Without it voice and video messages, camera photos and "file from device" would ask for permissions again.Google Play policy
The grid reads MediaStore and needs
READ_MEDIA_IMAGES/READ_MEDIA_VIDEO; the system Photo Picker (#4829) was chosen before partly because of the Play policy (#4313). Whether the grid is acceptable at all, and the option of agplayvariant with only the Photo Picker (and noREAD_MEDIA_*request for the grid), are discussed in #6814.Current behavior of this branch:
genericandgplayare the same. The permissions are declared in the main manifest, so both flavors request them when the user taps "Allow access"; the merged manifests of both flavors containREAD_MEDIA_IMAGES,READ_MEDIA_VIDEOandREAD_MEDIA_VISUAL_USER_SELECTED. There is nogplay-only reduced variant yet; I will add one if the maintainers prefer it.Structure
Two commits on top of the video message PR:
feat(chat): replace attachment menu with a media bottom sheetfix(chat): count partial media access as files access on Android 14How to check on a device
Tested by the author on a Huawei DEL-LX9 with a fork build that has this feature together with other fork changes; this branch itself was built and unit-tested.
🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The change was written with the help of Claude Code (Anthropic). The author reviewed the code, built it and ran the unit tests, and checked the behavior on a device with the fork build.