Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sleep Timer: automatically restart if playback is resumed within 5 minutes #1612

Conversation

leandroalonso
Copy link
Member

@leandroalonso leandroalonso commented Apr 11, 2024

This PR implements a mechanism to restart the sleep timer whenever the user restarts playback within 5 minutes.

  • The sleep timer will restart with the same configuration as before (same time or end of episode)
  • If playing after more than 5 minutes, the sleep timer should not restart

I haven't added any confirmation sound because if I'm sleeping I don't feel like hearing a voice or a confirmation sound for the sleep timer, but we can adjust accordingly in the future based on feedback.

To test

To make testing faster, go to SleepTimerViewController.swift and change the fiveMinutesTapped function to 5.seconds (instead of 5.minutes), also go to Profile > Beta Features > and enable tracksLogging.

Restarting timer for a given amount of time

  1. Run the app
  2. Add 5 to 10 podcasts to your Up Next
  3. Play something
  4. Open the full player, tap the sleep timer icon (zZz), select 5 minutes (in our case it will be 5 seconds)
  5. After 5 seconds the playback should stop
  6. Tap play again
  7. ✅ The sleep timer should restart with 5 seconds
  8. ✅ Ensure 🔵 Tracked: player_sleep_timer_restarted ["duration": 5.0] is tracked

Not restarting timer

Go to SleepTimerManager.swift and change restartSleepTimerIfPlayingAgainWithin to 5.seconds.

  1. Play something
  2. Open the full player, tap the sleep timer icon (zZz), select 5 minutes (in our case it will be 5 seconds)
  3. After 5 seconds the playback should stop
  4. Wait for 10 seconds
  5. Tap play again
  6. ✅ The sleep timer should not restart

Restarting timer for end of episode

Go to SleepTimerManager.swift and revert restartSleepTimerIfPlayingAgainWithin to 5.minutes.

  1. Play something
  2. Open the full player, tap the sleep timer icon (zZz), select end of episode
  3. Drag the scrubber close to the end of the episode and wait till it's finished
  4. Tap play again
  5. ✅ The next episode should play and sleep timer should restart for end of episode
  6. ✅ Ensure 🔵 Tracked: player_sleep_timer_restarted ["duration": "end_of_episode"] is tracked

Pausing

  1. Play something
  2. Open the full player, tap the sleep timer icon (zZz), select any timer
  3. Pause the playback
  4. Check how many time is left in the timer
  5. Unpause
  6. ✅ The timer should restart from where it was, it should not reset

Canceling

  1. Play something
  2. Open the full player, tap the sleep timer icon (zZz), select any timer
  3. Open the timer sheet again and cancel the timer
  4. Pause and play again
  5. ✅ The timer should not restart

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

@leandroalonso leandroalonso added this to the 7.62 milestone Apr 11, 2024
@leandroalonso leandroalonso requested a review from a team as a code owner April 11, 2024 15:14
@leandroalonso leandroalonso requested review from bjtitus and removed request for a team April 11, 2024 15:14
@leandroalonso leandroalonso marked this pull request as draft April 11, 2024 16:09
@leandroalonso leandroalonso marked this pull request as ready for review April 11, 2024 16:27
}

func restartSleepTimerIfNeeded() {
guard !PlaybackManager.shared.sleepTimerActive() else {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's kinda weird to call PlaybackManager here while this is injected there, but I thought it would be better to extract extending sleep timer functionality outside of PlaybackManager, given it's already big and complex.

Copy link
Contributor

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Sleep timer behaved as expected
player_sleep_timer_restarted was logged
✅ Sleep timer wasn't restarted once I waited past the restartSleepTimerIfPlayingAgainWithin value
❌ I couldn't get the "Restarting timer for end of episode" function to work like I expected. Playback did end at the end of the episode but the timer set back to End of Episode and playback continued for the second podcast past the end and into the next in queue. I also didn't see the player_sleep_timer_restarted event logged and didn't hit a breakpoint in SleepTimerManager.playbackTrackChanged.
✅ Checked timer countdown on Pause & Cancel

Here's a recording of the End of Episode behavior I saw:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-11.at.21.34.34.mp4

Comment on lines 44 to 45
Analytics.shared.track(.playerSleepTimerRestarted, properties: ["duration": "end_of_episode"])
PlaybackManager.shared.sleepOnEpisodeEnd = true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @leandroalonso 👋🏽 Awesome work!!

Just to double check... According to our Pocket Casts Tracking Plan sheet, the property for playerSleepTimerRestarted event should be time instead of duration. Is that right? I can use duration and update the sheet if you wanted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops, my mistake! It should be time so it's consistent with the other events!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 19f6c88

@mebarbosa mebarbosa requested review from mebarbosa and removed request for mebarbosa April 12, 2024 17:05
@leandroalonso
Copy link
Member Author

@bjtitus ops, sorry! One change that I made after testing this step and adding the last two caused a regression. Thanks for the thorough testing and spotting that. I updated the code and now it should behave correctly.

Copy link
Contributor

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Everything works now!

@leandroalonso leandroalonso merged commit 6461135 into trunk Apr 15, 2024
5 checks passed
@leandroalonso leandroalonso deleted the sleep-timer/restart-automatically-if-playing-within-5-minutes branch April 15, 2024 14:01
@bitmanlger
Copy link

Thanks, this is awesome, though please also follow up with automatic sleep timer setting during configurable hours, i.e. it should work also if you wait more than 5 mins.
I see [7.64] has more sleep timer stuff, so I assume you're still working on it?

@leandroalonso
Copy link
Member Author

@bitmanlger this is still in the works!

@bitmanlger
Copy link

Great, looking forward to it :)

@pachlava pachlava added the [Type] Enhancement Improve an existing feature. label Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sleep Timer [Type] Enhancement Improve an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants