Added support for taking snapshots on Android #625

IjzerenHein · 2016-09-28T21:19:42Z

This pull request adds support for takeSnapshot on Android.

The takeSnapshot function was purposefully implemented on the AirMapModule class and not as a command on AirMapManager. The reason was that with using commands it is not possible to call the callback that is provided. The callback cannot be extracted from ReadableMap. This made it unsuitable for this specific function. I've taking inspiration from react-native-view-shot and was able to implement the functionality on the AirMapModule instead.

The region argument is not used on Android and the width and height are interpreted differently. This is because the GoogleMaps.snapshot doesn't take the size or region as input. This creates a feature difference with the iOS version.

Additionally, I've made takeSnapshot promise aware. This means it now returns a promise with the resulting snapshot value (the uri) (or rejects the promise in case of an error).

Android screenshot:

# Conflicts: # android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

IjzerenHein · 2016-09-28T21:40:47Z

Additionally, I'd like to make a proposal for a new and improved takeSnapshot API. The API is inspired by that react-native-view-shot but shall still be compatible with the old takeSnapshot function. When only 1 argument is specified, the function uses the new API:

const promise = map.takeSnapshot({
  width: 100, // optional, when omitted the view width is used
  height: 100, // optional, when omitted the view height is used
  result: 'file', // values: 'file', 'base64' (default: 'file')
  format: 'png', // file-format: 'png', 'jpg' (default: 'png')
  quality: 0.8, // compression-quality (only relevant for jpg) (default: 1.0)
});

The result of the promise is a file-uri when format='file' or a base-64 encoded string
when format='base64'.
Region has been removed as it cannot be implemented on Android. On both iOS and Android the currently selected region shall be used.

And for backwards compatibility the function can be used the old way:

map.takeSnapshot(300, 200, region, (err, result) => {
  ...
});

I'd like to hear your thoughts on this.

cheers, Hein

spikebrehm · 2016-09-28T23:34:38Z

This is awesome, great! I'm not super excited about the forking between iOS & Android, but it's opaque to the end developer so that's fine.

I like your new new API. We can introduce that with deprecation warning and then cut over to the new API in the subsequent breaking release.

spikebrehm · 2016-09-28T23:30:49Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                        // Save the snapshot to disk
+                        OutputStream outputStream = null;
+                        try {
+                            if ("file".equals(result)) {


Perhaps we can pull these strings out to CONSTANTs?

spikebrehm · 2016-09-28T23:31:45Z

components/MapView.js

+      if (typeof width === 'object') {
+        options = width;
+      } else {
+        options = {


Can you add a deprecation warning for this case? That way we can remove the old API in the next breaking release.

Done. I've updated the function so that it uses the old API when 4 arguments are specified (and only when on iOS). In all other cases the new API is used.
The deprecation message is generated using console.warn.

IjzerenHein · 2016-09-29T07:42:31Z

@spikebrehm Good calls! I'm gonna make those changes and let you know.

What exactly did you mean by "forking between iOS & Android" ?

# Conflicts: # components/MapView.js # ios/AirMaps/AIRMapManager.m

IjzerenHein · 2016-10-01T15:46:15Z

Alright, I've implemented the new takeSnapshot API. I've updated the iOS code to support optional width, height and region and to support the new format, quality and result arguments. There is a special case in the iOS native code to support the legacy API.

The API now looks like this:

const snapshot = this.refs.map.takeSnapshot({
    width: 300,      // optional, when omitted the view-width is used
    height: 300,     // optional, when omitted the view-height is used
    region: {..},    // iOS only, optional region to render: {latitude, longitude, latitudeDelta, longitudeDelta}
    format: 'png',   // image formats: 'png', 'jpg' (default: 'png')
    quality: 0.8,    // image quality: 0..1 (only relevant for jpg, default: 1)
    result: 'file'   // result types: 'file', 'base64' (default: 'file')
});

christopherdro · 2016-10-16T15:52:21Z

Looks great! Should we try and get this merged in for the next release ?

IjzerenHein · 2016-10-19T15:12:23Z

Any chance we can get this in for the next release?
If there's anything I need to change for that to happen let me know.

christopherdro · 2016-10-19T15:55:27Z

/cc @felipecsl You good with this?

felipecsl

@christopherdro thanks for the heads up. I left some comments but overall looks good. Can approve once they are addressed!

felipecsl · 2016-10-19T16:52:17Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                                try {
+                                    outputStream.close();
+                                } catch (IOException e) {
+                                    e.printStackTrace();


remove this line, just ignore the exception

you could use closeQuietly here too

Thanks so much for all the feedback @felipecsl . I will update the code accordingly and re-test and the update the PR. Hopefully this week, but probably next week.
Cheers, Hein

felipecsl · 2016-10-19T16:54:35Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                                outputStream = null;
+                                byte[] bytes = ((ByteArrayOutputStream) outputStream).toByteArray();
+                                String data = Base64.encodeToString(bytes, Base64.NO_WRAP);
+                                promise.resolve(data);


it would be nice if you could break down this logic into smaller methods. maybe takeBase64Snapshot() and takeFileSnapshot()?

felipecsl · 2016-10-19T16:54:50Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                    return;
+                }
+                view.map.snapshot(new GoogleMap.SnapshotReadyCallback() {
+                    public void onSnapshotReady(Bitmap snapshot) {


annotate with @Nullable

felipecsl · 2016-10-19T16:55:18Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                        if (snapshot != null && width != 0 && height != 0 && (width != snapshot.getWidth() || height != snapshot.getHeight())) {
+                            snapshot = Bitmap.createScaledBitmap(snapshot, width, height, true);
+                        }
+                        if (snapshot == null) {


invert the order of these 2 ifs

felipecsl · 2016-10-19T16:57:02Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                                File tempFile = File.createTempFile("AirMapSnapshot", "." + format, context.getCacheDir());
+                                outputStream = new FileOutputStream(tempFile);
+                                snapshot.compress(compressFormat, (int)(100.0 * quality), outputStream);
+                                outputStream.close();


close() can throw IOException. A common pattern is to create a method closeQuietly() that looks like this:

public static void closeQuietly(Closeable closeable) { if (closeable == null) return; try { closeable.close(); } catch (IOException ignored) { } }

felipecsl · 2016-10-19T16:57:41Z

android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

+                                promise.resolve(data);
+                            }
+                        }
+                        catch (Exception e) {


what exceptions are you catching exactly? can you be more specific or narrow down its scope?

IjzerenHein · 2016-11-01T14:01:05Z

Hi, it look a little longer but I've updated the code and addressed all the issues you pointed out.
Hope this gets in the next release.
regards, Hein

spikebrehm · 2016-11-10T20:46:56Z

Thanks, will take a look soon.

IjzerenHein · 2016-11-12T09:39:11Z

Awesome, thanks @spikebrehm

IjzerenHein · 2016-11-23T08:53:30Z

@spikebrehm Any idea when the next release will be?
Cheers, Hein

spikebrehm

I added some non-blocking comments. I'll merge now and feel free to follow up.

Sorry for the delay!!! We've been very busy with the release of the new Airbnb Trips product.

spikebrehm · 2016-12-08T21:43:48Z

components/MapView.js

+      this._runCommand('takeSnapshot', [
+        width || 0,
+        height || 0,
+        region || {},


The region logic used to be:

const finalRegion = region || this.props.region || this.props.initialRegion;

Shouldn't we preserve that here?

spikebrehm · 2016-12-08T23:32:18Z

components/MapView.js

+   * @param [config.region] Region to render (Only supported on iOS).
+   * @param [config.format] Encoding format ('png', 'jpg') (default: 'png').
+   * @param [config.quality] Compression quality (only used for jpg) (default: 1.0).
+   * @param [config.result] Result format ('file', 'base64') (default: 'file').


Maybe add legacy here for now?

spikebrehm · 2016-12-08T23:35:03Z

components/MapView.js

+      quality: args.quality || 1.0,
+      result: args.result || 'file',
+    };
+    if ((config.format !== 'png') &&


Consider pulling these out to constants at the top of the file, like:

const VALID_FORMATS = ['png', 'jpg']; const VALID_RESULTS = ['file', 'base64']; // ... if (!VALID_FORMATS.include(config.format)) { throw new Error('Invalid format specified'); }

Also it may be nice to use the invariant package like:

invariant(VALID_FORMATS.include(config.format), 'Invalid format specified');

spikebrehm · 2016-12-10T00:18:12Z

Published as v0.12.2

IjzerenHein · 2016-12-11T09:58:12Z

Awesome, thanks so much @spikebrehm !

IjzerenHein added 2 commits September 28, 2016 23:07

Added support for taking snapshots on Android

1d9d529

# Conflicts: # android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java

Fixed eslint errors in MapView.js

38ce6d5

spikebrehm approved these changes Sep 28, 2016

View reviewed changes

IjzerenHein added 3 commits October 1, 2016 17:34

Added Const definition for snapshot result & format on Android

3334e16

Implemented new takeSnapshot API for iOS

8da7db7

# Conflicts: # components/MapView.js # ios/AirMaps/AIRMapManager.m

Updated README with new Snapshot API

fe61cd1

felipecsl reviewed Oct 19, 2016

View reviewed changes

Improved code for takeSnapshot function (based on airbnb feedback)

b12503c

spikebrehm approved these changes Dec 8, 2016

View reviewed changes

spikebrehm merged commit bf676e3 into react-native-maps:master Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for taking snapshots on Android #625

Added support for taking snapshots on Android #625

IjzerenHein commented Sep 28, 2016

IjzerenHein commented Sep 28, 2016

spikebrehm commented Sep 28, 2016

spikebrehm Sep 28, 2016

IjzerenHein Oct 1, 2016

spikebrehm Sep 28, 2016

IjzerenHein Oct 1, 2016

IjzerenHein commented Sep 29, 2016

IjzerenHein commented Oct 1, 2016

christopherdro commented Oct 16, 2016

IjzerenHein commented Oct 19, 2016

christopherdro commented Oct 19, 2016

felipecsl left a comment

felipecsl Oct 19, 2016

felipecsl Oct 19, 2016

IjzerenHein Oct 19, 2016

felipecsl Oct 19, 2016

felipecsl Oct 19, 2016

felipecsl Oct 19, 2016

felipecsl Oct 19, 2016

felipecsl Oct 19, 2016

IjzerenHein commented Nov 1, 2016

spikebrehm commented Nov 10, 2016

IjzerenHein commented Nov 12, 2016

IjzerenHein commented Nov 23, 2016

spikebrehm left a comment

spikebrehm Dec 8, 2016

spikebrehm Dec 8, 2016

spikebrehm Dec 8, 2016

spikebrehm commented Dec 10, 2016

IjzerenHein commented Dec 11, 2016

Added support for taking snapshots on Android #625

Added support for taking snapshots on Android #625

Conversation

IjzerenHein commented Sep 28, 2016

IjzerenHein commented Sep 28, 2016

spikebrehm commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IjzerenHein commented Sep 29, 2016

IjzerenHein commented Oct 1, 2016

christopherdro commented Oct 16, 2016

IjzerenHein commented Oct 19, 2016

christopherdro commented Oct 19, 2016

felipecsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IjzerenHein commented Nov 1, 2016

spikebrehm commented Nov 10, 2016

IjzerenHein commented Nov 12, 2016

IjzerenHein commented Nov 23, 2016

spikebrehm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spikebrehm commented Dec 10, 2016

IjzerenHein commented Dec 11, 2016