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

Refresh podcasts in background in watch app #1036

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Jun 2, 2023

Description

This adds a background refresh toggle in settings and refreshes podcasts in the background if the toggle is set to on. Initially, the toggle is set to off to prevent unnecessary background processing for the watch app.

Task #1008

Testing Instructions

  1. Apply this patch to reduce refesh initial and periodic interval. [Note that periodic delay lesser than 15 minutes is not allowed.]
  2. Fresh install the app
  3. Login with a plus account having podcasts
  4. Go to Settings
  5. Notice that you see "Background refresh" toggle which is set to off
  6. Clear logs and go to recent apps
  7. Search BgTask: RefreshPodcastsTask in logs
  8. Notice that you do not see BgTask: RefreshPodcastsTask - runNow - Start in logs
  9. Reopen the app
  10. Go to Settings
  11. Toggle on "Background refresh"
  12. Clear logs and go to recent apps
  13. Notice that you see BgTask: RefreshPodcastsTask - runNow - Start in logs
  14. (Optional) Wait for the periodic interval set in the patch in Step 1. [Step 15, 16 are optional, you can see results in the screen shot included below.]
  15. (Optional) Notice that you again see BgTask: RefreshPodcastsTask - runNow - Start in logs

Screenshots or Screencast

logs

I added toggle description in 62a2ce9. If the text feels too long, feel free to revert the commit or shorten the text for wear app.

On Off

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • 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

@ashiagr ashiagr added the [Area] Wear OS watch app label Jun 2, 2023
@ashiagr ashiagr force-pushed the task/wear-app-background-refresh branch from 1d27a2b to 62a2ce9 Compare June 2, 2023 12:02
@ashiagr ashiagr marked this pull request as ready for review June 2, 2023 12:08
@ashiagr ashiagr requested a review from a team as a code owner June 2, 2023 12:08
Base automatically changed from task/wear-app-refresh-on-app-open to main June 2, 2023 15:11
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

These changes look good.

It seems like the text is not entirely accurate. For example, it seems to me that with background refresh turned on, the app refreshes every time the app comes from the background, but if it is turned off it will wait at least 5 minutes before refreshing again.

With that said, this is the same setting and text that we use in the phone app, so for now I think it's fine to leave it as-is and we can iterate on it.

Similarly, the length of the text isn't ideal for a watch, but it feels like having some explanatory text is good. Maybe there's a different UI pattern (opening a full screen toggle or using a toast message) that we could use here in the future (cc: @david-gonzalez-a8c ).

@mchowning mchowning merged commit 6913f1f into main Jun 2, 2023
@mchowning mchowning deleted the task/wear-app-background-refresh branch June 2, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants