Fix: Sleep timer stops podcast after given time #3377

mebarbosa · 2024-12-17T15:32:05Z

Description

This is the alternative approach to fix the sleep timer issue. The first attempt was this one Fix: Sleep timer stops podcast after given time #3364 but we decided to not follow the approach of using Alarm clock for sleep timer.
This PR includes a refactor in Sleep Timer feature. This is not the final version of Sleep Timer, I can see a room for improvement, but I propose to do this in incremental parts.
Some important changes:
- Moved the Sleep timer to PlaybackService instead of using Alarm Clock for it: 5f1e1ad
- Extracted Sleep timer state to SleepTimer class: 727f292
- Extracted the volume fade out to a new class f4f7e20. I also implemented the same logic that iOS

Fixes: #3355
Fixes: #2219

Testing Instructions

We need to regression test the whole sleep timer feature. I am going to describe all scenarios we should cover, but feel free to try more in case I miss anything.

Sleep Timer for X minutes

Smoke test the X minutes
✅ Ensure sleep timer finishes in X minutes you set
✅ Ensure you hear the fade out volume when there is about 5 seconds to end the sleep timer

Sleep Timer for end of episodes

Smoke test the end of episodes
✅ Ensure you don't hear the fade out volume

Sleep Timer for end of chapter

Smoke test the end of chapter
Note: Every time we change to another chapter or change to another episode, we consider that we ended the episode, so it will decrease the remaining number of chapters left

Auto Restart Sleep Timer

Restarting timer

Run the app
Add 5 to 10 podcasts to your Up Next
Play something
Open the full player, tap the sleep timer icon (zZz), select X minutes (You can select 1 minute using the last option)
After X minutes the playback should stop
Tap play again
✅ The sleep timer should restart with X minutes you set
You can also try this for end of episode and end of chapter

Not restarting timer

You can change the value of SleepTimer.MIN_TIME_TO_RESTART_SLEEP_TIMER_IN_MINUTES to 1 so you don't need to wait for 5 minutes

Play something
Open the full player, tap the sleep timer icon (zZz), select X minutes (You can select 1 minute using the last option)
After X minutes the playback should stop
Wait for 5 minutes. (or the time you set)
Tap play again
✅ The sleep timer should not restart

Auto Restart Sleep Timer toggle disabled

Open Sleep Timer setting
Disable Auto Restart Sleep Timer toggle
Repeat the steps from Restarting timer section above
The sleep timer should not restart when this toggle is OFF ✅

Shake device to Restart

Restarting timer after shaking phone in foreground

Run the app
Play something
Open the full player, tap the sleep timer icon (zZz), select X minutes
Shake your device
✅ The sleep timer should restart with X minutes you set
✅ You should see the toast with Sleep timer restarted after device shake message

Restarting timer after shaking phone in background

Run the app
Play something
Open the full player, tap the sleep timer icon (zZz), select X minutes
Put your device in background
Shake your device
Go back to Pocket Casts App
✅ The sleep timer should have restarted with X minutes you set
✅ You should did not see the toast with Sleep timer restarted after device shake message
✅ The Sleep timer restart sound should have played

Do not restart timer after shaking phone

Run the app
Play something
Open the full player, tap the sleep timer icon (zZz), select end of episode or chapter
Shake your device
✅ The sleep timer should not restart
Set Sleep timer for a X minutes
Go to Sleep Timer settings and disable Shake to Restart Sleep Timer toggle
Shake your device
✅ The sleep timer should not restart

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md
Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
I have considered whether it makes sense to add tests for my changes
All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
Any jetpack compose components I added or changed are covered by compose previews
I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

dangermattic · 2024-12-17T15:33:10Z

	2 Warnings
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `PlayerVolumeFadeOut` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

wpmobilebot · 2024-12-17T15:34:49Z

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.

	App Name	📱 Mobile
	Build Type	debugProd
	Commit	`1850bda`
	Direct Download	`pocketcasts-app-prototype-build-pr3377-1850bda.apk`

📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.

	App Name	🚗 Automotive
	Build Type	debugProd
	Commit	`1850bda`
	Direct Download	`pocketcasts-automotive-prototype-build-pr3377-1850bda.apk`

📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.

	App Name	⌚ Wear
	Build Type	debugProd
	Commit	`1850bda`
	Direct Download	`pocketcasts-wear-prototype-build-pr3377-1850bda.apk`

mebarbosa · 2024-12-17T15:38:17Z

...tories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt

+        sleepTimer.stateFlow
+            .map { it }
+            .distinctUntilChanged()
+            .flowOn(Dispatchers.IO)
+            .onEach { state ->
+                onSleepTimerStateChange(state)
+            }
+            .catch { throwable ->
+                Timber.e(throwable, "Error observing SleepTimer state")
+            }
+            .launchIn(this)
+    }


I was thinking to observe the sleep timer changes by using sleeptimer.stateFlow.collect {} instead, but searching for the best approach I found this one.

The collect { } is suspending, which means it will block the current coroutine until the flow is canceled.

mebarbosa · 2024-12-17T15:41:03Z

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

+    private fun shouldRestartSleepEndOfEpisode(
+        diffTime: Duration,
+        currentEpisodeUuid: String,
+        isSleepEndOfEpisodeRunning: Boolean,
+    ) = diffTime < MIN_TIME_TO_RESTART_SLEEP_TIMER_IN_MINUTES && !sleepTimerHistory.lastEpisodeUuidAutomaticEnded.isNullOrEmpty() && currentEpisodeUuid != sleepTimerHistory.lastEpisodeUuidAutomaticEnded && !isSleepEndOfEpisodeRunning
+
+    private fun shouldRestartSleepEndOfChapter(diffTime: Duration, isSleepEndOfChapterRunning: Boolean) = diffTime < MIN_TIME_TO_RESTART_SLEEP_TIMER_IN_MINUTES && !isSleepEndOfChapterRunning && sleepTimerHistory.lastSleepAfterEndOfChapterTime != null
+


I did not change the logic of these functions. I just moved them to the bottom of the class since they are both private

mebarbosa · 2024-12-17T15:42:05Z

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

+        sleepTimerHistory = sleepTimerHistory.copy(
+            lastEpisodeUuidAutomaticEnded = uuid,
+            lastTimeSleepTimeHasFinished = System.currentTimeMillis().milliseconds,
+        )


The change here is that now we have the sleepTimerHistory, instead of having separated variables like lastEpisodeUuidAutomaticEnded and lastTimeSleepTimeHasFinished

mebarbosa · 2024-12-17T15:42:31Z

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

+        sleepTimerHistory = sleepTimerHistory.copy(
+            lastSleepAfterEndOfChapterTime = time,
+            lastTimeSleepTimeHasFinished = time,
+        )


The change here is that now we have the sleepTimerHistory, instead of having separated variables like lastSleepAfterEndOfChapterTime and lastTimeSleepTimeHasFinished

mebarbosa · 2024-12-17T15:43:22Z

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

-        LogBuffer.i(TAG, "Added extra time: $minutes")
-        createAlarm(time.timeInMillis)
+
+        LogBuffer.i(TAG, "Added extra time: $newTimeLeft")
    }

    fun restartTimerIfIsRunning(onSuccess: () -> Unit): Duration? {


I simplified this method by removing all the callbacks since we now have the state within sleep timer

CookieyedCodes · 2024-12-17T19:03:28Z

Can I make a small suggestion,
If I long press on a certain sleep timer tital,
it would load a preset timer based of settings or resets the number back to 1?, hitting the +&- is a tad annoying and it would be a nice enhancement and appreciated imo :)

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

geekygecko · 2024-12-18T05:16:35Z

...tories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt

@@ -2329,9 +2267,6 @@ open class PlaybackManager @Inject constructor(
                if (isPlaying()) {
                    statsManager.addTotalListeningTime(UPDATE_TIMER_POLL_TIME)
                }
-                if (playbackStateRelay.blockingFirst().isSleepTimerRunning && !isSleepAfterEpisodeEnabled()) { // Does not apply to end of episode sleep time
-                    setupFadeOutWhenFinishingSleepTimer()
-                }
                verifySleepTimeForEndOfChapter()


I could be wrong but would it be better to move the sleep timer logic and state into the SleepTimer class. Changing the code to something like the following:

Suggested change

verifySleepTimeForEndOfChapter()

val playbackState = playbackStateRelay.blockingFirst()

if (sleepTimer.shouldPause(playbackState = playbackState)) {

pause(sourceView = SourceView.AUTO_PAUSE)

}

@geekygecko I like this suggestion and I was trying to push this refactor today, but I am facing with more issues because the PlaybackManager.setupUpdateTimer (the place we also verify end of chapter) using RxJava and some methods of this end of chapter code uses Coroutine.

Could we move this refactor to other PR? It would make easier to test and also development. Every refactoring I am doing I need to smoke test the whole sleep timer just because there are a lot of changes here.

Sorry I have approved the PR. Feel free to do any refactoring in another PR.

No worries. I created this issue for this refactor #3384

...epositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimer.kt

mebarbosa · 2024-12-18T17:08:14Z

Can I make a small suggestion, If I long press on a certain sleep timer tital, it would load a preset timer based of settings or resets the number back to 1?, hitting the +&- is a tad annoying and it would be a nice enhancement and appreciated imo :)

Thank you for your suggestion, @CookieyedCodes . I can't not address anything new here and I would also need to check with our designs first, but I will share with them

CookieyedCodes · 2024-12-18T17:09:16Z

No bothers, just a suggestion 😉

...tories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimerState.kt

mebarbosa added 11 commits December 17, 2024 08:25

refactor: move sleep timer state to playback manager

01e2570

refactor: extract last listened state for end of the chapter

7b5f4cc

refactor: move sleep timer creation to playback service

5f1e1ad

refactor: move sleep timer state out of playback manager

727f292

fix end of episode and chapters were being cancelled after set

e2382c0

refactor: extract sleep timer history state

0da168a

refactor: simplify restart method

b14ff93

refactor: move end of episode and chapter to sleep timer

828ed4a

fix: observe sleep timer state in shelf

e6c816f

refactor: move private functions to the bottom

fc4e802

refactor: extract player volume fade out into a new class

f4f7e20

mebarbosa added [Type] Bug Not functioning as intended. [Project] Sleep Timer labels Dec 17, 2024

mebarbosa added this to the 7.80 milestone Dec 17, 2024

mebarbosa requested a review from a team as a code owner December 17, 2024 15:32

mebarbosa requested review from geekygecko and removed request for a team December 17, 2024 15:32

update CHANGELOG.md

cc3e0ca

mebarbosa added the unit-tests-exemption label Dec 17, 2024

mebarbosa commented Dec 17, 2024

View reviewed changes

geekygecko reviewed Dec 18, 2024

View reviewed changes

mebarbosa added 2 commits December 18, 2024 08:51

refactor: make state as variable instead of function

76613e2

refactor: simplify sleepAfter by removing callback

6a8a84b

mebarbosa added 3 commits December 18, 2024 10:44

refactor: make SleepTimerHistory as sealed interface

630e4e7

fix: set correct number of chapters when playing an chapter

ea35f1e

fix: unit tests

229f235

mebarbosa requested a review from geekygecko December 18, 2024 17:08

geekygecko approved these changes Dec 19, 2024

View reviewed changes

...tories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/SleepTimerState.kt Outdated Show resolved Hide resolved

mebarbosa mentioned this pull request Dec 19, 2024

[Tech Debt] Extract end of episode logic out of PlaybackManager #3384

Open

mebarbosa added 2 commits December 19, 2024 08:52

refactor: rename property

f5e0ec1

refactor: update SleepTimerHistory

1850bda

mebarbosa enabled auto-merge (squash) December 19, 2024 12:05

mebarbosa merged commit cf564f9 into main Dec 19, 2024
14 checks passed

mebarbosa deleted the task/fix/sleep-timer branch December 19, 2024 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Sleep timer stops podcast after given time #3377

Fix: Sleep timer stops podcast after given time #3377

mebarbosa commented Dec 17, 2024 •

edited

Loading

dangermattic commented Dec 17, 2024 •

edited

Loading

wpmobilebot commented Dec 17, 2024 •

edited

Loading

mebarbosa Dec 17, 2024

mebarbosa Dec 17, 2024

mebarbosa Dec 17, 2024

mebarbosa Dec 17, 2024

mebarbosa Dec 17, 2024

CookieyedCodes commented Dec 17, 2024 •

edited

Loading

geekygecko Dec 18, 2024

mebarbosa Dec 18, 2024

geekygecko Dec 19, 2024

mebarbosa Dec 19, 2024

mebarbosa commented Dec 18, 2024

CookieyedCodes commented Dec 18, 2024

-                verifySleepTimeForEndOfChapter()
+                val playbackState = playbackStateRelay.blockingFirst()
+                if (sleepTimer.shouldPause(playbackState = playbackState)) {
+                    pause(sourceView = SourceView.AUTO_PAUSE)
+                }

Fix: Sleep timer stops podcast after given time #3377

Fix: Sleep timer stops podcast after given time #3377

Conversation

mebarbosa commented Dec 17, 2024 • edited Loading

Description

Testing Instructions

Sleep Timer for X minutes

Sleep Timer for end of episodes

Sleep Timer for end of chapter

Auto Restart Sleep Timer

Restarting timer

Not restarting timer

Auto Restart Sleep Timer toggle disabled

Shake device to Restart

Restarting timer after shaking phone in foreground

Restarting timer after shaking phone in background

Do not restart timer after shaking phone

Checklist

dangermattic commented Dec 17, 2024 • edited Loading

wpmobilebot commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CookieyedCodes commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mebarbosa commented Dec 18, 2024

CookieyedCodes commented Dec 18, 2024

mebarbosa commented Dec 17, 2024 •

edited

Loading

dangermattic commented Dec 17, 2024 •

edited

Loading

wpmobilebot commented Dec 17, 2024 •

edited

Loading

CookieyedCodes commented Dec 17, 2024 •

edited

Loading