Discuss API refinements, including caching

[ghost]

Over at issue #61 we considered a refactored design for asynchronously loading TileJSON, and at pull request #71 we discussed my issue61 branch with a new approach to solving problems around the asynchronous TileJSON stuff.

The conclusion from #71 seems to be that the general approach in the issue61 branch is sound, but it needs to be packaged within an API which feels consistent with Apple's API's in general, and MKMapKit in particular. So this ticket is for discussing refinements to the API. One of the big open questions is what the caching will look like.

My plan for the moment is:

• [x] Make a new branch based on issue61
• [x] Get rid of MBXStandardDelegate by moving its code back to the OS X and iOS sample apps.
• [x] Change the caching mechanism to be based on NSURLCache (see #46, #66, and #67).
• [x] Build a tile downloading and storage mechanism for persistent storage of offline regional maps, similar to RMTileCache from the SDK (see #66).
• [x] Once the delegate protocol implementations and caching stuff are in good order, work on simplifying, renaming, and otherwise polishing up the remaining API.

I'm guessing this iteration will reach a good stopping point by approximately March 20th (next Thursday).

[ghost]

I just hit a significant milestone. Basically the first draft of my sqlite stuff for offline maps is complete and working. By that I mean I can create a db using an array of urls which need to be downloaded, I can read progress counts out of it, I can read batches of urls (using SELECT... LIMIT) to download, I can write image blobs and status codes back out, I can save the completed db to a unique filename, and I can find and display completed offline map db's on the map. This is progress!

The main things left on the todo list for this ticket are:

• [x] Calculate urls to be downloaded for a given map region (it's using a hardcoded list for now)
• [x] In the sample app, add a way to delete offline maps
• [x] Beat on the demo app, see what breaks, and fix it.

[ghost]

Status update: rough cut at dynamically generating the tile urls is working, but the marker icon stuff needs work in a couple different ways. When I hardcoded a url for a marker icon that I knew was in the markers.geojson, it didn't show up on the map, and that's a bit puzzling. Also, on the MBXOfflineMapDownloader side, I need to implement something to download and parse the marker geojson at the time that the list of urls is being created.

[ghost]

Offline maps work on iOS, markers included!

[incanus]

Handy pre-PR link comparing the issue73 branch against master:

https://github.com/mapbox/mbxmapkit/compare/master...issue73

[incanus]

This seems really well thought-out, @wsnook! I can't possibly hope to totally grok 65 commits and 32 changed files with 4,961 additions and 5 deletions just yet, particularly the whole functionality of MBXOfflineMapDownloader and its dual queue system, but here's how I see things proceeding:

1. We address some more important comments below leading up to / when we voice Tuesday.
2. We consider some other comments, more as backburner things that we can discuss on GitHub.
3. We squash these into a new commit or handful of commits and open a pull request for more in-depth review and testing, getting it into master for some testing before release of 0.3.0.

Main comments (voice)

• Overall, and I mean this from a general "Mapbox how we do it" angle, it would help in future if you could more clearly highlight how to test the new functionality. I'm able to read through all the source myself and figure it out, but it takes a while and having a nice "run this app and press X button" comment would really help before I dive all in, since I've been away from these for some weeks / never used your example app directly anyway.
• Related to the above, we need to clean up all the v030 stuff and just make the final proposed-merge branch reflect the state that master should be. In a branch, don't be shy about blowing away our current view controller/sample app/whatever. Branches are cheap and non-permanent.
• If I'm understanding things properly, MBXOfflineMapDatabase instances, though referenced by the MBXOfflineMapDownloader and used as arguments by the delegate methods and -removeOfflineMapDatabase:, are meant to be opaque, "private" items. We should consider making all the methods and most/all of the properties on MBXOfflineMapDatabase private, with a category inside MBXOfflineMapDownloader / any other class that needs to use them directly. That is to say the "public" should never be calling these methods. As a descendent of NSObject, too, you can throw an exception inside of a declared -init to discourage such direct use. Otherwise, believe me, it's gonna happen / generate support inquiries.
• Why is the delegate passed in -[MBXRasterTileOverlay initWithOfflineMapDatabase:delegate:]? How is this different from instantiating with a mapID? I'm asking both out of implementation curiosity but also as a future potential confused user of the API.

Minor comments (online)

• Properties like these should be more declarative and explain what they are for. metadata says "data structure" to me, not a BOOL value. Similar for markers, which says "image data or GeoJSON", not a boolean value. Probably something like hasMetadata? This is especially confusing when reading code like https://github.com/mapbox/mbxmapkit/blob/7ca0eca211a5b4cf89291c8e6fc7e80737fb3430/v030/MBXRasterTileOverlay.m#L208-L215 where it's not really obvious what's happening. Does metadata = YES mean that metadata is not loaded?
• Thinking ahead to how MBXOfflineMapDatabase will be used, could we maybe take -initWithContentsOfFile: one step further as we do over in https://github.com/mapbox/mapbox-ios-sdk/blob/3a1835bc8871dfba53befa20b41bddfdf1f4ca33/MapView/Map/RMMBTilesSource.h#L53-L63. These examples allow the user to specify a bundle resource or even just the first part of the filename (assuming .mbtiles) and the framework does the rest. Similarly, for offline map databases, these are basically always going to be in the app jail and either in the documents folder or the caches folder. I would recommend some default, but changeable, file extension (.mboffline?) perhaps before your .partial/.complete or even nothing appended to .mboffline in place of .complete, and then something like -initWithDatabaseName:inDocuments:, the latter argument being a BOOL defaulting to YES.
• Related to the above, we should make iCloud backup configurable (https://github.com/mapbox/mbxmapkit/blob/7ca0eca211a5b4cf89291c8e6fc7e80737fb3430/v030/MBXOfflineMapDownloader.m#L80) per our discussions in https://github.com/mapbox/mapbox-ios-sdk/pull/410. Defaulting as you've done but with the ability to change seems like good practice.
• I like how the user agent for background downloads is differentiated (https://github.com/mapbox/mbxmapkit/blob/7ca0eca211a5b4cf89291c8e6fc7e80737fb3430/v030/MBXOfflineMapDownloader.m#L163-L167) but we should allow customization there, as well as setting it in one place for all uses of the SDK to avoid code duplication. The iOS SDK has a good model for this now in https://github.com/mapbox/mapbox-ios-sdk/blob/3a1835bc8871dfba53befa20b41bddfdf1f4ca33/MapView/Map/RMConfiguration.h#L52. It's just crazy simple there, as it should be.
• Inside of -completeDatabaseAndInstantiateOfflineMapWithError: the idea of moving a file from a background thread (albeit supposedly a single-concurrency one) is kind of gross to me. Maybe an assert or dispatch_sync() to main queue there? Just something that ensures that the -setMaxConcurrentOperationCount:1 has happened elsewhere rather than implicitly linking these by intent, which will eventually be lost to the sands of time.
• Why isn't -[MBXOfflineMapDownloader delegate] a property as is custom? Why is this write-only? That's weird for a delegate.

[ghost]

The Plan from voice chat just now with @incanus... I need to:

1. Squash all these commits into a new branch and create a new issue/PR for discussing that branch
2. Track down a glitch where the offline map downloader can hang up when changing states. This is probably a new thing that I introduced in the past couple days with the marker icon code.
3. Write up some instructions about how to use the sample app to exercise and test the capabilities of the api, perhaps in the readme.
4. Start looking at what needs to happen for cleaning out cruft and polishing things for up an 0.3.0 release. First item on that list should be replacing the main sample app with the code from the v030 sample app. What to do about OS X support is still unresolved

[ghost]

I'm closing this to be continued over at #79.