Repository navigation
feat(chat): rework playback of voice messages and audio files - #6840
minichhenoch-sir wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 15.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 233 functions across 44 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: reworking voice-message and audio-file playback. Description check The description explains the problem, summarizes the implementation, documents testing, links issues, includes screenshots, and completes most checklist items. The TODO section is omitted and the mile… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
2117395 to
595960c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
77b41721-d573-4d30-9e5d-f90fb74fb575
📒 Files selected for processing (5)
app/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/ui/model/ChatMessageUi.ktapp/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.ktapp/src/main/java/com/nextcloud/talk/ui/chat/ChatView.kt
💤 Files with no reviewable changes (1)
- app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
Audio messages were downloaded completely before playback, their state was mirrored into the chat messages and reset by list updates, taps on another message were ignored while one was playing, the speed button had no effect, and playback ignored audio focus and unplugged headphones. VoiceMessageMediaService now streams audio messages through the app's HTTP client into a size-limited disk cache and is the single source of truth for chat audio. It handles audio focus, becoming noisy, wake locks and retries after network errors, and exposes playback to the notification, the lock screen and headsets. Voice messages continue with the newer voice messages; audio files are played among the audio files of the conversation and can be shuffled and repeated. The service only accepts media items from Talk itself, and account credentials are only sent to the WebDAV files of that account. The chat and the conversation list observe the playback through ChatAudioPlayer, a MediaController that is connected while the screen is started. Tapping another message switches playback, seeking happens once a drag ends, and a player bar shows what is playing, also for another conversation. Tapping the bar opens a full player with seeking, previous and next, speed, shuffle, repeat and the list of queued items, scrolled to the current one. Speeds are kept per account and separately for voice messages and audio files. ChatAudioStore keeps durations, waveforms and the position of long recordings. Waveforms are computed from the cached file once the player has loaded it, so the file is not downloaded twice. Assisted-by: Claude Code:Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174tMBt2Kwh4HoSaBBUQJrR Signed-off-by: Henoch Minich <301610408+minichhenoch-sir@users.noreply.github.com>
595960c to
a49f1f9
Compare
Opening the chat of the playing audio message from the player bar of the conversation list started ChatActivity without the internal user id, which fails in debug builds and opens the room with the default account in release builds. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
|
Thank you @minichhenoch-sir , this looks pretty solid 👍 I wonder if we really want to have a shuffle feature. I dont see a usecase for a chat app that audiofiles are shuffled? @rapterjet2004 Can you also have a look? Esp as you did a lot of things regarding voice messages @minichhenoch-sir could you please let us know if it's okay that we take over e.g. for rebasing (and maybe fixing) or if you would like to do this? |
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks for this, the rework looks good overall. The only thing I'd like changed before merging is what happens when an account is removed.
Requested change: stop playback and delete metadata when the playing account is removed
When an account is removed (from settings or by remote wipe), AccountRemovalWorker deletes the account, but the chat audio isn't touched:
- Playback keeps going.
VoiceMessageMediaServicekeeps playing the removed account's queue. Once the account is scheduled for deletion,UsersDao.getUsers()no longer returns it, so no credentials are sent anymore. Items that are already fully cached still play from the cache, though, and the media notification and lock screen keep showing that account's sender names and avatars until the user closes the player. After a remote wipe in particular, that account's audio shouldn't keep playing. - Metadata stays behind. The files in
cacheDir/chat_audio_metadata(duration, waveform, position, and the room token in the file name) are never deleted, and neither are the entries in the in-memory map ofChatAudioStore.
Could you:
- Stop playback and clear the queue when the account of the current item is removed. For example,
AccountRemovalWorker.initiateUserDeletioncould tell the service, or the service could check that the account still exists when the item changes. - Delete the account's metadata, both the files and the in-memory entries in
ChatAudioStore. The sanitized file names start withtalk-audio_<internalUserId>_, so a prefix match is enough; the trailing_keeps id 1 from matching id 12.
The cached audio files themselves can stay for now. Their cache keys don't identify the account, and clearing the cache on account removal is a broader issue that also affects attachments on master.
Non-blocking note (efficiency)
This doesn't need to block the PR, just noting it: ChatAudioDataSources.credentialsFor (ChatAudioDataSources.kt:68) runs runBlocking { userManager.getUsers() }, a database query, every time a data source is opened. It runs on a loader thread, so it's not a UI problem, but the accounts could be cached or observed instead.
|
Thank you for testing and for the fix! Yes, you are welcome to take over the PR, for rebasing and any fixes. About shuffle: it is meant for people who are looking for an alternative to Telegram, where you can listen to music shared in chats. |
|
@rapterjet2004 Thanks for the review! Since you are welcome to take over, it would be great if you could include the change for account removal. |
Could you do this @rapterjet2004 ? |
3 participants
Problem:
It was not possible to keep listening to audio in the background, and the playback speed of voice messages could not be changed.
What changes:
Tested:
Xiaomi Pad 7 Pro – Android 16
M301 MP3 Player – Android 9
Issues: #6093, #6408
AI:
Code and tests were created with Claude Code (Claude Opus 5.5).
I tested it myself on Nextcloud Hub 26 Spring (35.0.1). I defined the behavior of the points above and tested it on several devices.
🖼️ Screenshots
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)