Added MBTiles support for iOS and Android

[Stophface]

Does any other open PR do the same thing?

No, there is no other Pull Request doing the same thing.

What issue is this PR fixing?

No issue, it is a new feature.
Requested e.g. here: #1946, #1876

How did you test this PR?

I implemented an application locally with it. It can be recreated with this repo: https://github.com/Stophface/maps. If needed I can create a branch from the repo I doing the pull request with to make an example application.

• Which platform (eg. Android/iOS) & Maps API (eg. Google/Apple) does this change affect, if any?
  -> Its changes affects both platforms.

• Did you test this on a real device, or in a simulator?
  -> On iOS and Android simulator.

• Are there any platforms you were not able to test?
  -> No.

Generally

This feature adds MBTiles support for react-native-maps. The component LocalTile is great. But when there are a lot of maptiles, the downloading, unzipping and managing on the dvivce becomes ressource intensive and slow. MBTiles is a widely known format to store Map Tiles in a SQLite database. The MBTiles specification was created by Mapbox.

Set up a project as usual and include these lines into your application:

<MapView.MbTile
    pathTemplate={"path/to/mbtiles/database.mbtiles"}
    tileSize={256} />

Make sure to include the .mbtiles ending at the path!
An MBTiles database can easily be created with various tools e.g.

• https://github.com/mapbox/mbtiler
• http://www.gdal.org/frmt_mbtiles.html

Also make sure that the database uses the z/x/y specification of Google, and not the TMS or another Tile-Specification (information)! The MBTiles implementation is based on #1876. It uses FMDB on iOS to access the database. FMDB is statically linked to AirMaps.xcodeproj in lib/ios/. On Android it uses JDBC to connect to the database. No linking needed here. JDBC comes natively with Android. For a complete history please see this repository: https://github.com/Stophface/react-native-maps-0.20.1.

[h3ll0w0rld123]

Awesome work! Glad you got it to work. I had a few questions about certain things, hope you can inform me.

[h3ll0w0rld123]

Sorry I'm not too familiar with ios, but isn't there an included sqlite lib? Can you explain the rationale of including a large library?

[Stophface]

The FMDB library is not that large - 173KB in total. It makes working with SQLite way simpler and compared to the native SQLite3 only a few lines of code are needed. The native SQLite3 lib is sort of a raw library which makes it hard to use. One needs a lot of lines of code to achieve what can be achieved with the FMDB library with only a few lines. Further, I have the feeling that the FMDB database is the standard when it comes to SQLite access on iOS: Rewriting it to SQLite3would make it harder for other collaborators to maintain and extend the code. Just look at the online ressources around FMDB and SQLite3. I think it is better to stick with FMDB. Rewriting to SQLite3 would make the code overly complicated and less maintainable. Maintainability and extenability is in my opinion more important on a community based project than adding 173KB of size to the project.

[h3ll0w0rld123]

Interesting, can someone with more iOS experience than I do comment?

[h3ll0w0rld123]

Consider the following (may contain syntax errors since I'm not familiar with this language)

NSData *tile = nil
if ([databaseResult next]) {
  tile = [databaseResult dataForColumn:@"tile_data"];
}
[offlineDataDatabase close];
result(tile,nil);

[Stophface]

I won't change that due to code consistency. Look at

https://github.com/react-community/react-native-maps/blob/29fb4a9228b6a7d33881c0d74e403f68b60e4bef/lib/ios/AirMaps/AIRMapLocalTileOverlay.m#L22-L28

which is the template for my feature. I think consistency is better here. Thus I will stick with

if ([databaseResult next]) {
    NSData *tile = [databaseResult dataForColumn:@"tile_data"];
    [offlineDataDatabase close];
    result(tile,nil);
} else {
    [offlineDataDatabase close];
    result(nil,nil);
}

[h3ll0w0rld123]

I believe catching a generic exception like this might lead to some bugs. In general, it is better to be specific about your exceptions when you know they exist, like this.

Consider the following:

1. You open a connection successfully, but your string replace (currently line line 55) throws an error. I believe you will end up not closing the database.

I would look into other code that uses this lib and see how they do error handling, but on very cursory glance, I would structure it like the following:

try {
  SQLiteDatabase offlineDataDatabase offlineDataDatabase = SQLiteDatabase.openDatabase(this.pathTemplate, null, SQLiteDatabase.OPEN_READONLY);
  String query = rawQuery.replace("{x}", Integer.toString(x)).replace("{y}", Integer.toString(y))
                                                                                                    .replace("{z}", Integer.toString(zoom));
  Cursor cursor = offlineDataDatabase.rawQuery(query, null);
  byte[] tile = null
  if(cursor.moveToFirst()) {
    tile = cursor.getBlob(5);
    cursor.close();
  } 
  offlineDataDatabase.close();
  return tile;
} catch (SQLiteCantOpenDatabaseException e) {
  // error handle, this case is when database can't open so return null?
} catch (android.database.sqlite.SQLiteException e) {
  // error handle, past an open database connection so close and return null.
} catch (Exception e) {
  // all other cases, return null?
}

[h3ll0w0rld123]

Possibly even better:

byte[] tile = null
try {
  SQLiteDatabase offlineDataDatabase offlineDataDatabase = SQLiteDatabase.openDatabase(this.pathTemplate, null, SQLiteDatabase.OPEN_READONLY);
  String query = rawQuery.replace("{x}", Integer.toString(x)).replace("{y}", Integer.toString(y))
                                                                                                    .replace("{z}", Integer.toString(zoom));
  Cursor cursor = offlineDataDatabase.rawQuery(query, null);
  if(cursor.moveToFirst()) {
    tile = cursor.getBlob(5);
    cursor.close();
  } 
  offlineDataDatabase.close();
} catch (SQLiteCantOpenDatabaseException e) {
  // error handle, this case is when database can't open so return null?
} catch (android.database.sqlite.SQLiteException e) {
  // error handle, past an open database connection so close and return null.
} catch (Exception e) {
  // all other cases, return null?
}

return tile;

[Stophface]

Thanks for pointing that out. I changed the "closing the database" problem and included some catch. I prefer the code written slightly different, so I left that. I included a more elaborated error handling namely SQLiteCantOpenDatabaseException, SQLiteDatabaseCorruptException and SQLiteDatabaseLockedException.

https://github.com/Stophface/react-native-maps/blob/a4b5460d37f901a2befd36481461412f18ac0c3c/lib/android/src/main/java/com/airbnb/android/react/maps/AirMapMbTile.java#L55-L81

byte[] tile = null;
try {
    SQLiteDatabase offlineDataDatabase = SQLiteDatabase.openDatabase(this.pathTemplate, null, SQLiteDatabase.OPEN_READONLY);
    String query = rawQuery.replace("{x}", Integer.toString(x))
        .replace("{y}", Integer.toString(y))
        .replace("{z}", Integer.toString(zoom));
    Cursor cursor = offlineDataDatabase.rawQuery(query, null);
        if (cursor.moveToFirst()) {
            tile = cursor.getBlob(5);
        }
    cursor.close();
    offlineDataDatabase.close();
    } catch (SQLiteCantOpenDatabaseException e) {
        e.printStackTrace();
        throw e;
    } catch (SQLiteDatabaseCorruptException e) {
        e.printStackTrace();
        throw e;
    } catch (SQLiteDatabaseLockedException e) {
        e.printStackTrace();
        throw e;
    } catch (Exception e) {
        e.printStackTrace();
        throw e;
    } finally {
        return tile;
    }

[h3ll0w0rld123]

My personal preference is to consolidate all cleanup and return into one section to prevent future bugs, see code snippet below.

[Stophface]

I changed it to which makes more sense to me.

if (cursor.moveToFirst()) {
    tile = cursor.getBlob(5);
}
cursor.close();
offlineDataDatabase.close();
return tile;

[h3ll0w0rld123]

Whitespace between ) and {, 'if' and '('.

[Stophface]

Added whitespaces.

if (cursor.moveToFirst()) {
    tile = cursor.getBlob(5);
}

[h3ll0w0rld123]

Does this function need error handling?

[Stophface]

SQLite does not return execptions, only an error code for which one can check. I added a check if a file exists at the given path.

NSFileManager *fileManager = [NSFileManager defaultManager];
if ([fileManager fileExistsAtPath:self.URLTemplate]) {
    FMDatabase *offlineDataDatabase = [FMDatabase databaseWithPath:self.URLTemplate];
    [offlineDataDatabase open];
    ....
} else {
    NSLog(@"Database not found. Wrong path");
}
``

[h3ll0w0rld123]

Out of curiosity, what appears for the tile in this case? Is it blank? I see https://github.com/react-community/react-native-maps/blob/f1126a4f307084005f7b62b0dc50fbfcfc810cd4/lib/android/src/main/java/com/airbnb/android/react/maps/AirMapLocalTile.java#L62 does it too so I assume it's standard.

[Stophface]

Yep, should be blank. iOS and Android.

[Stophface]

I won't have time the next ~two weeks to look into this. Using SQLite instead of FMDB makes sense. The rest I have to check.

[h3ll0w0rld123]

No worries at all, I would love to help more if I can, but not sure if I can be that helpful implementation wise. I won't be needing this for a few weeks myself, but when the time comes I will definitely be more proactive about getting something like this out.

Whom should we talk to to also review this?

[h3ll0w0rld123]

@Stophface Any updates on timetables? No rush, just wanted a heads up. In 2 or so weeks I'll have to start pushing this along and fork my own repo if I have to--would highly prefer to not because It'll take me a lot longer to pick up on the native code.

Do we have any one else to code review too to get this moving along?

[Stophface]

@h3ll0w0rld123 I think in around two weeks I have time to look into this. Maybe a few days earlier. Pretty much snowed in with work at the moment.

It works though, so you can use it :)

[h3ll0w0rld123]

Good to know @Stophface, but I'll need any code to be merged into the mainline before using it. I'm also using Expo, so it takes them a while to accept the new changes as well, which is why I wanted to get this moving. Do you know who reviews PR for this project that we can tag?

[h3ll0w0rld123]

@rborn @zavadpe @alvelig Since you guys were involved with #1876 do you mind taking a look at this PR and let @Stophface and me know if there needs to be any changes?

[alvelig]

@zavadpe 🐽

[ahmetabdi]

🙏 Get this in please

[ahmetabdi]

@terribleben Is another name to tag

[h3ll0w0rld123]

@Stophface have you gotten more time to look at this?

[zavadpe]

@Stophface Sorry, been busy last couple of weeks. From a brief look I took it looks quite fine. I would consider notes from @h3ll0w0rld123 and you've got some conflicts to solve. I can take a deeper look then.

[Stophface]

@zavadpe Yeah the notes from @h3ll0w0rld123 are valuable. I might be able to look into it on the weekend.
Regarding the conflicts: I do not have write access to repo so I cannot resolve them.

[zavadpe]

@Stophface Conflicts must be resolved in your fork repository..

[Stophface]

I commented on all the comments of @h3ll0w0rld123 and inserted the changes into my code as written in the comments (@alvelig, @ahmetabdi). I am working on resolving the merge conflicts @zavadpe.

[Stophface]

@h3ll0w0rld123, @alvelig, @ahmetabdi, @zavadpe, @mlc I added the changes, resolved the merge conflicts. I also extended the Readme and included an example file.

If everyone is happy with my changes (see comments on @h3ll0w0rld123 suggestions) and the merge conflicts are properly solved, someone should merge it?!

[h3ll0w0rld123]

@Stophface looks like your travis ci build has some errors, looks like it's a linter problem.

Anyone else can review this?

[Stophface]

@stobis @rborn Well yeah. I didn't remove it....

[rborn]

@Stophface any chance to fix this? I'll try to revert the merge

[rborn]

Ok, looks like it worked.
@alvelig @h3ll0w0rld123 @zavadpe @alvelig @Stophface Maybe we could put some tests in place to avoid breaking things like now?

[Stophface]

@rborn Not sure how to fix this. Did you remove the line?! I am positive that I did not touch any imports in AirMapManager.java.

I can add the import though if that helps...

[rborn]

I didn't touch anything, just merged after @alvelig LGTM-ed (is that a word? 😹) the PR.
Would be good if you do this and maybe @stobis could test the PR before merging it.

[Stophface]

@rborn haha yes, that definitly is a word ;)

I added the line and pushed it. Do I need to make a new PR?! Itis not showing here. Its online though: https://github.com/Stophface/react-native-maps/commit/e1296bb3b6ad975ecc5e453286edce2bc5180f06

[rborn]

@Stophface I believe yes

[Stophface]

@rborn @alvelig @stobis @zavadpe @h3ll0w0rld123 I opened a new PR #2388