Refactor listening stats tracking to MusicService#1976
Conversation
Moves the responsibility of tracking listening statistics from the `PlayerViewModel` and UI state holders to `MusicService`. This ensures more accurate tracking that persists independently of the UI lifecycle and centralizes logic for both local and remote (Cast) playback. - Refactors `ListeningStatsTracker` to be thread-safe using `@Synchronized` and decouples it from the `Song` data model by using `songId`. - Introduces `ensureSession` and `onTrackChanged` in `ListeningStatsTracker` to handle state synchronization more robustly. - Implements `syncLocalListeningStatsFromPlayer` in `MusicService` to update tracking state based on Media3 `Player.Listener` events. - Adds `syncCastListeningStatsFromRemote` to `MusicService` to support listening stats for Cast sessions via `RemoteMediaClient` callbacks. - Removes `ListeningStatsTracker` dependencies and manual progress reporting from `PlayerViewModel`, `PlaybackStateHolder`, and `CastTransferStateHolder`. - Ensures listening sessions are finalized in `MusicService.onDestroy`.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cddca23ef0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| persistenceScope.launch { | ||
| runCatching { | ||
| persistPlaybackInternal(songId = songId, listened = listened, timestamp = timestamp) |
There was a problem hiding this comment.
When MusicService.onDestroy()/unload calls finalizeCurrentSession(forceSynchronousPersistence = true), the forced flag now reaches this method but is explicitly ignored and the write is only enqueued on persistenceScope. In those shutdown paths Android can tear down the service/process before the launched coroutine records the final session, so the last play can be missing from engagement/history even though callers asked for synchronous persistence; the forced path should block or otherwise await the write before returning.
Useful? React with 👍 / 👎.
1 participant
Moves the responsibility of tracking listening statistics from the
PlayerViewModeland UI state holders toMusicService. This ensures more accurate tracking that persists independently of the UI lifecycle and centralizes logic for both local and remote (Cast) playback.ListeningStatsTrackerto be thread-safe using@Synchronizedand decouples it from theSongdata model by usingsongId.ensureSessionandonTrackChangedinListeningStatsTrackerto handle state synchronization more robustly.syncLocalListeningStatsFromPlayerinMusicServiceto update tracking state based on Media3Player.Listenerevents.syncCastListeningStatsFromRemotetoMusicServiceto support listening stats for Cast sessions viaRemoteMediaClientcallbacks.ListeningStatsTrackerdependencies and manual progress reporting fromPlayerViewModel,PlaybackStateHolder, andCastTransferStateHolder.MusicService.onDestroy.