fix review comments

Signed-off-by: VishnuSanal <t.v.s10123@gmail.com>

Author: VishnuSanal

CLAUDE.md

@@ -7,7 +7,8 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
 DialogMusicPlayer is a minimal Android music player that displays as a dialog. It handles audio files via `VIEW`/`SEND` intents — it is not a library browser. Licensed under GPLv3.
 
 Package: `phone.vishnu.dialogmusicplayer` (debug suffix: `.debug`)
-Min SDK 21, Target SDK 35, Kotlin 2.0, Java 21.
+Min SDK 21, Target SDK 35, Kotlin 2.0, Java 21. The app code is Kotlin + Jetpack
+Compose; only the boilerplate test stubs remain in Java.
 
 ## Build Commands
 
@@ -23,27 +24,29 @@ Min SDK 21, Target SDK 35, Kotlin 2.0, Java 21.
 ## Code Formatting
 
 Spotless is configured in the root `build.gradle`:
-- **Java**: Google Java Format (AOSP style), import order: `android, androidx, com, java, phone`, unused imports removed
+- **Java**: Google Java Format (AOSP style), import order: `android, androidx, com, java, phone`, unused imports removed (only the test stubs are Java)
 - **Kotlin**: ktlint 0.49.1, trailing whitespace trimmed
 - **XML**: tabs for indentation, trailing whitespace trimmed
 - All source files must have the GPLv3 license header from `spotless-header`
 
 ## Architecture
 
-Single-module app (`app/`), single-activity (`MainActivity`), mixed Java/Kotlin codebase.
+Single-module app (`app/`), single-activity (`MainActivity`), Kotlin codebase. MVVM:
+`MainActivity` maps `MediaControllerCompat` callbacks into a `StateFlow<PlayerUiState>`
+on `MainViewModel`; the Compose UI renders that state.
 
-**UI layer (Java):**
-- `MainActivity` — Dialog-styled activity that receives audio URIs via intent, connects to `MediaPlaybackService` through `MediaBrowserCompat`, controls playback via `MediaControllerCompat`
-- Layout: `activity_main.xml` (single layout)
+**UI layer (Kotlin + Jetpack Compose):**
+- `MainActivity` — Dialog-styled activity that receives audio URIs via intent, connects to `MediaPlaybackService` through `MediaBrowserCompat`, controls playback via `MediaControllerCompat`, and hosts the Compose UI with `setContent`
+- `PlayerScreen` — the player UI as composables (album art, marquee title/artist, slider, transport + speed controls)
+- `PlayerUiState` — immutable snapshot the screen renders; `DmpTheme` — Material3 theme wrapper (dynamic color on API 31+)
 
-**Playback layer (Java):**
+**Playback layer (Kotlin):**
 - `MediaPlaybackService` — `MediaBrowserServiceCompat` that manages `MediaPlayer`, audio focus, media session, and foreground notification. Communicates state back to `MainActivity` via media session callbacks.
 
 **Persistence layer (Kotlin):**
 - Room database for saving/resuming playback position per track
 - `SaveItem` (entity, keyed by track ID) → `SaveItemDao` → `SaveItemDatabase` → `SaveItemRepository` → `MainViewModel` (coroutines + LiveData)
 
-**Utilities (Java):**
+**Utilities (Kotlin):**
 - `AudioUtils` — metadata extraction from audio URIs
-- `ColorUtils` — dynamic theming from album art
-- `FileUtils` — file path resolution from URIs
+- `FileUtils` — one-time cleanup of legacy internal-storage files

CODE_REVIEW_NOTES.md

@@ -99,9 +99,9 @@ app/src/main/java/phone/vishnu/dialogmusicplayer/SaveItemDatabase.kt
 app/src/main/java/phone/vishnu/dialogmusicplayer/SaveItemRepository.kt
 ```
 
-`AndroidManifest.xml` was intentionally **not** changed — Spotless wanted a
-whitespace-only reformat of a pre-existing comment block; out of scope for this
-pass.
+`AndroidManifest.xml` carries one **non-functional** addition — an API /
+testing-status comment block at the top of the file. No permissions,
+components or attributes were changed.
 
 ---
 
@@ -176,8 +176,10 @@ The whole UI layer was migrated from XML Views to Jetpack Compose.
 ### Removed
 - `activity_main.xml` and `layout-v23/activity_main.xml`.
 - `ColorUtils.kt`.
-- The (already `visibility="gone"`) playback-speed UI was not carried over — it
-  was dead UI in the XML version. The service still supports speed changes.
+
+> **Correction (see §9.2):** an earlier draft of these notes claimed the
+> playback-speed button was dead UI. It was not — `layout-v23/activity_main.xml`
+> showed it on API 23+. The Compose `SpeedButton` restores that control.
 
 ### Not verifiable here
 `assembleDebug` passes, but **the dialog window sizing, dynamic theming, marquee
@@ -224,3 +226,49 @@ listener. On the test device `requestAudioFocus()` returned
 
 Verified on device: file plays, position advances, metadata + embedded album
 art render in the Compose UI.
+
+---
+
+## 9. PR #68 review fixes
+
+Addressed the 11 automated review comments on
+[PR #68](https://github.com/VishnuSanal/DialogMusicPlayer/pull/68).
+
+### 9.1 Correctness
+- **`FileUtils.clearApplicationData`** marked the legacy cleanup "done" even when
+  `deleteRecursively()` failed (its `Boolean` result was ignored and the
+  preference was written unconditionally). Now the flag is persisted only when
+  every entry was actually deleted, so a failed/partial sweep is retried.
+- **`MediaPlaybackService` repeat-one** — an `isPlayingOnceInProgress` flag made
+  `REPEAT_MODE_ONE` replay only every *other* completion. Removed the flag;
+  repeat-one (and repeat-all) now replay on every completion.
+
+### 9.2 Restored playback-speed control
+The Compose migration dropped the speed button, but `layout-v23` exposed it on
+API 23+ and the service still handles `onSetPlaybackSpeed`. Restored:
+- `PlayerUiState.playbackSpeed`; `MainViewModel.onPlaybackStateChanged` now
+  carries speed (keeping the last real value while the session reports `0`).
+- `PlayerScreen` has a tappable `SpeedButton` (0.5×→0.75×→1×→1.25×→1.5×→2×),
+  shown only on API 23+ — exactly where the service honours it.
+- `MainActivity.cyclePlaybackSpeed()` drives it via `setPlaybackSpeed`.
+
+### 9.3 Accessibility
+- The remaining-time toggle used a raw `pointerInput`/`detectTapGestures`, which
+  is invisible to TalkBack and keyboard users. Switched to `Modifier.clickable`
+  with `Role.Button` and an `onClickLabel`. The new `SpeedButton` does the same.
+- Rewind/forward content descriptions moved to string resources.
+
+### 9.4 Permission gating for shared files
+`onCreate` gated *all* playback on the library-wide media permission. A
+`content://` URI from `ACTION_VIEW`/`SEND` carries its own temporary read grant,
+so a file the user explicitly opened can play without that permission. Playback
+now starts when the essential permission is held **or** the intent URI is a
+`content://` URI (`canPlayWithoutPermission`).
+
+### 9.5 Documentation
+- `CLAUDE.md` updated — the UI is Kotlin + Compose (not Java/XML), the playback
+  layer is Kotlin, `ColorUtils` is gone, and only the test stubs are Java.
+- The root `build.gradle` Java Spotless block was restored (the two Java test
+  stubs still need formatting/header coverage).
+- §4 and §7 above corrected (manifest comment block; speed button was not dead
+  UI).

app/src/main/java/phone/vishnu/dialogmusicplayer/FileUtils.kt

@@ -44,11 +44,21 @@ object FileUtils {
         if (prefs.getBoolean(KEY_LEGACY_FILES_CLEARED, false)) return
 
         ioExecutor.execute {
-            runCatching {
-                context.filesDir.listFiles()?.forEach { it.deleteRecursively() }
-            }.onFailure { Log.w(TAG, "clearApplicationData() failed", it) }
+            // deleteRecursively() returns false on a failed/partial delete; only
+            // treat the cleanup as done when every entry was actually removed,
+            // otherwise it is retried on the next launch.
+            val cleared = runCatching {
+                context.filesDir.listFiles()
+                    ?.map { it.deleteRecursively() }
+                    ?.all { it }
+                    ?: true
+            }.onFailure {
+                Log.w(TAG, "clearApplicationData() failed", it)
+            }.getOrDefault(false)
 
-            prefs.edit().putBoolean(KEY_LEGACY_FILES_CLEARED, true).apply()
+            if (cleared) {
+                prefs.edit().putBoolean(KEY_LEGACY_FILES_CLEARED, true).apply()
+            }
         }
     }
 }

app/src/main/java/phone/vishnu/dialogmusicplayer/MainActivity.kt

@@ -23,6 +23,7 @@ import android.Manifest
 import android.annotation.SuppressLint
 import android.content.BroadcastReceiver
 import android.content.ComponentName
+import android.content.ContentResolver
 import android.content.Context
 import android.content.Intent
 import android.content.IntentFilter
@@ -90,7 +91,7 @@ class MainActivity : AppCompatActivity() {
         override fun onPlaybackStateChanged(state: PlaybackStateCompat?) {
             super.onPlaybackStateChanged(state)
             state ?: return
-            viewModel.onPlaybackStateChanged(state.state, state.position)
+            viewModel.onPlaybackStateChanged(state.state, state.position, state.playbackSpeed)
         }
 
         override fun onRepeatModeChanged(repeatMode: Int) {
@@ -126,6 +127,7 @@ class MainActivity : AppCompatActivity() {
                     onRewind = { seekBy(-SEEK_STEP_MS) },
                     onForward = { seekBy(SEEK_STEP_MS) },
                     onRepeat = ::cycleRepeatMode,
+                    onCycleSpeed = ::cyclePlaybackSpeed,
                     onBackgroundTap = { moveTaskToBack(false) },
                 )
             }
@@ -134,8 +136,10 @@ class MainActivity : AppCompatActivity() {
         // Playback only needs the audio-read permission. POST_NOTIFICATIONS is
         // optional (a nicer notification) and must NOT gate playback — so we
         // start as soon as the essential permission is available and request
-        // anything still missing separately.
-        if (hasEssentialPermission()) initTasks(intent)
+        // anything still missing separately. A content:// URI delivered by
+        // ACTION_VIEW/SEND carries its own temporary read grant, so the file
+        // the user explicitly opened plays even without library-wide access.
+        if (hasEssentialPermission() || canPlayWithoutPermission(intent)) initTasks(intent)
         requestMissingPermissions()
     }
 
@@ -227,6 +231,10 @@ class MainActivity : AppCompatActivity() {
             return
         }
 
+        // A content:// file may already be playing on its own temporary grant —
+        // don't nag about the denied library-wide permission in that case.
+        if (mediaBrowser != null) return
+
         val essential = essentialPermission() ?: return
         if (ActivityCompat.shouldShowRequestPermissionRationale(this, essential)) {
             Toast.makeText(
@@ -276,6 +284,16 @@ class MainActivity : AppCompatActivity() {
         viewModel.onRepeatModeChanged(next)
     }
 
+    /** Steps to the next speed in [PLAYBACK_SPEEDS], wrapping around. */
+    private fun cyclePlaybackSpeed() {
+        val current = mediaController?.playbackState?.playbackSpeed ?: 1f
+        val baseIndex = PLAYBACK_SPEEDS.indexOfFirst { it == current }
+            .let { if (it == -1) PLAYBACK_SPEEDS.indexOf(1f) else it }
+        transportControls?.setPlaybackSpeed(
+            PLAYBACK_SPEEDS[(baseIndex + 1) % PLAYBACK_SPEEDS.size],
+        )
+    }
+
     // ---- Media browser plumbing ------------------------------------------------
 
     private fun initTasks(intent: Intent) {
@@ -286,12 +304,7 @@ class MainActivity : AppCompatActivity() {
             return
         }
 
-        val uri: Uri? = if (Intent.ACTION_VIEW == intent.action) {
-            intent.data
-        } else {
-            IntentCompat.getParcelableExtra(intent, Intent.EXTRA_STREAM, Uri::class.java)
-        }
-
+        val uri = resolveUri(intent)
         if (uri == null) {
             showFatalError(intent.action)
             return
@@ -304,6 +317,27 @@ class MainActivity : AppCompatActivity() {
         }
     }
 
+    /** The audio URI carried by an ACTION_VIEW / ACTION_SEND intent, if any. */
+    private fun resolveUri(intent: Intent): Uri? {
+        return when (intent.action) {
+            Intent.ACTION_VIEW -> intent.data
+            Intent.ACTION_SEND ->
+                IntentCompat.getParcelableExtra(intent, Intent.EXTRA_STREAM, Uri::class.java)
+
+            else -> null
+        }
+    }
+
+    /**
+     * A `content://` URI from ACTION_VIEW/SEND comes with a temporary read grant
+     * that is enough to open that one file, so it can play even when the user
+     * has denied the library-wide media permission. `file://` URIs have no such
+     * grant and genuinely need [essentialPermission].
+     */
+    private fun canPlayWithoutPermission(intent: Intent): Boolean {
+        return resolveUri(intent)?.scheme == ContentResolver.SCHEME_CONTENT
+    }
+
     private fun showFatalError(action: String?) {
         Toast.makeText(this, "Oops! Something went wrong\n\n$action", Toast.LENGTH_LONG).show()
         finish()
@@ -391,5 +425,8 @@ class MainActivity : AppCompatActivity() {
 
         private const val PERMISSION_REQUEST_CODE = 0
         private const val SEEK_STEP_MS = 10_000L
+
+        /** Speed values the speed control cycles through, in order. */
+        private val PLAYBACK_SPEEDS = listOf(0.5f, 0.75f, 1f, 1.25f, 1.5f, 2f)
     }
 }

app/src/main/java/phone/vishnu/dialogmusicplayer/MainViewModel.kt

@@ -52,8 +52,16 @@ class MainViewModel(application: Application) : AndroidViewModel(application) {
         }
     }
 
-    fun onPlaybackStateChanged(playbackState: Int, positionMs: Long) {
-        _uiState.update { it.copy(playbackState = playbackState, positionMs = positionMs) }
+    fun onPlaybackStateChanged(playbackState: Int, positionMs: Long, playbackSpeed: Float) {
+        _uiState.update {
+            it.copy(
+                playbackState = playbackState,
+                positionMs = positionMs,
+                // The session reports speed 0 before playback starts; keep the
+                // last real speed so the speed control never shows a "0x" label.
+                playbackSpeed = if (playbackSpeed > 0f) playbackSpeed else it.playbackSpeed,
+            )
+        }
     }
 
     fun onRepeatModeChanged(repeatMode: Int) {

app/src/main/java/phone/vishnu/dialogmusicplayer/MediaPlaybackService.kt

@@ -61,7 +61,6 @@ class MediaPlaybackService :
     private lateinit var mediaPlayer: MediaPlayer
     private var audio: Audio? = null
 
-    private var isPlayingOnceInProgress = false
     private var wasPlayingWhenLosingAudioFocus = false
     private var isPlayerReleased = false
 
@@ -136,18 +135,12 @@ class MediaPlaybackService :
         setPlaybackState(PlaybackStateCompat.STATE_STOPPED, KEEP_SPEED)
         startForeground(NOTIFICATION_ID, getNotification())
 
+        // Single-track player: REPEAT_ONE and REPEAT_ALL both mean "replay this
+        // track". Replay on *every* completion — no every-other-time guard.
         when (mediaSession.controller.repeatMode) {
-            PlaybackStateCompat.REPEAT_MODE_ONE -> {
-                if (!isPlayingOnceInProgress) {
-                    isPlayingOnceInProgress = true
-                    if (mediaPlayer.currentPosition == mediaPlayer.duration) mediaPlayer.seekTo(0)
-                    mediaSession.controller.transportControls.play()
-                } else {
-                    isPlayingOnceInProgress = false
-                }
-            }
-
-            PlaybackStateCompat.REPEAT_MODE_ALL -> {
+            PlaybackStateCompat.REPEAT_MODE_ONE,
+            PlaybackStateCompat.REPEAT_MODE_ALL,
+            -> {
                 if (mediaPlayer.currentPosition == mediaPlayer.duration) mediaPlayer.seekTo(0)
                 mediaSession.controller.transportControls.play()
             }