Update OfflineRegion/OfflineRegionDefinition interfaces, synchronize with iOS and Android #545
Conversation
The interface has changed to send region as a argument, as well as the channelName. Without this change, the region download fails. Fixes #534
While the Android SDK generates a region ID in createOfflineRegion, the iOS SDK does not have this feature. To maintain a parallel interface and relieve the client of the need to generate IDs, this change generates an ID before submitting the download request to the iOS SDK. As in the Android implementation, the completed OfflineRegionData is immediately returned through the open channel. Fixes #533 The following changes suggested in #491 are also pursued here: * Restructure OfflineRegionData(native)/OfflineRegion(dart) classes into OfflineRegionDefinition and OfflineRegion classes to mirror SDK class structure. This is basically equivalient to something like OfflineRegionOptions. * Accept OfflineRegionDefinition as an arg to downloadOfflineRegion, and return OfflineRegion as a result Without those changes, the client code is `@required` to submit an id along with the original download request, even though the id will not be used and will be replaced by the generated ID. Furthermore, the object returned from `downloadOfflineRegion` is now decoded so that the client can await it to extract the generated ID. I also made the properties of the definition and data classes immutable on the iOS side, and renamed values dealing with the downloaded pack context from "metadata" to "context". This is because the context is no longer just the user-supplied metadata; it is a structure separately holding the metadata and generated ID. This change is not backwards compatible with regions previously downloaded on iOS, however, offline downloading is not functional in the latest or any release of this library because of #534. It should be fully compatible with the existing implementation on Android, though it would be a good idea to undertake a similar separation of "definition" and "data" on that platform.
This was fairly tricky as the example depends on a fixed set of regions where the IDs were decided at compile time and expected to remain the same after being downloaded, which is no longer the case for Android or iOS. (This example would compile before the dart `OfflineRegion` refactoring, but it wouldn't have worked.) The most straightforward way to adapt it was to add a metadata field "name" which serves to identify elements of this fixed set of regions before and after they're saved. This feels contrived, but the case demonstrated in the example where the downloadable regions are fixed in advance is itself rather unusual. Also worth noting that this example does not wait for the download to complete before indicating that it was downloaded, it just waits for the channel response from the download request message. I've rearranged the code to read in the generated id from that channel response, but the behavior is the same as before.
lib/src/global.dart
Outdated
| Future<dynamic> downloadOfflineRegion( | ||
| OfflineRegion region, { | ||
| Future<OfflineRegion> downloadOfflineRegion( | ||
| OfflineRegionDefinition region, { |
There was a problem hiding this comment.
This breaks the api for android as well. However if you go down the base class route, dart should just cast it here, and existing implementations should not break.
This might also cause issues in json.encode(region._toJson()) for android, as this code probably expects the id?
There was a problem hiding this comment.
This breaks the api for android as well. However if you go down the base class route, dart should just cast it here, and existing implementations should not break.
True, that's a good reason to go with inheritance.
This might also cause issues in json.encode(region._toJson()) for android, as this code probably expects the id?
I don't think so, the Android PR mentions "Do not respect id passed into downloadOfflineRegion via OfflineRegionData, as it will be assigned by createOfflineRegion." The changes are in fromOfflineRegion:
https://github.com/tobrun/flutter-mapbox-gl/pull/491/files#diff-dbcc20feb45d56319fdc09f95c724ae0202964e5befc014f5009981ad449929eR85-R101
There was a problem hiding this comment.
This breaks the api for android as well. However if you go down the base class route, dart should just cast it here, and existing implementations should not break.
True, that's a good reason to go with inheritance.
This is a kuch cleaner api and it might be worth that from the beginning rather than be burdened with unnecessary cruft from a feature that is not even functional in the latest release. I should be able to make the corresponding changes on Android in the next couple of days.
There was a problem hiding this comment.
Just saw the other discussion and change re inheritance and this makes sense now.
This preserves backwards compatibility with the `downloadOfflineRegion` API, and allows `OfflineRegion` objects from the downloaded list to be easily recreated as definitions. #545 (comment)
This naming is consistent with the class names in Dart. #545 (comment)
|
This looks like great work @n8han! I haven't touched iOS code in a while so I don't know if I can give detailed feedback, but I'll do my best to review in the next couple of days. I'm surprised to hear of this difference between the Android and iOS SDKs. In that case I see why the original author of the offline download feature may have chosen to leave it user assignable. I think this approach of generating ids makes sense. My only all conern is that the random id generation, may occasionally result in collision. |
|
Closed by accident |
|
iOS has UUID() - https://developer.apple.com/documentation/foundation/uuid |
There was a problem hiding this comment.
Okay, I had some more time to think about this, look at docs, and remember my initial thoughts.
The Android and iOS APIs are a bit divergent, with naming schemes that are overlapping but don't exactly match, but here is what I found:
-
OfflineRegionon Android corresponds toMGLOfflinePackon iOS. This class on both platforms contains the region definition object, as well as context/metadata. Additionally, on Android, it also contains the ID. This is what we are callingOfflineRegion. -
OfflineRegionDefinitionon Android roughly corresponds toMGLOfflineRegionprotocol on iOS. I say roughly because theMGLOfflineRegionis missingminZoomLevel,maxZoomLevel, andstyleURLproperties, but both types that inherit from it (MGLTilePyramidOfflineRegion and MGLShapeOfflineRegion) have those properties.
The inheritance approach for OfflineRegion does work and has the advantage of being backward compatible, but binds us very early on to an implementation that doesn't reflect what it is supposed to.
Also, given 1 and 2, the current implementation of OfflineRegion is only able to represent TilePyramid region definitions, but not Shape ones, and it might become harder to make room for that or later additions once this decision is made.
In short, it would be best to mimic the native API structure unless there is a good reason not to.
| regionId: regionData.id | ||
| ) | ||
| // Save downloader so it does not get deallocated | ||
| activeDownloaders[regionData.id] = downloader | ||
|
|
||
| // Download region | ||
| downloader.download() | ||
|
|
||
| // Provide region with generated id | ||
| result(regionData.toJsonString()) |
There was a problem hiding this comment.
This seems like it should be called from downloader upon success (and I guess nil in case of error?).
| @@ -43,7 +43,7 @@ class OfflinePackDownloader { | |||
| // MARK: Public methods | |||
| func download() { | |||
| let storage = MGLOfflineStorage.shared | |||
| storage.addPack(for: region, withContext: metadata) { [weak self] (pack, error) in | |||
| storage.addPack(for: region, withContext: context) { [weak self] (pack, error) in | |||
There was a problem hiding this comment.
The FlutterResult callback should be called here upon success or failure, and not from the caller as it won't be aware of the result.
It would probably also be a good idea to read the context from the pack as that is the ultimate source of truth for what is stored.
There was a problem hiding this comment.
The FlutterResult callback should be called here upon success or failure, and not from the caller as it won't be aware of the result.
For success, yes I think so. I'm moving it now to onPackCreated. For failure I hadn't realized but result is already being called with a failure object from onPackCreationError.
I'm pushing a change now with this as well as reading context back from the pack.
This is response to code review requests to call result success within the downloader (a good idea, that's where failures were already handled) and to read id and metadata back from the pack rather than returning what we had requested the sdk to put in the pack. #545 (comment) While doing this it made sense to do the id generation within the downloader, so that fewer objects are passed in and only the downloader knows the details about the sdk objects.
I think we should use the same type as the android sdk since it's exposed in the shared dart interface as a 64 bit integer. |
I'm not sure if you mean mimic on the native interfaces or the dart interfaces. I agree that it would be best to make the dart api forwards compatible with polygonal bounds, if you want to propose a specific interface for that or push to this branch? For the native (swift) classes, in my view it doesn't so much matter since they can be fully rewritten at any point without breaking compatibility with flutter apps. They're not how I would have designed them but I'd do other things than further refactor them right now. |
UUID().hashValue would give you an int and be better than randomly generated int but not quite as strong as the full 128 bits of the UUID. |
`downloadOfflineRegion` now accepts a separate `metadata` arg in addition to the `definition` The `definition` parameter to `invokeMethod` (formerly `region`) is no longer a JSON string, but the object itself, so deserialization may have to change.
|
Not sure if the Android changes should be a separate PR since this one is about iOS? |
I think it's better if native changes tied to the dart changes are all in one PR. I'll update the name and description when I make the further iOS changes, next day or two, to reflect the broader scope. |
OfflineRegion in dart now composes around the OfflineRegionDefinition class rather than inheriting its properties, so this updates the iOS classes model correspondingly, as well as JSON encoding and decoding. I had hoped to use Swift's automatic serialization via the `Codable` protocol, but the loosely typed `metadata` contained in `OfflineRegion` makes this very difficult (impossible?). This is surprising since an otherwise strict schema containing a "bag" of untyped data is fairly common in JSON, but no `Codable` examples I can find deal with it. That said, the to/from dictionary functions implemented here do the job in a straightforward way. I rather wish I hadn't wasted so much time trying to do anything different. Note that in the dart-side deserializer I made an apparently necessary change to promote `int` values to `double` for `minZoom` and `maxZoom`. Without this, I'm seeing an exception when deserializing and I would expect the same on Android.
|
The latest changes put iOS in sync once again with the dart interfaces. Any other changes we want to make here? |
n8han
changed the title
There was a problem hiding this comment.
@n8han Did you run into the issue in the example? Worked well on my device without the calls to toDouble(). OfflineRegionDefinition.getXxxZoom() return doubles on Android, which I guess maybe get serialized with .0 by the platform. In any case, adding toDouble() calls can't hurt.
|
I was testing with my own code rather than the example, but it makes sense
that different json serialization behaviors for floating point numbers with
a fractional component of zero would be the cause. 👍
Le sam. 27 févr. 2021 à 22:36, shroff <[email protected]> a écrit :
… ***@***.**** approved this pull request.
@n8han <https://github.com/n8han> Did you run into the issue in the
example? Worked well on my device without the calls to toDouble().
OfflineRegionDefinition.getXxxZoom() return doubles on Android, which I
guess maybe get serialized with .0 by the platform. In any case, adding
toDouble() calls can't hurt.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#545 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZCKXQ7S7FS3WJY2GSBRDTBG25NANCNFSM4XWNKCJA>
.
|
This is response to code review requests to call result success within the downloader (a good idea, that's where failures were already handled) and to read id and metadata back from the pack rather than returning what we had requested the sdk to put in the pack. #545 (comment) While doing this it made sense to do the id generation within the downloader, so that fewer objects are passed in and only the downloader knows the details about the sdk objects.
This implements the two changes required by #491:
Handled updated downloadOfflineRegion interface #534
The interface has changed to send
regionas a argument, as well as thechannelName. Without this change, the region download fails.
Generate offline region IDs and adapt interfaces #533
While the Android SDK generates a region ID in createOfflineRegion, the
iOS SDK does not have this feature. To maintain a parallel interface and
relieve the client of the need to generate IDs, this change generates an
ID before submitting the download request to the iOS SDK. As in the
Android implementation, the completed OfflineRegionData is immediately
returned through the open channel.
Fixes #533
The following changes suggested in #491 are also pursued here:
OfflineRegionDefinition and OfflineRegion classes to mirror SDK class
structure. This is basically equivalient to something like
OfflineRegionOptions.
return OfflineRegion as a result
Without those changes, the client code is
@requiredto submit an idalong with the original download request, even though the id will not be
used and will be replaced by the generated ID. Furthermore, the object
returned from
downloadOfflineRegionis now decoded so that the clientcan await it to extract the generated ID.
This change is not backwards compatible with regions previously
downloaded on iOS, however, iOS offline downloading is not functional
in the latest or any release of this library because of #534.
OfflineRegion and OfflineRegionDefinition
To better harmonize with the Mapbox sdk data structures for offline regions on iOS and Android, and to pave the way for polygonal offline regions, the dart classes are revamped in 71186fb. This required changes on the Android side which are also in this PR, and it will require changes in any client code using the download functionality.