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

Feature/bg fetch #1303

Draft
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Draft

Feature/bg fetch #1303

wants to merge 11 commits into from

Conversation

charliescheer
Copy link
Contributor

Fix

This is a continuation of the original PR #1297 I moved the changes to a new branch, so created a new PR to isolate those features from the widgets changes.

Note that in this new PR I have changed what the fetch is doing. Now the fetch simply wakes the app and lets it run for a time, shutting down before we hit the apple limit. I am still testing if it is actually doing the bg fetch on it's own. If triggered from lldb everything seems to work fine, but iOS hasn't wanted to do background updates for me today, so still trying to see how it responds. Will comment as I know more about what happens if I can get it to actually log a fetch

This is a draft PR for adding background sync back into SN iOS. At this point all this does is implement the BackgroundTasks API into the app and setup Simperium to do a background sync every 30 minutes or more (sync will be triggered by apple when they feel like it, but not before 30 minutes)

Test

The background sync will print to the logs, so there are two options for testing. First, run the simulator to a device (must be at least iOS 13) and leave it alone for a while. As mentioned above the earliest begin date on the refresh is 30 minutes, if you want to make that faster go to

@objc func scheduleAppRefresh() { let request = BGAppRefreshTaskRequest(identifier: BackgroundRefreshConstants.identifier) request.earliestBeginDate = Date(timeIntervalSinceNow: BackgroundRefreshConstants.earliestBeginDate) do { try BGTaskScheduler.shared.submit(request) } catch { print("Couldn't schedule app refersh: \(error)") } }

and change request.earliestBeginDate to Date(timeIntervalSinceNow: 60) which will make it happen in a minute or more. After giving it some time you should see some events appear in the logs for showing how the background sync went

OR,

  1. run the app while connected to Xcode
  2. pause execution in the debug tools
  3. in lldb run this command

e -l objc -- (void)[[BGTaskScheduler sharedScheduler] _simulateLaunchForTaskWithIdentifier:@"com.codality.NotationalFlow.backgroundRefresh"]

  1. unpause running. When you do this a background refresh will be simulated.

Review

(Required) Add instructions for reviewers. For example:

Only one developer and one designer are required to review these changes, but anyone can perform the review.

Release

(Required) Add a concise statement to RELEASE-NOTES.txt if the changes should be included in release notes. Include details about updating the notes in this section. For example:

RELEASE-NOTES.txt was updated in d3adb3ef with:

Added markdown support

If the changes should not be included in release notes, add a statement to this section. For example:

These changes do not require release notes.

@charliescheer charliescheer added this to the Future milestone Jun 1, 2021
@charliescheer charliescheer self-assigned this Jun 1, 2021
@charliescheer charliescheer marked this pull request as draft June 1, 2021 22:37
@charliescheer
Copy link
Contributor Author

Okay, so it turns out when I leave iOS alone and let it do it's thing the BG Fetch does happen when iOS is ready for it. I was able to see it do a few cycles of refreshing in the background and from what I am seeing in the console logs it seems to be working as expected. The app wakes up, reconnects to the web socket, does some updating, and sits quietly till we shut it down.

The way I tested this was:

  1. connecting my device to my computer and opening the console app.
  2. Selecting the device from the left panel and then searching for "background refresh" (there should be nothing there now
  3. Run the app from Xcode to the device. Once it loads, return the to the home screen on the device. You should see "Background Refresh Scheduled" in the console now
  4. Walk away for a while. In the PR the delay time is set to 30 minutes, but you can drop that down to 1 minute and it should cause the sync more frequently. I was seeing it once every ten minutes or so when I had it set to trigger each minute.
  5. When you return, you should see more background refresh messages. Select one of the "background refresh initiated" messages and change the search to "Simplenote" to see how it responds.

Comment on lines 429 to 442
private func handleAppRefresh(task: BGAppRefreshTask) {
NSLog("Did fire handle app refresh")
guard BuildConfiguration.current == .debug else {
return
}

NSLog("Background refresh intiated")
scheduleAppRefresh()

DispatchQueue.main.asyncAfter(deadline: .now() + BackgroundRefreshConstants.timeOut) {
NSLog("Background refresh finishing")
task.setTaskCompleted(success: true)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also set an expiration handler, because according to Apple Docs:

If you don’t set an expiration handler, the system will mark your task as complete and unsuccessful instead of sending a warning.

If we set an expiration handler I think there is no reason to have our own timeout.


Other ideas:

  • We can extract all BG related code to a separate class to clean up app delegate
  • To avoid always waiting for BG task to expire we can try to be smart: re-set a timer for x seconds on every change that we receive (- (void)bucket:(SPBucket *)bucket didChangeObjectForKey:(NSString *)key....). So when all changes are received and processed, we wait a bit and finish BG task.

@peril-automattic
Copy link

peril-automattic bot commented Jun 2, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

Simplenote/SPAppDelegate.m Outdated Show resolved Hide resolved
Simplenote/SPAppDelegate.m Show resolved Hide resolved
Comment on lines 13 to 19
var finished: Bool = false {
didSet {
if finished == true {
handler?()
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to track finished

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A totally. The finished variable was something I had put in because I was having issues making an instance of the backgroundFetchManager in the app delegate. Was trying to use changing it to trigger ending the refresh, but using the handler instead can serve that same purpose. I have removed the finished var.

Simplenote/BackgroundRefreshManager.swift Show resolved Hide resolved
@charliescheer
Copy link
Contributor Author

Thanks for the suggestions @eshurakov I have updated the PR per these suggestions.

Did some more testing today and it is working for me again! Seems that I made iOS mad yesterday by not shutting down the BG task

@jleandroperez jleandroperez removed their request for review March 14, 2022 12:44
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