[offline][android] use region ids provided by MB + code cleanup #491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Merge `downloadOfflineRegion` and `downloadOfflineRegionStream` - change format of args from single json string to obj with `accessToken`, `channelName`, and `region` keys - Do not store region id in metadata as this is unnecessary - Perform `deleteOfflineRegion` based on ID given by mapbox at creation time rather than arbitrarily self-assigned id - Return `OfflineRegionData` result from the created `OfflineRegion` upon callback of `onCreate` in `createOfflineRegion` (needed to pass back ID to dart) Considered but not implemented mostly because of lack of iOS resources: - Restructure `OfflineRegionData`(native)/`OfflineRegion`(dart) classes into `OfflineRegionDefinition` and `OfflineRegion` classes to mirror SDK class structure. - Accept `OfflineRegionDefinition` as arg to `downloadOfflineRegion`, and return `OfflineRegion` as a result
|
This looks really good! Could we get this merged? |
|
Web build failure is unrelated |
tobrun
approved these changes
Feb 12, 2021
|
Thank you @shroff for contributing and patching based on the recent changes! Much appreciated! |
This was referenced Feb 12, 2021
n8han
added a commit
that referenced
this pull request
Feb 16, 2021
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.
shroff
pushed a commit
that referenced
this pull request
Feb 28, 2021
…with iOS and Android (#545) * Handled updated downloadOfflineRegion interface The interface has changed to send region as a argument, as well as the channelName. Without this change, the region download fails. Fixes #534 * Generate offline region IDs and adapt interfaces 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. * 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#336 was a great step forward for offline, but there's a lot that could be cleaned up IMO. Particularly, I want to implement
mergeOfflineRegions, which is hard with the current design of assigning region ids, especially since one db file can contain multiple regions.Current Changes (android only):
downloadOfflineRegionanddownloadOfflineRegionStreamaccessToken,channelName, andregionkeys (this will break iOS unless updated, but the change is simple)idpassed intodownloadOfflineRegionviaOfflineRegionData, as it will be assigned bycreateOfflineRegion.OfflineRegionDatametadata as this is unnecessary and misleading.deleteOfflineRegionbased on ID given by mapbox at creation time rather than arbitrarily self-assigned idOfflineRegionDataresult from the createdOfflineRegionupon callback ofonCreateincreateOfflineRegion(needed to pass back ID to dart) instead of waiting for the download to finish, as there is already an event stream set up to take care of the download progressConsidered but not implemented mostly because of lack of iOS resources:
OfflineRegionData(native)/OfflineRegion(dart) classes intoOfflineRegionDefinitionandOfflineRegionclasses to mirror SDK class structure. This is basically equivalient to something likeOfflineRegionOptions.OfflineRegionDefinitionas an arg todownloadOfflineRegion, and returnOfflineRegionas a result