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 #2048

Merged
merged 13 commits into from
Apr 15, 2024

Conversation

mebarbosa
Copy link
Contributor

Description

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 initially set or end of episode
  • If playing after more than 5 minutes, the sleep timer should not restart

While I was working on this, I have found two issues that I reported here:

Testing Instructions

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 X minutes (You can select 1 minute using the last option)
  5. After X minutes the playback should stop
  6. Tap play again
  7. ✅ The sleep timer should restart with X minutes you set
  8. ✅ Ensure 🔵 Tracked: player_sleep_timer_restarted ["time": [time here]] is tracked

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
  1. Play something
  2. Open the full player, tap the sleep timer icon (zZz), select X minutes (You can select 1 minute using the last option)
  3. After X minutes the playback should stop
  4. Wait for 5 minutes. (or the time you set)
  5. Tap play again
  6. ✅ The sleep timer should not restart

Restarting timer for end of episode

  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 ["time": "end_of_episode"] is tracked

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

  • 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 tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@mebarbosa mebarbosa added this to the 7.62 milestone Apr 12, 2024
@mebarbosa mebarbosa requested a review from a team as a code owner April 12, 2024 18:44
@mebarbosa mebarbosa requested review from MiSikora and removed request for a team April 12, 2024 18:44
@CookieyedCodes
Copy link

CookieyedCodes commented Apr 12, 2024

I personally would find 5 mins a tad long, personally i think it should be between 90 seconds or a maximum of 3 mins but if it's a factor of accurate clocks then fair, I would potentially also have a toast with a button to cancel because a user might be unaware of the functionality & that would be quite bad in my opinion 🤔

Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

Nice work! Feature works as described. I've tried breaking it by kill app process and leaving only media notification but everything worked. I left some comments regarding design of the class that would make it easier to maintain in the long-term.

Also, a changelog entry is missing.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 7.62. 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

@mebarbosa
Copy link
Contributor Author

Nice work! Feature works as described. I've tried breaking it by kill app process and leaving only media notification but everything worked. I left some comments regarding design of the class that would make it easier to maintain in the long-term.

Also, a changelog entry is missing.

Hey @MiSikora Thanks for your review! I've just addressed all changes

@mebarbosa
Copy link
Contributor Author

I personally would find 5 mins a tad long, personally i think it should be between 90 seconds or a maximum of 3 mins but if it's a factor of accurate clocks then fair, I would potentially also have a toast with a button to cancel because a user might be unaware of the functionality & that would be quite bad in my opinion 🤔

@CookieyedCodes Thank you for share your feedback! I will share this with our team. I agree that we should at least let the user knows about it

@mebarbosa mebarbosa requested a review from MiSikora April 15, 2024 11:51
@mebarbosa mebarbosa requested a review from MiSikora April 15, 2024 12:24
Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

LGTM! Left one formatting nitpick.

Comment on lines 141 to 147
}
private fun cancelAutomaticSleepAfterTimeRestart() {
lastSleepAfterTime = null
}
private fun cancelAutomaticSleepOnEpisodeEndRestart() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some empty space formatting is missing here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought ./gradlew spotlessApply would handle cases like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm surprised it doesn't complain about it. Maybe something to configure.

@mebarbosa mebarbosa enabled auto-merge (squash) April 15, 2024 12:41
@mebarbosa mebarbosa merged commit 7b4fca6 into main Apr 15, 2024
11 of 13 checks passed
@mebarbosa mebarbosa deleted the task/restart-timer branch April 15, 2024 12:54
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

4 participants