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): iOS background sync #1758

Merged
merged 38 commits into from Feb 20, 2023
Merged

Conversation

martyfuhry
Copy link
Contributor

@martyfuhry martyfuhry commented Feb 14, 2023

Enables background sync on iOS.

  • Currently not well formatted since I had to write most of this in stinky old XCode
  • Probably could use some better restructuring
  • Background tasks with BGAppRefreshTaskRequest (short refresh task)
  • Background tasks with BGProcessingTaskRequest: (execute a processing task that can take minutes to complete)
  • Testing outside of manual scheduling
  • User settings are passed and respected
  • Starts and stops the background service when enabled rather than at launch
  • Shows notification with updates while running feat(mobile): Notifications on background sync ios #1790
  • Shows error notifications feat(mobile): Notifications on background sync ios #1790
  • Disable max delay slider for iOS
  • Passes the thread lock from the background backup process to the foreground process when the foreground process is launched during a backup (needs tested)

@vercel
Copy link

vercel bot commented Feb 14, 2023

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

Name Status Preview Comments Updated
immich-code-coverage ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 3:42AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
immich ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2023 at 3:42AM (UTC)

@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 14, 2023 15:20 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 14, 2023 17:42 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 14, 2023 18:13 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 14, 2023 19:13 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 14, 2023 20:19 Inactive
…d its failing at the root asset bundle in the localization.dart service
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 05:00 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 15:03 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 15:11 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 16:05 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 16:11 Inactive
@martyfuhry
Copy link
Contributor Author

This finally at least works! I was able to sync a photo using the background fetch manual initialization.

@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 18:54 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 15, 2023 19:34 Inactive
Copy link
Contributor Author

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

I've reverted most of my comments from before. It seems like something I did worked, at least, because in the past my background code was able to acquire the lock while the app was in the foreground (which was wrong), and the foreground app was sometimes unable to acquire the lock from the background execution (which I haven't tested yet).

Now, when I run I simulate the background task while the app is in the foreground, I correctly see:

flutter: [_callHandler] could not acquire lock, exiting
performBackgroundRequest.UIBackgroundFetchResult(rawValue: 2) (finished in 5.209082007408142 seconds)

@@ -244,7 +228,7 @@ class BackgroundService {
return true;
}

Future<void> _checkLockReleasedWithHeartbeat(final int lockTime) async {
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've reverted it.

@@ -389,8 +378,8 @@ class BackgroundService {
return false;
}
// check for new assets added while performing backup
} while (true ==
await _backgroundChannel.invokeMethod<bool>("hasContentChanged"));
} while (false); //TODO: broken on iOS (true ==
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs some better treatment for iOS, as we do not check for new assets added while performing backup in iOS.

@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 17, 2023 13:12 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 17, 2023 13:42 Inactive
Copy link
Contributor

@fyfrey fyfrey left a comment

Choose a reason for hiding this comment

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

looks good to me now!

@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 17, 2023 18:28 Inactive
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 17, 2023 18:31 Inactive
@@ -527,15 +543,15 @@
CODE_SIGN_IDENTITY = "Apple Development";
CODE_SIGN_STYLE = Automatic;
CURRENT_PROJECT_VERSION = 86;
MARKETING_VERSION = "$(FLUTTER_BUILD_NAME)";
DEVELOPMENT_TEAM = 2F67MQ8R79;
DEVELOPMENT_TEAM = 6WU78265ZG;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alextran1502 I had to change this to compile it here. You should change it back by checking it out yourself and going to Runner -> Targets -> Signing & Capabilities -> Signing, then changing it back correctly. I'm hesitant of mangling the project.pbxproj files by editing it myself. I think all that needs done is to change back the DEVELOPMENT_TEAM.

Alternatively, maybe you should add my Apple Developer Account to your Development Team, so I can compile it without changing this again.

@@ -12,6 +12,7 @@ dependencies:
flutter:
sdk: flutter

path_provider_ios:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I commit the pubspec.lock and Podfile.lock too?

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 think that is fine.

@martyfuhry
Copy link
Contributor Author

martyfuhry commented Feb 18, 2023

All that's left to do here is some testing & validation. My last checkbox in the description is to check:

  • Passes the thread lock from the background backup process to the foreground process when the foreground process is launched during a backup (needs tested)

I guess try to open the app while it's in the middle of a sync job? Hard to do since the OS schedules the sync job. On the other hand, you can manually schedule one yourself. The logic hasn't changed between this and iOS, so it's likely OK. And when I run the background job while the foreground app is running, the background job correctly is unable to acquire a lock, so I'm confident it will work.

Finally, some testing should be done to assure that Android's background sync has not regressed, but @zoodyy was kind enough to help there.

And I made #1790 to go along with this, but I could bring it in here, too, if that's easier. I'd prefer to keep them separate and rebase, but I don't have any real reason for the preference.

I just think it needs some more testing and I'm happy with the feature. Anything else you'd like to see here?

@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 19, 2023 01:08 Inactive
…s and fixed network connectivity always required
@vercel vercel bot temporarily deployed to Preview – immich-code-coverage February 19, 2023 16:23 Inactive
@alextran1502 alextran1502 enabled auto-merge (squash) February 20, 2023 05:30
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

3 participants