Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offline region feature #336

Merged
merged 57 commits into from Dec 19, 2020
Merged

Conversation

kleeb
Copy link
Contributor

@kleeb kleeb commented Jun 22, 2020

Added iOS and Android handling of the great Mapbox feature, which allows to download and cache dynamically selected region during the app usage.

kleeb and others added 30 commits January 9, 2020 11:53
disabled debug mode
Add mapbox offline dependency.
- handle null metadata
- handle delete region that not exists in db
@AAverin
Copy link
Contributor

AAverin commented Sep 7, 2020

Does this leverage offline plugin too https://docs.mapbox.com/android/plugins/overview/offline/ ?
When downloading large maps for offline use, user should be able to dismiss the application and download should continue.
iOS documentation of Mapbox SDK is scarce, I only found this: https://docs.mapbox.com/ios/maps/examples/offline-pack/
Not sure if there is also some kind of plugin that can be used, or if iOS SDK supports background downloads out of the box.
@m0nac0 do you have any insight on that?

},
);
Iterable regions = json.decode(regionsJson);
return regions.map((region) => OfflineRegion.fromJson(region)).toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As as suggestion, we could follow this article https://flutter.dev/docs/cookbook/networking/background-parsing
and put json parsing into compute, moving it to the background.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for other places that parse json

@joandervieira
Copy link

Hi, how is this PR ? any chances for merging ? I'm really waiting for this PR to be approved in order to use it

Copy link
Collaborator

@tobrun tobrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @kleeb
Would you able to patch the remarks made?
Or if someone else is up for patching?

Looking forward getting this merged, apologies for the delay

@@ -40,6 +40,7 @@ android {
implementation "com.mapbox.mapboxsdk:mapbox-android-sdk:9.2.0"
implementation "com.mapbox.mapboxsdk:mapbox-android-plugin-annotation-v9:0.8.0"
implementation "com.mapbox.mapboxsdk:mapbox-android-plugin-localization-v9:0.12.0"
implementation "com.mapbox.mapboxsdk:mapbox-android-plugin-offline-v8:0.6.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use offline-v9 instead? Plugins have the major version of the maps SDK appended to them.

@@ -61,6 +75,14 @@ private InputStream openTilesDbFile(String tilesDb) throws IOException {
}
}

private String extractAccessToken(MethodCall methodCall, String fallbackValue) {
if (methodCall.hasArgument("accessToken")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you look into patching this @kleeb ?

@kleeb
Copy link
Contributor Author

kleeb commented Nov 9, 2020

Unfortunately not. We lost all developers that were contributing. Can any of you finish this up?

@lfofelipe
Copy link
Contributor

lfofelipe commented Nov 12, 2020

Hi guys. I created a fork with the suggested corrections and I already merged with master (0.9.0). I have a project to go into production and I couldn't wait any longer. Whoever needs this functionality, can use my fork directly in the project while I do the PR correctly.
Replace the mapbox_gl package with proper indentation:

  mapbox_gl:
    git:
      url: https://github.com/lfofelipe/flutter-mapbox-gl.git
      ref: master

Comments:

  • To work on ios, fill in info.plist:
<key>MGLMapboxAccessToken</key>
<string>YOUR_KEY_HERE</string>
  • Sometimes the example screen "Offline Regions" is not working (I didn't have time to check why)
  • Tested on iOS and Android (virtual devices seem to give connection problems, test on real devices)

I will make corrections appropriately in the coming days.
If someone can create a PR by correcting the problem of the "Offline Regions" screen example from my fork, this would be absurdly quick to merge.

Besides assigning the information that the key should exist in the info.plist ...

@lfofelipe
Copy link
Contributor

The issue "Sometimes the example screen "Offline Regions" is not working (I didn't have time to check why)" has been fixed on my fork

@kleeb
Copy link
Contributor Author

kleeb commented Nov 13, 2020

@lfofelipe would you manage to solve all conflicts between my branch and your fork ?
This would speed up the process of putting it to master

@tobrun @m0nac0 what do you think ?

@lfofelipe
Copy link
Contributor

@lfofelipe would you manage to solve all conflicts between my branch and your fork ?
This would speed up the process of putting it to master

@tobrun @m0nac0 what do you think ?

Done.

sync with bugfix branch
@kleeb
Copy link
Contributor Author

kleeb commented Nov 14, 2020

@tobrun @m0nac0
bugfix branch fixed

@lfofelipe
Copy link
Contributor

25 days and no merge ? :(

@felix-ht
Copy link
Collaborator

@tobrun hows the status for a merge?

@tobrun
Copy link
Collaborator

tobrun commented Dec 19, 2020

25 days and no merge ?

Sorry about that, I'm only able to dedicate a Saturday per month to the project.
If you are interested in becoming a maintainer, please hit me up on email!

@tobrun tobrun merged commit 9fb4d52 into flutter-mapbox-gl:master Dec 19, 2020
@kleeb
Copy link
Contributor Author

kleeb commented Dec 22, 2020

Sweet! Thanks a lot guys and all contributors.

@gabeschine
Copy link
Contributor

Hey just wanted to say THANK YOU for your time implementing this feature. Nice work :).

@tantzygames tantzygames mentioned this pull request Jan 4, 2021
@philiplindberg
Copy link
Contributor

First of all, thank you @kleeb and contributors for implementing this awesome feature 👍

I did want to make you aware of a potential bug I found when testing the example app on iOS. In offline_regions.dart, if you use downloadOfflineRegionStream instead of downloadOfflineRegion to download a particular region, the following exception is raised on iOS:

PlatformException (PlatformException(Downloading error, The operation couldn’t be completed. HTTP status code 401, null, null))

Seems to be an issue with the Mapbox access token not being properly set, even though I've defined ACCESS_TOKEN in main.dart. Again, this only seems to be happening on iOS and with the downloadOfflineRegionStream method (as opposed to downloadOfflineRegion, which works fine).

Interestingly enough, I've found that setting MGLMapboxAccessToken in Info.plist to the same access token defined in main.dart prevents the exception from being thrown.

@kleeb
Copy link
Contributor Author

kleeb commented Jan 7, 2021

Thanks @philiplindberg for letting me know. Will check on that.
If you manage to reproduce and fix would be also awesome.

@tantzygames
Copy link

I've been using Flutter_map and can cache a mapbox instance for each session, which is important as a user can open quite a few map views in my app, and mapbox charges for each time the access token is used.

I've been working towards swapping to Mapbox_gl, but it seems to require a new Mapbox instance every time a map is opened. I've tried to cache the MapboxMap, but it creates a new instance with a new controller each time.

As well as caching a region, could this also cache a Mapbox instance for a whole session?

@AAverin
Copy link
Contributor

AAverin commented Feb 13, 2021

@Kretin1 I think this is tracked here #245
There is no solution yet to have single reusable instance, but using keepAlive mixin seemed to have worked for my case so far

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet