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

Added support for taking snapshots on Android #625

Merged

Conversation

IjzerenHein
Copy link
Contributor

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:
image

# Conflicts:
#	android/src/main/java/com/airbnb/android/react/maps/AirMapModule.java
@IjzerenHein
Copy link
Contributor Author

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
Copy link

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.

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

Choose a reason for hiding this comment

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

Perhaps we can pull these strings out to CONSTANTs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE!

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

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

@IjzerenHein
Copy link
Contributor Author

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
Copy link
Collaborator

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

@IjzerenHein
Copy link
Contributor Author

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
Copy link
Collaborator

/cc @felipecsl You good with this?

Copy link
Contributor

@felipecsl felipecsl left a comment

Choose a reason for hiding this comment

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

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

try {
outputStream.close();
} catch (IOException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line, just ignore the exception

Copy link
Contributor

Choose a reason for hiding this comment

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

you could use closeQuietly here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

annotate with @Nullable

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

Choose a reason for hiding this comment

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

invert the order of these 2 ifs

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

Choose a reason for hiding this comment

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

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) {
        }
    }

promise.resolve(data);
}
}
catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@IjzerenHein
Copy link
Contributor Author

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
Copy link

Thanks, will take a look soon.

@IjzerenHein
Copy link
Contributor Author

Awesome, thanks @spikebrehm

@IjzerenHein
Copy link
Contributor Author

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

Copy link

@spikebrehm spikebrehm left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

The region logic used to be:

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

Shouldn't we preserve that here?

* @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').

Choose a reason for hiding this comment

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

Maybe add legacy here for now?

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

Choose a reason for hiding this comment

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

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 spikebrehm merged commit bf676e3 into react-native-maps:master Dec 8, 2016
@spikebrehm
Copy link

Published as v0.12.2

@IjzerenHein
Copy link
Contributor Author

Awesome, thanks so much @spikebrehm !

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

Successfully merging this pull request may close these issues.

4 participants