-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
7eb468f
to
25cf43d
Compare
@jfirebaugh suggested calling |
Further terminology discussion in #4250. |
} | ||
if (completion) { | ||
dispatch_async(dispatch_get_main_queue(), [&, task, completion, error](void) { | ||
[task invalidate]; |
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 believe [task invalidate]
should be the first line in this method -- as soon as mbglOfflineRegion
is moved out of task
, it's invalidated.
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.
What happens if the removal errors out? Would the region be valid again from mbgl’s perspective, or is it still invalid?
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.
It's still invalid. As soon as deleteOfflineRegion
is called, the caller relinquishes ownership of the region. You can get notified of errors, but not do anything about them, other than obtaining another reference to the region via listOfflineRegions
.
@boundsj pointed out that MGLOfflineStorage should KVO-observe MGLAccountManager’s access token just like MGLMapView does, in case client code sets the access token programmatically at a later point. |
Setting the access token after activating an offline download isn't going to work very well. In that situation, resource downloads may have already been attempted, and |
Oh, I forgot I already implemented KVO on the access token.
Agreed. Even for MGLMapView, setting the access token after initialization doesn’t have guaranteed behavior around caching, but we use KVO because it’s the most reliable way for the shared MGLAccountManager object, all instances of MGLMapView, the shared MGLMapboxEvents object, and now the shared MGLOfflineStorage object to stay on the same page with the same access token. |
_state = MGLOfflineTaskStateUnknown; | ||
|
||
mbgl::DefaultFileSource *mbglFileSource = [[MGLOfflineStorage sharedOfflineStorage] mbglFileSource]; | ||
MBGLOfflineRegionObserver *mbglObserver = new MBGLOfflineRegionObserver(self); |
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.
You can write these two lines more succinctly and with only one allocation, as:
mbglFileSource->setOfflineRegionObserver(*_mbglOfflineRegion, std::make_unique<MBGLOfflineRegionObserver>(self));
564362e
to
037640b
Compare
LGTM. Providing this build passes, . |
Renamed SDK classes related to offline viewing to more closely match the terminology used by mbgl and the Android SDK while remaining consistent with Cocoa naming principles.
Tapping on a download now pauses or resumes it. Tapping on a completed download navigates to the downloaded region and switches to the downloaded style.
Also categorized offline symbols for jazzy and moved styleURL from MGLOfflineRegion to MGLTilePyramidOfflineRegion, since you never know if there will be a region type not constrained by a style in the future.
Added a missing conditional to the invalidation checking macro that I factored out at the last minute.
Fixed a race condition that could occur if a progress update comes after a call to remove the task but before the removal is complete. Avoid synchronously setting the task’s state inside -resume and -suspend; instead, rely on the observer to set the state asynchronously.
“Offline pack” more effectively communicates the persistent nature of the downloaded content.
This PR implements APIs for offline viewing in the iOS and OS X SDKs, based on the work in #3715. Additionally, iosapp’s interface has been partially rewritten using an Interface Builder storyboard and an interface for managing offline downloads has been added.
Here’s a high-level overview of the new classes:
mbgl::OfflineRegionDefinition
, acting as a “bill of materials” for an MGLOfflineTask.mbgl::OfflineRegion
and provides methods for pausing and resuming a download. It pairs an MGLOfflineRegion with an opaque context data blob (analogous tombgl::OfflineRegionMetadata
).mbgl::DefaultFileSource
and provides methods for creating and destroying MGLOfflineTask objects.A word about naming: I chose to diverge from the offline class names used by mbgl and the Android SDK in #4085. The intent is to keep the SDK’s naming intuitive for developers who are familiar with Cocoa development but not necessarily with maps, to avoid the kind of confusion we’ve been seeing around MGLAnnotationImage. In particular, along with MapKit, Core Location, and other Apple-provided frameworks, the iOS SDK already uses the word “region” to refer to a more-or-less geographic model object rather than a task or controller. Additionally, in keeping with strong Cocoa conventions, “context” replaces “metadata” and “delegate” replaces “observer”.
To ensure KVO compliance, MGLOfflineTask registers an internal
mbgl::OfflineRegionObserver
for its entire lifetime that dispatches progress updates to an optional MGLOfflineTaskDelegate. There is a bit of an impedance mismatch withmbgl::OfflineRegion
, which expects all progress information to be requested and communicated to the delegate asynchronously via callbacks. You can see how this gets complicated with-[MGLOfflineTask requestProgress]
. I’m not completely opposed to removing thestate
andprogress
properties and routing all progress information through an asynchronous API. However, we should then consider some kind of affordance for KVO compliance in the near future, because this is a very common paradigm for frequently changing properties in Cocoa. (KVO would help with hooking up MGLOfflineTask to NSProgress, for example.)In the iosapp Downloads view, tapping the + button adds and begins a download of the main map’s current viewport, from the current zoom level all the way to the maximum zoom level. It is recommended that you zoom in before using this feature, to avoid maxing out the tile limit. Tapping an in-progress download pauses and resumes the download, while tapping a completed download switches to its associated style and navigates to the downloaded viewport.
Fortuitously, my Internet connection went kaput last night as I was preparing 34611a7801fab053bbbb73ec5af61c63eda4ce38, allowing me to test much of the offline functionality in a real-world environment.
Fixes #3892.
/cc @jfirebaugh @boundsj @friedbunny @zugaldia