search

mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

Updating offline metadata causes existing offline region to go stale #16450

Open 1ec5 opened 4 years ago

1ec5 commented 4 years ago

The mbgl::DatabaseFileSource::updateOfflineMetadata() method added in #6338 updates the offline region’s associated metadata in the offline database, but calling it effectively invalidates any mbgl::OfflineRegion the SDK has previously obtained and wrapped in a platform-specific object. Getting that region’s metadata continues to return the same old value without reflecting the change that has taken place in the offline database.

It’s very awkward for the iOS/macOS map SDK to tell developers that calling -[MGLOfflinePack setContext:completionHandler:], as in mapbox/mapbox-gl-native-ios#289, would cause the MGLOfflinePack.context property to become stale and that the only way to get the current context would be to reload the entire set of offline packs by calling -[MGLOfflineStorage reloadPacks] (which calls mbgl::DatabaseFileSource::listOfflineRegions() under the hood).

The mbgl::DatabaseFileSource::updateOfflineMetadata() method’s callback should accept a replacement mbgl::OfflineRegion with the updated metadata. The SDK could then update the MGLOfflinePack’s internal pointer to the mbgl::OfflineRegion with its replacement, and the SDK would remain internally consistent.

/cc @mapbox/gl-native @mapbox/maps-ios

1ec5 commented 4 years ago

Actually, calling mbgl::DatabaseFileSource::listOfflineRegions() afterward returns exactly the same offline regions as before, as though the database write failed silently.

Edit: Never mind, PEBKAC.

1ec5 commented 4 years ago

It looks like we have a unit test for mbgl::OfflineDatabase::updateMetadata(), but not for mbgl::DatabaseFileSource::updateOfflineMetadata(). As far as I know, the iOS/macOS map SDK has easy access to the DatabaseFileSource but not to the OfflineDatabase.

https://github.com/mapbox/mapbox-gl-native/blob/296ca0d437d05d3ede17f8202c1f53bc9ee6bc3a/test/storage/offline_database.test.cpp#L456-L470

chloekraw commented 4 years ago

cc @mapbox/maps-android @zugaldia

1ec5 commented 4 years ago

mapbox/mapbox-gl-native-ios#289 worked around this issue so that developers don’t need to work around it themselves. However, the workaround may not perform as well as if mbgl would return the updated region.