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

feat(mobile): Background app refresh status #1839

Merged
merged 8 commits into from
Feb 23, 2023

Conversation

martyfuhry
Copy link
Contributor

Adds a message that App Refresh is currently disabled and a settings button to navigate to app settings.

Unfortunately, the Go To Settings takes you to Settings > Immich instead of Settings > General > Background App Refresh.

  • There is not a safe way to navigate to the latter. Maybe the message should say that's where they need to go?
  • If Background App Refresh is on globally, but off for Immich, the system still reports that it's "available". This will be obvious for folks with 0 processes queued, though.
  • App Refresh tasks can now be scheduled even when connected power is required; they will check to see if they have power and if so, run, otherwise immediately complete.
  • Fixes an issue where turning off and on background app refresh would clear all Immich tasks until a full app restart. Now you just need to reopen the Immich app to bring it back to foreground to reschedule the background tasks.

image

@vercel
Copy link

vercel bot commented Feb 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
immich ⬜️ Ignored (Inspect) Feb 23, 2023 at 5:54PM (UTC)

}

final iOSBackgroundSettingsProvider = StateProvider<IOSBackgroundSettingsState>(
(ref) => IOSBackgroundSettingsState(ref.read(backgroundServiceProvider)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use ref.watch here? is there any reason you are choosing to use ref.read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure. I guess watch will update this provider when it changes? This only barely depends on the backgroundServiceProvider at all; it just had all the method channel communication functions in it, so I piggybacked there.

I'm still not super familiar with RiverPod and there's likely something I'm not doing correct here. But I made the change and it still seems happily working here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Riverpod suggests using ref.watch as much as possible I believe. Only using ref.read in button pressing context.

Here is some good info
https://riverpod.dev/docs/concepts/reading#using-ref-to-interact-with-providers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I gave it a read and it's becoming clearer to me.

In any event, I changed it to ref.watch and looked through the code to find other ref.read statements. In this case, the other ref.read statements are justified as they are just one-off refreshes (app resume and page load).


}

final iOSBackgroundSettingsProvider = StateProvider<IOSBackgroundSettingsState>(
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using StateNotifier I believe you have to use StateNotifierProvider here instead of StateProvider. Is using StateProvider do what you are expecting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to say I resolved this.

@alextran1502 alextran1502 merged commit 2b988e1 into main Feb 23, 2023
@alextran1502 alextran1502 deleted the feat/background-app-refresh-status branch February 23, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants