-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
9222e23
to
21d6313
Compare
This PR has been tested for offline integration and showing multiple maps on screen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, besides the one comment.
|
||
private FileSource(String cachePath, AssetManager assetManager) { | ||
initialize(Mapbox.getAccessToken(), cachePath, assetManager); | ||
} | ||
|
||
public void activate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would opt to make redundant calls possible instead of doing counting here. I suggested a patch to do so here: 034551f
This will also subtly break if the file source would be paused/resumed from another place in the future.
cc @jfirebaugh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfirebaugh could you add your preference on who is responsible for ref counting? the binding or core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in #10174 (review), my preference is to keep pause
/resume
as strict as possible -- in particular callable only by the creating thread -- because they otherwise get very difficult to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivovandongen can you 👍 this if you are onboard on adding this?
@@ -264,6 +264,7 @@ public void onStatusChanged(OfflineRegionStatus status) { | |||
if (status.isComplete()) { | |||
// Download complete | |||
endProgress("Region downloaded successfully."); | |||
offlineRegion.setDownloadState(OfflineRegion.STATE_INACTIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobrun Ref counting aside, this looks like new behavior for region downloads. Are there any side effects if a developer (like until now) doesn't manually set the region as inactive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will result in not pausing the Filesource, in other words will match the same behavior as it is today. For this reason I'm not seeing this as a semver change but an enhancement to API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobrun Makes sense, thanks for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on my end.
closes #9965,
This PR integrates with FileSource pause/resume when the app state changes (foreground vs background). I'm currently looking into counting ongoing filesource interactions for two reasons: