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: add the option to select the number of episodes #1640

Merged
merged 13 commits into from May 13, 2024

Conversation

leandroalonso
Copy link
Member

Adds the option for the user to configure the number of episodes for Sleep Timer.

To test

Go to Profile > Settings > Beta Features > enable tracksLogging.

  1. Add a bunch of episodes to your Up Next queue
  2. Play an episodes
  3. Open the full player > tap Sleep Timer (zZz)
  4. Use the + / - button on the penultimate row to change the number of episodes
  5. ✅ The label should update accordingly
  6. Change it to 3 and tap "In 3 episodes"
  7. ✅ Ensure 🔵 Tracked: player_sleep_timer_enabled ["number_of_episodes": 3, "time": "end_of_episode"] is tracked
  8. Tap Sleep Timer (zZz) again
  9. ✅ You should see "Sleeping in 3 episodes"
  10. Move the scrubber to the end
  11. Tap Sleep Timer (zZz) again
  12. ✅ You should see "Sleeping in 2 episodes"
  13. Move the scrubber to the end
  14. Tap Sleep Timer (zZz) again
  15. ✅ You should see "Sleeping at the end of the current episode"
  16. Move the scrubber to the end
  17. ✅ Playback should pause
  18. Tap play again
  19. ✅ Once playback starts, sleep timer should restart for 3 episodes
  20. ✅ Ensure 🔵 Tracked: player_sleep_timer_restarted ["number_of_episodes": 3, "time": "end_of_episode"] is tracked

Extending

  1. Play an episode
  2. Open the full player > tap Sleep Timer (zZz)
  3. Choose 30 minutes
  4. Tap Sleep Timer (zZz) again
  5. Tap "In 3 episodes"
  6. ✅ Ensure 🔵 Tracked: player_sleep_timer_extended ["number_of_episodes": 3, "amount": "end_of_episode"] is tracked
  7. ✅ The screen should change to "Sleeping in 3 episodes"

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.63 milestone Apr 23, 2024
@leandroalonso leandroalonso requested a review from a team as a code owner April 23, 2024 20:07
@leandroalonso leandroalonso requested review from SergioEstevao and removed request for a team April 23, 2024 20:07
@dangermattic
Copy link
Collaborator

dangermattic commented Apr 23, 2024

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is assigned to the milestone 7.63. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@@ -0,0 +1,188 @@
import UIKit

class CustomStepper: UIControl {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not proud of copying/pasting CustomTimeStepper here. I tried playing with generics but forgot that Storyboard only plays nice with Obj-C. 🤡

Comment on lines 95 to 98
// Here we watch the current episode until it actually starts playing to
// restart the timer per episode.
// This avoid issues because the Sleep Timer per episode pauses on the end
// of the last episode.
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'm not sure this comment conveys the complexity of restarting the sleep timer. I tried many different events and logic, and ultimately everything — besides this solution — failed.

I'll be more verbose here to try to explain:

  1. When we finish a sleep timer per episode, we play at the end of the last played episode;
  2. When we tap play again, this episode will still be queued to play and the timer will restart
  3. However, we can't differentiate if this episode "playing" is what was playing before or not. And due to the nature of PlaybackManager, currentEpisode() already changes to the next one (even though the whole player displays the previous one).
  4. If we restart the timer, this "end of episode" will be counted as a one-played episode, which leads to if the timer is set to 1 episode, nothing happens. The app thinks that this episode that just finished was the episode meant to be played. If it's bigger than 1 episode, 1 will be deduced and the timer will start with one less episode.
  5. As none of this is enough, buffering is also part of the equation: we can't restart the timer unless an episode is playing (thus the check here).

I spent a good amount of hours trying to make that simpler but I couldn't. So any suggestion on improving this comment — if it's not clear — would be appreciated.

Comment on lines +36 to +43
var bigIncrements: Int = 1
var smallIncrements: Int = 1
var smallIncrementThreshold: Int = 1
var currentValue: Int = 1 {
didSet {
accessibilityValue = "\(currentValue)"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional Suggestion: Instead of generics why not abstract this to a protocol and have two implementations one for Int and another for TimeInterval that you can set on init?

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

This is working great! Tested the scenarios presented and all working correctly.

@leandroalonso leandroalonso merged commit 6d460ac into trunk May 13, 2024
4 checks passed
@leandroalonso leandroalonso deleted the sleep-timer/change-number-of-episodes branch May 13, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants