feat: add song countdown for sleep timer, redesign sleep timer ui#3302
Closed
Ryfters wants to merge 6 commits intoMetrolistGroup:mainfrom
Closed
feat: add song countdown for sleep timer, redesign sleep timer ui#3302Ryfters wants to merge 6 commits intoMetrolistGroup:mainfrom
Ryfters wants to merge 6 commits intoMetrolistGroup:mainfrom
Conversation
…o feat/redesign-sleeptimer-ui # Conflicts: # app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…o feat/redesign-sleeptimer-ui
📝 WalkthroughWalkthroughRefactors sleep timer from boolean flags to a typed model (TIME, TIME_FINISH, SONGS), adds persisted default timer type, updates service/player integrations to use value+type, replaces inline dialogs with a new SleepTimerDialog UI, and adjusts crossfade/scheduling to account for remaining songs. Changes
Sequence DiagramsequenceDiagram
participant User
participant PlayerUI as Player UI
participant DataStore as Preference Store
participant MusicService
participant SleepTimer
User->>PlayerUI: Open SleepTimerDialog
PlayerUI->>DataStore: Read SleepTimerDefaultTypeKey & default value
DataStore-->>PlayerUI: Return default type/value
User->>PlayerUI: Select type & value (or set as default)
alt set as default
PlayerUI->>DataStore: Write SleepTimerDefaultTypeKey & default value
DataStore-->>PlayerUI: Ack
end
User->>PlayerUI: Confirm start
PlayerUI->>MusicService: sleepTimer.start(value, timerType)
MusicService->>SleepTimer: start(value, timerType)
alt timerType == SONGS
SleepTimer->>SleepTimer: remainingSongs = value, triggerTime = -1
else
SleepTimer->>SleepTimer: triggerTime = now + value.minutes
end
loop playback progression
MusicService->>SleepTimer: notifySongTransition()
SleepTimer->>SleepTimer: decrement remainingSongs if SONGS
SleepTimer-->>PlayerUI: update remaining display
end
alt remainingSongs == 0 or triggerTime elapsed
SleepTimer->>MusicService: completeTimerAndPause()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
isotjs
suggested changes
Mar 21, 2026
Comment on lines
+112
to
+115
| if (remainingSongs > 0) { | ||
| if (--remainingSongs == 0){ | ||
| completeTimerAndPause() | ||
| } |
Contributor
There was a problem hiding this comment.
notifySongTransition() already does the exact same decrement. If MusicService calls notifySongTransition() on song change and this listener might also fire, remainingSongs drops twice per transition. This is likely the root cause of the "manually skipping ends the timer early" bug you mentioned.
Author
There was a problem hiding this comment.
notifySongTransition() is only called on the crossfade transition, as this doesn't trigger onMediaItemTransition(). I meant to say that manually skipping correctly decrements the song counter, but when there are no more songs, it deactivates the sleep timer, but without pausing the song like it should.
Comment on lines
+119
to
+125
| // override fun onPlaybackStateChanged( | ||
| // @Player.State playbackState: Int, | ||
| // ) { | ||
| // if (playbackState == Player.STATE_ENDED && pauseWhenSongEnd) { | ||
| // completeTimerAndPause() | ||
| // } | ||
| // } |
Contributor
There was a problem hiding this comment.
This needs to come back, adapted for remainingSongs. When the playlist ends and STATE_ENDED fires, the timer is never cleared now.
| sleepTimerJob?.cancel() | ||
| sleepTimerJob = null | ||
| updateVolumeMultiplier(1f) | ||
| // updateVolumeMultiplier(1f) |
Contributor
There was a problem hiding this comment.
Commenting these out means if a previous session left the volume multiplier below 1.0 mid-fade, it never resets. These should stay active unconditionally, or at minimum be guarded with if (fadeOutEnabled) rather than removed entirely.
Comment on lines
+136
to
+158
| // private fun updateVolumeMultiplierForRemainingTime(remainingMs: Long) { | ||
| // updateVolumeMultiplier(volumeMultiplierForRemainingTime(remainingMs)) | ||
| // } | ||
|
|
||
| // private fun updateVolumeMultiplierForCurrentSong() { | ||
| // val duration = player.duration | ||
| // if (duration == C.TIME_UNSET || duration <= 0) { | ||
| // updateVolumeMultiplier(1f) | ||
| // return | ||
| // } | ||
| // | ||
| // val remainingMs = (duration - player.currentPosition).coerceAtLeast(0L) | ||
| // updateVolumeMultiplierForRemainingTime(remainingMs) | ||
| // } | ||
|
|
||
| // private fun volumeMultiplierForRemainingTime(remainingMs: Long): Float { | ||
| // if (remainingMs >= FADE_OUT_WINDOW_MS) return 1f | ||
| // return (remainingMs.toFloat() / FADE_OUT_WINDOW_MS).coerceIn(0f, 1f) | ||
| // } | ||
| // | ||
| // private fun updateVolumeMultiplier(multiplier: Float) { | ||
| // onVolumeMultiplierChanged(multiplier) | ||
| // } |
Contributor
There was a problem hiding this comment.
I understand fade-out is deferred, but commenting out the implementation while keeping fadeOutEnabled as a live field and FADE_OUT_WINDOW_MS as a constant leaves the class in an inconsistent state. Either keep the functions (even if not called yet) or mark the feature clearly as a stub.
Comment on lines
550
to
712
| // if (showSleepTimerDialog) { | ||
| // ActionPromptDialog( | ||
| // titleBar = { | ||
| // Row( | ||
| // modifier = Modifier.fillMaxWidth(), | ||
| // horizontalArrangement = Arrangement.Center, | ||
| // ) { | ||
| // Text( | ||
| // text = stringResource(R.string.sleep_timer), | ||
| // overflow = TextOverflow.Ellipsis, | ||
| // maxLines = 1, | ||
| // style = MaterialTheme.typography.headlineSmall, | ||
| // ) | ||
| // } | ||
| // }, | ||
| // onDismiss = { showSleepTimerDialog = false }, | ||
| // onConfirm = { | ||
| // showSleepTimerDialog = false | ||
| // playerConnection.service.sleepTimer.start( | ||
| // value = sleepTimerValue.roundToInt(), | ||
| // _type = sleepTimerType | ||
| //// fadeOut = sleepTimerFadeOut, | ||
| // ) | ||
| // }, | ||
| // onCancel = { | ||
| // showSleepTimerDialog = false | ||
| // }, | ||
| // onReset = { | ||
| // sleepTimerValue = sleepTimerDefault | ||
| // sleepTimerType = sleepTimerDefaultType | ||
| // }, | ||
| // content = { | ||
| // Column(horizontalAlignment = Alignment.CenterHorizontally) { | ||
| // | ||
| // Row( | ||
| // horizontalArrangement = Arrangement.spacedBy(2.dp), | ||
| // verticalAlignment = Alignment.CenterVertically, | ||
| // modifier = Modifier | ||
| // ) { | ||
| // OutlinedToggleButton( | ||
| // checked = sleepTimerType == SleepTimer.TIME, | ||
| // onCheckedChange = { | ||
| // if (sleepTimerType == SleepTimer.SONGS) sleepTimerValue*=5 | ||
| // sleepTimerType = 0 | ||
| // }, | ||
| // shapes = ButtonGroupDefaults.connectedLeadingButtonShapes(), | ||
| // colors = ToggleButtonDefaults.toggleButtonColors(), | ||
| // modifier = Modifier.weight(1f) | ||
| // ) { | ||
| // Text("Time") | ||
| // } | ||
| // OutlinedToggleButton( | ||
| // checked = sleepTimerType == SleepTimer.TIME_FINISH, | ||
| // onCheckedChange = { | ||
| // if (sleepTimerType == SleepTimer.SONGS) sleepTimerValue*=5 | ||
| // sleepTimerType = SleepTimer.TIME_FINISH | ||
| // }, | ||
| // shapes = ButtonGroupDefaults.connectedMiddleButtonShapes(), | ||
| // colors = ToggleButtonDefaults.toggleButtonColors(), | ||
| // modifier = Modifier.weight(1f) | ||
| // ) { | ||
| // Text("Time (FS)") | ||
| // } | ||
| // OutlinedToggleButton( | ||
| // checked = sleepTimerType == SleepTimer.SONGS, | ||
| // onCheckedChange = { | ||
| // if (sleepTimerType != SleepTimer.SONGS ) sleepTimerValue/=5 | ||
| // sleepTimerType = SleepTimer.SONGS | ||
| // }, | ||
| // shapes = ButtonGroupDefaults.connectedTrailingButtonShapes(), | ||
| // colors = ToggleButtonDefaults.toggleButtonColors(), | ||
| // modifier = Modifier.weight(1f) | ||
| // ) { | ||
| // Text("Songs") | ||
| // } | ||
| // } | ||
| // | ||
| // Text( | ||
| // text = "ⓘ FS: Finishes Song before stopping", | ||
| // style = MaterialTheme.typography.bodySmall, | ||
| // color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| // textAlign = TextAlign.End, | ||
| // modifier = Modifier.fillMaxWidth() | ||
| // ) | ||
| // | ||
| // Spacer(Modifier.height(16.dp)) | ||
| // | ||
| // Text( | ||
| // text = pluralStringResource( | ||
| // if (sleepTimerType == SleepTimer.SONGS) R.plurals.song else R.plurals.minute, | ||
| // sleepTimerValue.roundToInt(), | ||
| // sleepTimerValue.roundToInt(), | ||
| // ), | ||
| // style = MaterialTheme.typography.bodyLarge, | ||
| // ) | ||
| // | ||
| // Spacer(Modifier.height(8.dp)) | ||
| // | ||
| // val topValue = if (sleepTimerType == SleepTimer.SONGS) 24 else 120 | ||
| // val bottomValue = if (sleepTimerType == SleepTimer.SONGS) 1 else 5 | ||
| // Slider( | ||
| // value = sleepTimerValue, | ||
| // onValueChange = { sleepTimerValue = it }, | ||
| // valueRange = bottomValue.toFloat()..topValue.toFloat(), | ||
| // steps = (topValue - bottomValue) / bottomValue - 1, | ||
| // modifier = Modifier.fillMaxWidth(), | ||
| // ) | ||
| // | ||
| // Spacer(Modifier.height(8.dp)) | ||
| // | ||
| // Row( | ||
| // horizontalArrangement = Arrangement.spacedBy(8.dp), | ||
| // verticalAlignment = Alignment.CenterVertically, | ||
| // ) { | ||
| // if (isAtDefault) { | ||
| // val text = stringResource(R.string.already_set_as_default) | ||
| // Button( | ||
| // onClick = { | ||
| // Toast.makeText(context, text, Toast.LENGTH_SHORT).show() | ||
| // }, | ||
| // colors = ButtonDefaults.buttonColors( | ||
| // containerColor = MaterialTheme.colorScheme.primary, | ||
| // contentColor = MaterialTheme.colorScheme.onPrimary, | ||
| // ), | ||
| // ) { | ||
| // Text(stringResource(R.string.set_as_default)) | ||
| // } | ||
| // } else { | ||
| // OutlinedButton( | ||
| // onClick = { | ||
| // coroutineScope.launch { | ||
| // context.dataStore.edit { settings -> | ||
| // settings[SleepTimerDefaultKey] = sleepTimerValue | ||
| // settings[SleepTimerDefaultTypeKey] = sleepTimerType | ||
| // } | ||
| // } | ||
| // Toast.makeText( | ||
| // context, | ||
| // String.format(sleepTimerDefaultSetTemplate, sleepTimerValue.roundToInt()), | ||
| // Toast.LENGTH_SHORT, | ||
| // ).show() | ||
| // }, | ||
| // ) { | ||
| // Text(stringResource(R.string.set_as_default)) | ||
| // } | ||
| // } | ||
| // | ||
| //// OutlinedButton( | ||
| //// onClick = { | ||
| //// showSleepTimerDialog = false | ||
| //// playerConnection.service.sleepTimer.start( | ||
| //// minute = -1, | ||
| //// ) | ||
| //// }, | ||
| //// ) { | ||
| //// Text(stringResource(R.string.end_of_song)) | ||
| //// } | ||
| // } | ||
| // } | ||
| // }, | ||
| // ) | ||
| // } | ||
|
|
Contributor
There was a problem hiding this comment.
Both Player.kt and Queue.kt entry points should reflect the new state model consistently. Disabling one is a UX regression for users who open the timer from the player screen directly.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt (1)
183-188:⚠️ Potential issue | 🟡 MinorRemove duplicate imports.
These imports are duplicates of those already present earlier in the file (lines 115, 141-143, 170).
🧹 Proposed fix
-import com.metrolist.music.constants.SleepTimerDefaultKey -import com.metrolist.music.utils.dataStore -import androidx.datastore.preferences.core.edit -import com.metrolist.music.constants.SleepTimerFadeOutKey -import com.metrolist.music.constants.SleepTimerStopAfterCurrentSongKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt` around lines 183 - 188, Remove the duplicate imports listed in the diff (SleepTimerDefaultKey, dataStore, edit, SleepTimerFadeOutKey, SleepTimerStopAfterCurrentSongKey) so each symbol is imported only once; locate the repeated import statements in Player.kt and delete the redundant import block leaving the original import declarations earlier in the file intact.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt (1)
48-84: Consider renaming_typeparameter for clarity.The underscore prefix is unconventional in Kotlin. Consider
timerTypeorsleepTimerTypeto avoid shadowing the class property while maintaining readability.♻️ Suggested improvement
fun start( value: Int, - _type: Int, + timerType: Int, fadeOut: Boolean = false, ) { sleepTimerJob?.cancel() sleepTimerJob = null fadeOutEnabled = fadeOut - type = _type + type = timerType - if (type == SONGS) { + if (timerType == SONGS) { // ... } else { // ... - if (type == TIME_FINISH) { + if (timerType == TIME_FINISH) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt` around lines 48 - 84, Rename the confusing parameter `_type` in the SleepTimer.start function to a clearer name (e.g., timerType or sleepTimerType) to avoid the unconventional underscore prefix and shadowing with the class property `type`; update all references inside `start` (including the assignment `type = _type` and any comparisons against `SONGS` and `TIME_FINISH`) to use the new parameter name so behavior remains the same.app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt (1)
594-619: Consider clampingsleepTimerValueafter type conversion.The conversion logic multiplies/divides by 5 when switching modes. While the slider will eventually constrain the value, there's a brief moment where
sleepTimerValuecould be outside the new mode's valid range. Adding explicit clamping ensures consistent state.♻️ Suggested improvement
OutlinedToggleButton( checked = sleepTimerType == SleepTimer.TIME, onCheckedChange = { - if (sleepTimerType == SleepTimer.SONGS) sleepTimerValue*=5 + if (sleepTimerType == SleepTimer.SONGS) { + sleepTimerValue = (sleepTimerValue * 5).coerceIn(5f, 120f) + } sleepTimerType = SleepTimer.TIME }, // ... ) // Apply similar pattern to other toggle buttons🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt` around lines 594 - 619, When switching sleepTimerType in the OutlinedToggleButton onCheckedChange handlers, clamp the adjusted sleepTimerValue to the valid range for the target mode to avoid briefly holding an out-of-range value; update the three onCheckedChange blocks that modify sleepTimerValue (the handlers that multiply/divide by 5 when setting sleepTimerType to SleepTimer.TIME, SleepTimer.TIME_FINISH, or SleepTimer.SONGS) to call a clamp/coerceIn using the min/max bounds used by the slider that controls sleepTimerValue so the value is immediately constrained to the new mode's valid range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 480-482: Persisted SleepTimerDefaultTypeKey may contain invalid
ints; before calling start(...) validate and normalize the retrieved value. Read
the raw int from dataStore into a local (e.g., sleepTimerDefaultTypeRaw),
map/validate it against the SleepTimer enum/constants (e.g., ensure it equals a
known value like SleepTimer.TIME or other supported types) and fall back to a
safe default (SleepTimer.TIME) if out of range or unknown; then pass the
normalized value into start(...). Apply the same validation logic for the other
occurrence around the code referenced at lines 562-563 so both start(...) calls
use a validated SleepTimer type.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt`:
- Around line 591-627: Replace the hardcoded labels in the three
OutlinedToggleButton composables and any related info text with
stringResource(...) lookups: change the "Time", "Time (FS)", and "Songs" Text()
calls to use resources (e.g. R.string.sleep_timer_time,
R.string.sleep_timer_time_finish, R.string.sleep_timer_songs) and update any
other hardcoded info text nearby to use string resources as well; add
corresponding entries to strings.xml (with appropriate keys and translations)
and import androidx.compose.ui.res.stringResource where needed so the UI uses
localized strings while leaving logic around sleepTimerType / SleepTimer and
sleepTimerValue untouched.
In `@app/src/main/res/values/metrolist_strings.xml`:
- Line 253: Replace the single string resource sleep_timer_default_set_songs
with an Android plurals resource (name it e.g. sleep_timer_default_set_songs)
that defines quantity="one" and quantity="other" forms, then update the code
that displays this message to use
Resources.getQuantityString(R.plurals.sleep_timer_default_set_songs, count,
count) (or context.resources.getQuantityString(...) /
resources.getQuantityString(...) in Kotlin) passing the song count as both the
quantity and the format argument so "1 song" and "N songs" render correctly.
In `@app/src/main/res/values/strings.xml`:
- Around line 211-214: The plural resource "song" was added to the wrong file
and its other form has a stray quote; remove the <plurals name="song"> entry
from strings.xml and add it into the Metrolist resource file as a corrected
plural resource: <plurals name="song"> with <item quantity="one">1 song</item>
and <item quantity="other">%d songs</item> (no trailing quote); ensure you only
modify the Metrolist/app/src/main/res/values/metrolist_strings.xml when adding
the corrected resource and do not change other strings.xml or
metrolist_strings.xml files.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt`:
- Around line 183-188: Remove the duplicate imports listed in the diff
(SleepTimerDefaultKey, dataStore, edit, SleepTimerFadeOutKey,
SleepTimerStopAfterCurrentSongKey) so each symbol is imported only once; locate
the repeated import statements in Player.kt and delete the redundant import
block leaving the original import declarations earlier in the file intact.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt`:
- Around line 48-84: Rename the confusing parameter `_type` in the
SleepTimer.start function to a clearer name (e.g., timerType or sleepTimerType)
to avoid the unconventional underscore prefix and shadowing with the class
property `type`; update all references inside `start` (including the assignment
`type = _type` and any comparisons against `SONGS` and `TIME_FINISH`) to use the
new parameter name so behavior remains the same.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt`:
- Around line 594-619: When switching sleepTimerType in the OutlinedToggleButton
onCheckedChange handlers, clamp the adjusted sleepTimerValue to the valid range
for the target mode to avoid briefly holding an out-of-range value; update the
three onCheckedChange blocks that modify sleepTimerValue (the handlers that
multiply/divide by 5 when setting sleepTimerType to SleepTimer.TIME,
SleepTimer.TIME_FINISH, or SleepTimer.SONGS) to call a clamp/coerceIn using the
min/max bounds used by the slider that controls sleepTimerValue so the value is
immediately constrained to the new mode's valid range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 814ca679-8638-4806-a310-9dfe54ddb80c
📒 Files selected for processing (8)
app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktapp/src/main/kotlin/com/metrolist/music/playback/SleepTimer.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Player.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Queue.ktapp/src/main/res/values/metrolist_strings.xmlapp/src/main/res/values/strings.xml
| <string name="set_as_default">Set as default</string> | ||
| <string name="already_set_as_default">Already set as default</string> | ||
| <string name="sleep_timer_default_set">Sleep timer default set to %d min</string> | ||
| <string name="sleep_timer_default_set_songs">Sleep timer default set to %d songs</string> |
There was a problem hiding this comment.
Use pluralization for the songs default message.
"%d songs" will render incorrectly for 1. Please switch this to a plural resource and fetch it with quantity.
💡 Proposed fix
- <string name="sleep_timer_default_set_songs">Sleep timer default set to %d songs</string>
+ <plurals name="sleep_timer_default_set_songs">
+ <item quantity="one">Sleep timer default set to %d song</item>
+ <item quantity="other">Sleep timer default set to %d songs</item>
+ </plurals>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="sleep_timer_default_set_songs">Sleep timer default set to %d songs</string> | |
| <plurals name="sleep_timer_default_set_songs"> | |
| <item quantity="one">Sleep timer default set to %d song</item> | |
| <item quantity="other">Sleep timer default set to %d songs</item> | |
| </plurals> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values/metrolist_strings.xml` at line 253, Replace the
single string resource sleep_timer_default_set_songs with an Android plurals
resource (name it e.g. sleep_timer_default_set_songs) that defines
quantity="one" and quantity="other" forms, then update the code that displays
this message to use
Resources.getQuantityString(R.plurals.sleep_timer_default_set_songs, count,
count) (or context.resources.getQuantityString(...) /
resources.getQuantityString(...) in Kotlin) passing the song count as both the
quantity and the format argument so "1 song" and "N songs" render correctly.
Comment on lines
+211
to
+214
| <plurals name="song"> | ||
| <item quantity="one">1 song</item> | ||
| <item quantity="other">%d songs"</item> | ||
| </plurals> |
There was a problem hiding this comment.
Move this resource out of upstream strings.xml and fix the value typo.
This new feature string is added in the wrong file, and the other form includes an extra trailing quote (%d songs").
💡 Proposed fix
- <plurals name="song">
- <item quantity="one">1 song</item>
- <item quantity="other">%d songs"</item>
- </plurals>Then add the corrected resource under app/src/main/res/values/metrolist_strings.xml instead.
Based on learnings: All string edits must be made to the Metrolist/app/src/main/res/values/metrolist_strings.xml file, NOT Metrolist/app/src/main/res/values/strings.xml. Do not touch other strings.xml or metrolist_strings.xml files in the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values/strings.xml` around lines 211 - 214, The plural
resource "song" was added to the wrong file and its other form has a stray
quote; remove the <plurals name="song"> entry from strings.xml and add it into
the Metrolist resource file as a corrected plural resource: <plurals
name="song"> with <item quantity="one">1 song</item> and <item
quantity="other">%d songs</item> (no trailing quote); ensure you only modify the
Metrolist/app/src/main/res/values/metrolist_strings.xml when adding the
corrected resource and do not change other strings.xml or metrolist_strings.xml
files.
Member
|
DO NOT TOUCH strings.xml!!!!!!!!! |
Collaborator
|
I think they touched strings.xml... 🧐 |
Author
|
oops... 🫣 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt (1)
90-116:⚠️ Potential issue | 🔴 CriticalCritical: Double decrement of
remainingSongscauses timer to end prematurely.Both
notifySongTransition()(called byMusicServiceduring crossfade) andonMediaItemTransition()(player listener callback) decrementremainingSongs. When a normal song transition occurs,onMediaItemTransitionfires. When crossfade triggers,MusicServicecallsnotifySongTransition(). However, if both fire for the same transition, or if a manual skip triggers both paths,remainingSongsdrops twice.This is likely the root cause of the known bug where manually skipping with 1 song remaining ends the timer without pausing.
🐛 Proposed fix: guard against double-decrement per transition
+ private var lastTransitionTimestamp = 0L + private val transitionDebounceMs = 100L + fun notifySongTransition() { - if (remainingSongs > 0) { - if (--remainingSongs == 0){ + val now = System.currentTimeMillis() + if (remainingSongs > 0 && (now - lastTransitionTimestamp) > transitionDebounceMs) { + lastTransitionTimestamp = now + if (--remainingSongs == 0) { completeTimerAndPause() } } } // ... override fun onMediaItemTransition( mediaItem: MediaItem?, reason: Int, ) { - if (remainingSongs > 0) { - if (--remainingSongs == 0){ + val now = System.currentTimeMillis() + if (remainingSongs > 0 && (now - lastTransitionTimestamp) > transitionDebounceMs) { + lastTransitionTimestamp = now + if (--remainingSongs == 0) { completeTimerAndPause() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt` around lines 90 - 116, notifySongTransition() and onMediaItemTransition(...) both decrement remainingSongs causing double-decrements; fix by ensuring each physical transition only decrements once: add a small transition-identity guard (e.g., a field like lastHandledMediaId or lastHandledTransitionTimestamp) and in both notifySongTransition() and onMediaItemTransition() check that the current transition identity differs from lastHandled before doing --remainingSongs and calling completeTimerAndPause(); update lastHandled after you decrement. Reference symbols: notifySongTransition, onMediaItemTransition, remainingSongs, completeTimerAndPause.
🧹 Nitpick comments (7)
app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt (3)
534-534: Consider consolidating coroutine scope declarations.There are two
rememberCoroutineScope()calls in this composable (lines 206 and 534). While functionally equivalent, consolidating to a single scope improves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt` at line 534, The composable currently calls rememberCoroutineScope() twice; consolidate to a single coroutine scope by declaring one rememberCoroutineScope() (e.g., val scope = rememberCoroutineScope()) in the top-level of the Player composable (the existing declaration near line ~206) and remove the second declaration (the one at the later site referencing val scope = rememberCoroutineScope()); update any usages to reference that single scope variable so all launches use the same scope.
608-644: Value conversion when switching types may produce unexpected results.When switching from time to songs mode,
sleepTimerValue /= 5could produce fractional values (e.g., 7 minutes → 1.4 songs). While the slider clamps tobottomValue=1, the intermediate float value persists until user interaction. Consider rounding or clamping immediately:♻️ Clamp value after conversion
OutlinedToggleButton( checked = sleepTimerType == SleepTimer.SONGS, onCheckedChange = { - if (sleepTimerType != SleepTimer.SONGS ) sleepTimerValue/=5 + if (sleepTimerType != SleepTimer.SONGS) { + sleepTimerValue = (sleepTimerValue / 5).coerceIn(1f, 24f) + } sleepTimerType = SleepTimer.SONGS },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt` around lines 608 - 644, The sleep-timer value conversion can produce fractional values when toggling types (sleepTimerType, SleepTimer.SONGS, SleepTimer.TIME, TIME_FINISH, and sleepTimerValue), so update the toggle handlers to normalize the value immediately: when switching to SleepTimer.SONGS divide the current value by 5 then round (e.g., round or ceil) and clamp to a minimum of 1 (and cast to an integer/count type if you use integers for songs); when switching from SONGS to a time mode multiply and clamp/round as needed; apply the normalization in the onCheckedChange blocks for the three OutlinedToggleButton handlers so the stored sleepTimerValue is always valid and non-fractional for SONGS and stays within allowed bounds.
548-553: Verify thatsleepTimerEnabledrecomposes correctly.Using
remember()with the timer's state properties as keys should work, but consider usingderivedStateOffor consistency with other derived state in this file (likeisAtDefault). The current approach relies on Compose runtime snapshot state fromSleepTimer, which should trigger recomposition, but the explicitrememberkeys may cause unnecessary recompositions.♻️ Alternative using derivedStateOf
- val sleepTimerEnabled = remember( - playerConnection.service.sleepTimer.triggerTime, - playerConnection.service.sleepTimer.remainingSongs - ) { - playerConnection.service.sleepTimer.isActive - } + val sleepTimerEnabled by remember { + derivedStateOf { playerConnection.service.sleepTimer.isActive } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt` around lines 548 - 553, The current remember(...) block that computes sleepTimerEnabled from playerConnection.service.sleepTimer properties can cause unnecessary recompositions; replace it with a derived state to mirror other derived state usages (like isAtDefault). Specifically, change the sleepTimerEnabled declaration to use derivedStateOf { playerConnection.service.sleepTimer.isActive } (or a derived expression that reads triggerTime/remainingSongs if you need them) so Compose tracks the underlying snapshot reads instead of relying on explicit remember keys; keep the same variable name sleepTimerEnabled and reference playerConnection.service.sleepTimer to locate the code.app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt (3)
614-619: Same value conversion edge case as in Player.kt.The
/5conversion when switching to songs mode can produce fractional values. Apply the same fix recommended forPlayer.kt(coerce to valid range after conversion).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt` around lines 614 - 619, When switching to Songs mode in the OutlinedToggleButton handler (checked = sleepTimerType == SleepTimer.SONGS) the current division sleepTimerValue /= 5 can produce non-integer/invalid values; after converting (divide by 5) coerce the result into the valid range for song counts (use the same clamping logic used in Player.kt) and then assign sleepTimerType = SleepTimer.SONGS so sleepTimerValue is always within allowed bounds; locate the OutlinedToggleButton onCheckedChange block and replace the raw division with convert-then-coerce-to-valid-range logic referencing sleepTimerValue and SleepTimer.SONGS.
336-336: Consider adding unit indicator to sleep timer remaining display.When displaying
sleepTimerRemainingfor songs mode, it shows just a number (e.g., "5"). Users may not immediately understand this represents songs remaining vs. minutes. Consider adding a suffix or icon to differentiate.♻️ Example with unit indicator
text = if (sleepTimerEnabled) { if (playerConnection.service.sleepTimer.type == SleepTimer.SONGS) { "♫$sleepTimerRemaining" // or use an icon } else { sleepTimerRemaining } } else null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt` at line 336, The sleep timer label currently shows only sleepTimerRemaining which is ambiguous; update the expression assigning text (the line using sleepTimerEnabled and sleepTimerRemaining) to append a unit or icon when playerConnection.service.sleepTimer.type == SleepTimer.SONGS (e.g., add a "♫" prefix/suffix or "songs" suffix) so song-mode shows a clear indicator while keeping the existing value for minute mode; change the inline conditional at text = ... to branch on SleepTimer.SONGS and return the modified string when enabled, otherwise return the original sleepTimerRemaining or null.
582-706: Recommend extracting shared sleep timer dialog to a reusable component.The sleep timer dialog implementation in
Queue.ktis nearly identical toPlayer.kt. Consider extracting to a shared composable (e.g.,SleepTimerDialog) to reduce duplication and ensure future changes stay synchronized.♻️ Example extraction
Create a new composable in a shared location:
`@Composable` fun SleepTimerDialog( sleepTimerValue: Float, onSleepTimerValueChange: (Float) -> Unit, sleepTimerType: Int, onSleepTimerTypeChange: (Int) -> Unit, sleepTimerDefault: Float, sleepTimerDefaultType: Int, isAtDefault: Boolean, onDismiss: () -> Unit, onConfirm: () -> Unit, ) { // Shared implementation }Then both
Player.ktandQueue.ktcan use this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt` around lines 582 - 706, The sleep-timer UI block in Queue.kt (the Column starting at content = { ... } using sleepTimerValue, sleepTimerType, isAtDefault, coroutineScope, SleepTimerDefaultKey, SleepTimerDefaultTypeKey, and context) is duplicated in Player.kt; extract it into a shared composable like SleepTimerDialog(...) that takes sleepTimerValue, onSleepTimerValueChange, sleepTimerType, onSleepTimerTypeChange, isAtDefault, onSetAsDefault (or onConfirm) and onDismiss callbacks (or pass context if needed), move the Slider/Buttons/Toast/datastore edit logic into that composable or into the onSetAsDefault callback, and replace the inline Column blocks in both Queue.kt and Player.kt with calls to SleepTimerDialog to remove duplication and keep behavior identical.app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt (1)
43-45: Acknowledged:fadeOutEnabledis a placeholder.The comment clarifies fade-out is disabled. Consider removing this field entirely if fade-out won't be implemented soon, or add a TODO comment with a tracking issue reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt` around lines 43 - 45, The placeholder property fadeOutEnabled (declared as var fadeOutEnabled by mutableStateOf(false) in SleepTimer) should not remain unused: if no fade-out feature will be implemented soon, remove the fadeOutEnabled property and any references to it (search for fadeOutEnabled in SleepTimer and callers) to avoid dead state; otherwise keep the property but replace the inline comment with a TODO that references the tracking issue/ID (e.g., TODO: implement fade-out — ISSUE-123) and document why it's disabled, so intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 563-564: The log message is misleading when sleepTimerDefaultType
== SONGS because sleepTimerDefaultValue represents a song count, not minutes;
update the logging call (using TAG, sleepTimerDefaultValue,
sleepTimerDefaultType and the call to service.sleepTimer.start) to conditionally
format the message—e.g., log "AUTO SLEEP TIMER STARTED: X minutes" when type is
MINUTES and "AUTO SLEEP TIMER STARTED: X songs" when type is SONGS—so the log
matches the unit passed to service.sleepTimer.start.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt`:
- Around line 90-116: notifySongTransition() and onMediaItemTransition(...) both
decrement remainingSongs causing double-decrements; fix by ensuring each
physical transition only decrements once: add a small transition-identity guard
(e.g., a field like lastHandledMediaId or lastHandledTransitionTimestamp) and in
both notifySongTransition() and onMediaItemTransition() check that the current
transition identity differs from lastHandled before doing --remainingSongs and
calling completeTimerAndPause(); update lastHandled after you decrement.
Reference symbols: notifySongTransition, onMediaItemTransition, remainingSongs,
completeTimerAndPause.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt`:
- Around line 43-45: The placeholder property fadeOutEnabled (declared as var
fadeOutEnabled by mutableStateOf(false) in SleepTimer) should not remain unused:
if no fade-out feature will be implemented soon, remove the fadeOutEnabled
property and any references to it (search for fadeOutEnabled in SleepTimer and
callers) to avoid dead state; otherwise keep the property but replace the inline
comment with a TODO that references the tracking issue/ID (e.g., TODO: implement
fade-out — ISSUE-123) and document why it's disabled, so intent is clear.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt`:
- Line 534: The composable currently calls rememberCoroutineScope() twice;
consolidate to a single coroutine scope by declaring one
rememberCoroutineScope() (e.g., val scope = rememberCoroutineScope()) in the
top-level of the Player composable (the existing declaration near line ~206) and
remove the second declaration (the one at the later site referencing val scope =
rememberCoroutineScope()); update any usages to reference that single scope
variable so all launches use the same scope.
- Around line 608-644: The sleep-timer value conversion can produce fractional
values when toggling types (sleepTimerType, SleepTimer.SONGS, SleepTimer.TIME,
TIME_FINISH, and sleepTimerValue), so update the toggle handlers to normalize
the value immediately: when switching to SleepTimer.SONGS divide the current
value by 5 then round (e.g., round or ceil) and clamp to a minimum of 1 (and
cast to an integer/count type if you use integers for songs); when switching
from SONGS to a time mode multiply and clamp/round as needed; apply the
normalization in the onCheckedChange blocks for the three OutlinedToggleButton
handlers so the stored sleepTimerValue is always valid and non-fractional for
SONGS and stays within allowed bounds.
- Around line 548-553: The current remember(...) block that computes
sleepTimerEnabled from playerConnection.service.sleepTimer properties can cause
unnecessary recompositions; replace it with a derived state to mirror other
derived state usages (like isAtDefault). Specifically, change the
sleepTimerEnabled declaration to use derivedStateOf {
playerConnection.service.sleepTimer.isActive } (or a derived expression that
reads triggerTime/remainingSongs if you need them) so Compose tracks the
underlying snapshot reads instead of relying on explicit remember keys; keep the
same variable name sleepTimerEnabled and reference
playerConnection.service.sleepTimer to locate the code.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Queue.kt`:
- Around line 614-619: When switching to Songs mode in the OutlinedToggleButton
handler (checked = sleepTimerType == SleepTimer.SONGS) the current division
sleepTimerValue /= 5 can produce non-integer/invalid values; after converting
(divide by 5) coerce the result into the valid range for song counts (use the
same clamping logic used in Player.kt) and then assign sleepTimerType =
SleepTimer.SONGS so sleepTimerValue is always within allowed bounds; locate the
OutlinedToggleButton onCheckedChange block and replace the raw division with
convert-then-coerce-to-valid-range logic referencing sleepTimerValue and
SleepTimer.SONGS.
- Line 336: The sleep timer label currently shows only sleepTimerRemaining which
is ambiguous; update the expression assigning text (the line using
sleepTimerEnabled and sleepTimerRemaining) to append a unit or icon when
playerConnection.service.sleepTimer.type == SleepTimer.SONGS (e.g., add a "♫"
prefix/suffix or "songs" suffix) so song-mode shows a clear indicator while
keeping the existing value for minute mode; change the inline conditional at
text = ... to branch on SleepTimer.SONGS and return the modified string when
enabled, otherwise return the original sleepTimerRemaining or null.
- Around line 582-706: The sleep-timer UI block in Queue.kt (the Column starting
at content = { ... } using sleepTimerValue, sleepTimerType, isAtDefault,
coroutineScope, SleepTimerDefaultKey, SleepTimerDefaultTypeKey, and context) is
duplicated in Player.kt; extract it into a shared composable like
SleepTimerDialog(...) that takes sleepTimerValue, onSleepTimerValueChange,
sleepTimerType, onSleepTimerTypeChange, isAtDefault, onSetAsDefault (or
onConfirm) and onDismiss callbacks (or pass context if needed), move the
Slider/Buttons/Toast/datastore edit logic into that composable or into the
onSetAsDefault callback, and replace the inline Column blocks in both Queue.kt
and Player.kt with calls to SleepTimerDialog to remove duplication and keep
behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4009d816-4250-4625-8ab6-b293ee1abb2e
📒 Files selected for processing (5)
app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktapp/src/main/kotlin/com/metrolist/music/playback/SleepTimer.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Player.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Queue.ktapp/src/main/res/values/metrolist_strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/res/values/metrolist_strings.xml
- unedited strings.xml (moved to metrolist_strings.xml) - rename SleepTimer.start() _type param to timerType - pluralised sleep_timer_default_set_songs string - removed unused/unworking logic for fadeout - set the volume multiplier back to 1 - clear unused imports - ensure sleepTimerDefaultType is valid - clear sleep timer songs on playlist end - stringified hardcoded strings - fixed log message - extracted sleep timer dialog to a new component
Ryfters
force-pushed
the
feat/redesign-sleeptimer-ui
branch
from
5fd1c22 to
cd714aa
Compare
Ryfters
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt (1)
533-535:⚠️ Potential issue | 🟠 MajorPlayer screen still can't open the sleep timer dialog.
showSleepTimerDialogis only declared and consumed here; nothing inBottomSheetPlayerever sets it totrue. That leaves this dialog unreachable from the player screen, whileQueue.ktstill has the working entry point. Please wire the player-side action back to this state or remove the dead dialog until that path is restored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt` around lines 533 - 535, The sleep timer dialog is never opened because showSleepTimerDialog is only declared and passed to SleepTimerDialog but never set to true by BottomSheetPlayer; either remove the dead dialog or restore the player-side trigger by wiring an opener callback into BottomSheetPlayer. Modify BottomSheetPlayer to accept a lambda like onOpenSleepTimer: () -> Unit (or a MutableState<Boolean> setter) and call that lambda from the UI action that should open the timer; in Player.kt set showSleepTimerDialog = true in that callback (or pass showSleepTimerDialog setter), or alternatively remove the SleepTimerDialog/SleepTimerDialog(...) invocation if you prefer to keep the dialog unreachable until the path is reintroduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt`:
- Around line 111-114: When the sleep timer completes (completeTimerAndPause())
set a durable flag (e.g., timerCompleted or suppressPlayOnSkip) in SleepTimer
and expose a read-only accessor (or callback) so PlayerConnection can consult
it; then change PlayerConnection.seekToNext() and seekToPrevious() to check that
flag before forcing player.playWhenReady = true and skip re-enabling playback if
the timer has completed. Ensure the flag is set to true inside
completeTimerAndPause() and cleared appropriately when the timer is reset or
restarted.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/SleepTimerDialog.kt`:
- Around line 63-70: The local dialog state (sleepTimerDefault, sleepTimerValue,
sleepTimerDefaultType, sleepTimerType) is declared outside the dialog visibility
check so canceled edits persist across closes; reinitialize or recreate those
states when the dialog is opened by scoping them to the `if (show)` block or by
using a side-effect keyed on `show` (e.g., LaunchedEffect(show) or remember(key
= show)) to reset sleepTimerValue from rememberPreference(SleepTimerDefaultKey)
and sleepTimerType from rememberPreference(SleepTimerDefaultTypeKey) whenever
show becomes true so the dialog always starts with the stored defaults.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Player.kt`:
- Around line 533-535: The sleep timer dialog is never opened because
showSleepTimerDialog is only declared and passed to SleepTimerDialog but never
set to true by BottomSheetPlayer; either remove the dead dialog or restore the
player-side trigger by wiring an opener callback into BottomSheetPlayer. Modify
BottomSheetPlayer to accept a lambda like onOpenSleepTimer: () -> Unit (or a
MutableState<Boolean> setter) and call that lambda from the UI action that
should open the timer; in Player.kt set showSleepTimerDialog = true in that
callback (or pass showSleepTimerDialog setter), or alternatively remove the
SleepTimerDialog/SleepTimerDialog(...) invocation if you prefer to keep the
dialog unreachable until the path is reintroduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0795c79-6a77-4c27-b25a-70edd25e87f3
📒 Files selected for processing (6)
app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktapp/src/main/kotlin/com/metrolist/music/playback/SleepTimer.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Player.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Queue.ktapp/src/main/kotlin/com/metrolist/music/ui/player/SleepTimerDialog.ktapp/src/main/res/values/metrolist_strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/res/values/metrolist_strings.xml
Comment on lines
+111
to
+114
| if (remainingSongs > 0) { | ||
| if (--remainingSongs == 0){ | ||
| completeTimerAndPause() | ||
| } |
There was a problem hiding this comment.
Manual skip still overrides the final-song pause.
When remainingSongs reaches zero here, completeTimerAndPause() pauses the player, but the manual skip paths in PlayerConnection.seekToNext() / seekToPrevious() still force player.playWhenReady = true after the seek. That is why "1 song remaining + skip" keeps playing. The completion path needs to stop those handlers from re-enabling playback once the timer has fired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.kt` around lines
111 - 114, When the sleep timer completes (completeTimerAndPause()) set a
durable flag (e.g., timerCompleted or suppressPlayOnSkip) in SleepTimer and
expose a read-only accessor (or callback) so PlayerConnection can consult it;
then change PlayerConnection.seekToNext() and seekToPrevious() to check that
flag before forcing player.playWhenReady = true and skip re-enabling playback if
the timer has completed. Ensure the flag is set to true inside
completeTimerAndPause() and cleared appropriately when the timer is reset or
restarted.
Comment on lines
+63
to
+70
| val sleepTimerDefault by rememberPreference(SleepTimerDefaultKey, 30f) | ||
| var sleepTimerValue by remember { mutableFloatStateOf(sleepTimerDefault) } | ||
| val sleepTimerDefaultType by rememberPreference(SleepTimerDefaultTypeKey, SleepTimer.TIME) | ||
| var sleepTimerType by remember { | ||
| mutableIntStateOf(sleepTimerDefaultType.takeIf { | ||
| it in listOf(SleepTimer.TIME, TIME_FINISH, SleepTimer.SONGS) | ||
| } ?: SleepTimer.TIME) | ||
| } |
There was a problem hiding this comment.
Canceled edits persist across dialog reopen.
These local states live outside if (show), so dismissing the dialog does not discard unsaved changes. Reopening it can start the last canceled value/type instead of the current stored default. Reinitialize them when show becomes true.
🔧 One way to resync the dialog on open
val sleepTimerDefault by rememberPreference(SleepTimerDefaultKey, 30f)
var sleepTimerValue by remember { mutableFloatStateOf(sleepTimerDefault) }
val sleepTimerDefaultType by rememberPreference(SleepTimerDefaultTypeKey, SleepTimer.TIME)
-var sleepTimerType by remember {
- mutableIntStateOf(sleepTimerDefaultType.takeIf {
- it in listOf(SleepTimer.TIME, TIME_FINISH, SleepTimer.SONGS)
- } ?: SleepTimer.TIME)
-}
+val normalizedDefaultType =
+ sleepTimerDefaultType.takeIf {
+ it in listOf(SleepTimer.TIME, TIME_FINISH, SleepTimer.SONGS)
+ } ?: SleepTimer.TIME
+var sleepTimerType by remember { mutableIntStateOf(normalizedDefaultType) }
+
+LaunchedEffect(show, sleepTimerDefault, normalizedDefaultType) {
+ if (show) {
+ sleepTimerValue = sleepTimerDefault
+ sleepTimerType = normalizedDefaultType
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val sleepTimerDefault by rememberPreference(SleepTimerDefaultKey, 30f) | |
| var sleepTimerValue by remember { mutableFloatStateOf(sleepTimerDefault) } | |
| val sleepTimerDefaultType by rememberPreference(SleepTimerDefaultTypeKey, SleepTimer.TIME) | |
| var sleepTimerType by remember { | |
| mutableIntStateOf(sleepTimerDefaultType.takeIf { | |
| it in listOf(SleepTimer.TIME, TIME_FINISH, SleepTimer.SONGS) | |
| } ?: SleepTimer.TIME) | |
| } | |
| val sleepTimerDefault by rememberPreference(SleepTimerDefaultKey, 30f) | |
| var sleepTimerValue by remember { mutableFloatStateOf(sleepTimerDefault) } | |
| val sleepTimerDefaultType by rememberPreference(SleepTimerDefaultTypeKey, SleepTimer.TIME) | |
| val normalizedDefaultType = | |
| sleepTimerDefaultType.takeIf { | |
| it in listOf(SleepTimer.TIME, TIME_FINISH, SleepTimer.SONGS) | |
| } ?: SleepTimer.TIME | |
| var sleepTimerType by remember { mutableIntStateOf(normalizedDefaultType) } | |
| LaunchedEffect(show, sleepTimerDefault, normalizedDefaultType) { | |
| if (show) { | |
| sleepTimerValue = sleepTimerDefault | |
| sleepTimerType = normalizedDefaultType | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/SleepTimerDialog.kt` around
lines 63 - 70, The local dialog state (sleepTimerDefault, sleepTimerValue,
sleepTimerDefaultType, sleepTimerType) is declared outside the dialog visibility
check so canceled edits persist across closes; reinitialize or recreate those
states when the dialog is opened by scoping them to the `if (show)` block or by
using a side-effect keyed on `show` (e.g., LaunchedEffect(show) or remember(key
= show)) to reset sleepTimerValue from rememberPreference(SleepTimerDefaultKey)
and sleepTimerType from rememberPreference(SleepTimerDefaultTypeKey) whenever
show becomes true so the dialog always starts with the stored defaults.
Contributor
|
Redesign the buttons to match the apps standards |
Member
|
Stale + unwanted kinda |
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
4 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.
Add song countown, and redesign ui
Changes
Testing
Tested all new options, with and without crossfade
Known bug: If a user manually goes to the next song when only 1 song remains, it will end the timer without pausing.
For maintainers
Summary by CodeRabbit
New Features
Refactor
Behavior Changes
Localization