From 9baff8f437eee49f8ab0e6f433bf86466ca16662 Mon Sep 17 00:00:00 2001 From: Dave Miller Date: Thu, 21 Jan 2016 08:07:01 -0800 Subject: [PATCH] Make CameraRoll work with Promises Summary: public This is the first module moving to the new model of working with Promises. We now warn on uses of callback version. At some point we will remove that. Reviewed By: davidaurelio Differential Revision: D2849811 fb-gh-sync-id: 8a31924cc2b438efc58f3ad22d5f27c273563472 --- Libraries/CameraRoll/CameraRoll.js | 69 +++++++++---------- Libraries/CameraRoll/RCTCameraRollManager.m | 45 ++++++------ Libraries/Utilities/MessageQueue.js | 20 ++++-- Libraries/Utilities/Platform.android.js | 4 +- .../com/facebook/react/bridge/Promise.java | 4 +- .../facebook/react/bridge/PromiseImpl.java | 2 +- .../modules/camera/CameraRollManager.java | 56 +++++++-------- 7 files changed, 105 insertions(+), 95 deletions(-) diff --git a/Libraries/CameraRoll/CameraRoll.js b/Libraries/CameraRoll/CameraRoll.js index 5ea9b94677afb7..3b4b0ed1a2009c 100644 --- a/Libraries/CameraRoll/CameraRoll.js +++ b/Libraries/CameraRoll/CameraRoll.js @@ -128,57 +128,56 @@ class CameraRoll { * - a tag not matching any of the above, which means the image data will * be stored in memory (and consume memory as long as the process is alive) * - * @param successCallback Invoked with the value of `tag` on success. - * @param errorCallback Invoked with error message on error. + * Returns a Promise which when resolved will be passed the new uri + * */ - static saveImageWithTag(tag, successCallback, errorCallback) { + static saveImageWithTag(tag) { invariant( typeof tag === 'string', 'CameraRoll.saveImageWithTag tag must be a valid string.' ); - RCTCameraRollManager.saveImageWithTag( - tag, - (imageTag) => { - successCallback && successCallback(imageTag); - }, - (errorMessage) => { - errorCallback && errorCallback(errorMessage); - }); + if (arguments.length > 1) { + console.warn("CameraRoll.saveImageWithTag(tag, success, error) is deprecated. Use the returned Promise instead"); + let successCallback = arguments[1]; + let errorCallback = arguments[2] || ( () => {} ); + RCTCameraRollManager.saveImageWithTag(tag).then(successCallback, errorCallback); + return; + } + return RCTCameraRollManager.saveImageWithTag(tag); } /** - * Invokes `callback` with photo identifier objects from the local camera + * Returns a Promise with photo identifier objects from the local camera * roll of the device matching shape defined by `getPhotosReturnChecker`. * * @param {object} params See `getPhotosParamChecker`. - * @param {function} callback Invoked with arg of shape defined by - * `getPhotosReturnChecker` on success. - * @param {function} errorCallback Invoked with error message on error. + * + * Returns a Promise which when resolved will be of shape `getPhotosReturnChecker` + * */ - static getPhotos(params, callback, errorCallback) { - var metaCallback = callback; + static getPhotos(params) { if (__DEV__) { getPhotosParamChecker({params}, 'params', 'CameraRoll.getPhotos'); - invariant( - typeof callback === 'function', - 'CameraRoll.getPhotos callback must be a valid function.' - ); - invariant( - typeof errorCallback === 'function', - 'CameraRoll.getPhotos errorCallback must be a valid function.' - ); } - if (__DEV__) { - metaCallback = (response) => { - getPhotosReturnChecker( - {response}, - 'response', - 'CameraRoll.getPhotos callback' - ); - callback(response); - }; + if (arguments.length > 1) { + console.warn("CameraRoll.getPhotos(tag, success, error) is deprecated. Use the returned Promise instead"); + let successCallback = arguments[1]; + if (__DEV__) { + let callback = arguments[1]; + successCallback = (response) => { + getPhotosReturnChecker( + {response}, + 'response', + 'CameraRoll.getPhotos callback' + ); + callback(response); + }; + } + let errorCallback = arguments[2] || ( () => {} ); + RCTCameraRollManager.getPhotos(params).then(successCallback, errorCallback); } - RCTCameraRollManager.getPhotos(params, metaCallback, errorCallback); + // TODO: Add the __DEV__ check back in to verify the Promise result + return RCTCameraRollManager.getPhotos(params); } } diff --git a/Libraries/CameraRoll/RCTCameraRollManager.m b/Libraries/CameraRoll/RCTCameraRollManager.m index ad4ad171371c8c..fc97c9493b4af8 100644 --- a/Libraries/CameraRoll/RCTCameraRollManager.m +++ b/Libraries/CameraRoll/RCTCameraRollManager.m @@ -79,13 +79,16 @@ @implementation RCTCameraRollManager @synthesize bridge = _bridge; +NSString *const RCTErrorUnableToLoad = @"E_UNABLE_TO_LOAD"; +NSString *const RCTErrorUnableToSave = @"E_UNABLE_TO_SAVE"; + RCT_EXPORT_METHOD(saveImageWithTag:(NSString *)imageTag - successCallback:(RCTResponseSenderBlock)successCallback - errorCallback:(RCTResponseErrorBlock)errorCallback) + resolve:(RCTPromiseResolveBlock)resolve + reject:(RCTPromiseRejectBlock)reject) { [_bridge.imageLoader loadImageWithTag:imageTag callback:^(NSError *loadError, UIImage *loadedImage) { if (loadError) { - errorCallback(loadError); + reject(RCTErrorUnableToLoad, nil, loadError); return; } // It's unclear if writeImageToSavedPhotosAlbum is thread-safe @@ -93,21 +96,21 @@ @implementation RCTCameraRollManager [_bridge.assetsLibrary writeImageToSavedPhotosAlbum:loadedImage.CGImage metadata:nil completionBlock:^(NSURL *assetURL, NSError *saveError) { if (saveError) { RCTLogWarn(@"Error saving cropped image: %@", saveError); - errorCallback(saveError); + reject(RCTErrorUnableToSave, nil, saveError); } else { - successCallback(@[assetURL.absoluteString]); + resolve(@[assetURL.absoluteString]); } }]; }); }]; } -static void RCTCallCallback(RCTResponseSenderBlock callback, - NSArray *> *assets, - BOOL hasNextPage) +static void RCTResolvePromise(RCTPromiseResolveBlock resolve, + NSArray *> *assets, + BOOL hasNextPage) { if (!assets.count) { - callback(@[@{ + resolve(@[@{ @"edges": assets, @"page_info": @{ @"has_next_page": @NO, @@ -115,7 +118,7 @@ static void RCTCallCallback(RCTResponseSenderBlock callback, }]); return; } - callback(@[@{ + resolve(@[@{ @"edges": assets, @"page_info": @{ @"start_cursor": assets[0][@"node"][@"image"][@"uri"], @@ -126,8 +129,8 @@ static void RCTCallCallback(RCTResponseSenderBlock callback, } RCT_EXPORT_METHOD(getPhotos:(NSDictionary *)params - successCallback:(RCTResponseSenderBlock)successCallback - errorCallback:(RCTResponseErrorBlock)errorCallback) + resolve:(RCTPromiseResolveBlock)resolve + reject:(RCTPromiseRejectBlock)reject) { NSUInteger first = [RCTConvert NSInteger:params[@"first"]]; NSString *afterCursor = [RCTConvert NSString:params[@"after"]]; @@ -137,7 +140,7 @@ static void RCTCallCallback(RCTResponseSenderBlock callback, BOOL __block foundAfter = NO; BOOL __block hasNextPage = NO; - BOOL __block calledCallback = NO; + BOOL __block resolvedPromise = NO; NSMutableArray *> *assets = [NSMutableArray new]; [_bridge.assetsLibrary enumerateGroupsWithTypes:groupTypes usingBlock:^(ALAssetsGroup *group, BOOL *stopGroups) { @@ -157,9 +160,9 @@ static void RCTCallCallback(RCTResponseSenderBlock callback, *stopAssets = YES; *stopGroups = YES; hasNextPage = YES; - RCTAssert(calledCallback == NO, @"Called the callback before we finished processing the results."); - RCTCallCallback(successCallback, assets, hasNextPage); - calledCallback = YES; + RCTAssert(resolvedPromise == NO, @"Resolved the promise before we finished processing the results."); + RCTResolvePromise(resolve, assets, hasNextPage); + resolvedPromise = YES; return; } CGSize dimensions = [result defaultRepresentation].dimensions; @@ -188,18 +191,18 @@ static void RCTCallCallback(RCTResponseSenderBlock callback, } }]; } else { - // Sometimes the enumeration continues even if we set stop above, so we guard against calling the callback + // Sometimes the enumeration continues even if we set stop above, so we guard against resolving the promise // multiple times here. - if (!calledCallback) { - RCTCallCallback(successCallback, assets, hasNextPage); - calledCallback = YES; + if (!resolvedPromise) { + RCTResolvePromise(resolve, assets, hasNextPage); + resolvedPromise = YES; } } } failureBlock:^(NSError *error) { if (error.code != ALAssetsLibraryAccessUserDeniedError) { RCTLogError(@"Failure while iterating through asset groups %@", error); } - errorCallback(error); + reject(RCTErrorUnableToLoad, nil, error); }]; } diff --git a/Libraries/Utilities/MessageQueue.js b/Libraries/Utilities/MessageQueue.js index f22094bbead021..8e58ee06e3c71f 100644 --- a/Libraries/Utilities/MessageQueue.js +++ b/Libraries/Utilities/MessageQueue.js @@ -16,6 +16,7 @@ let Systrace = require('Systrace'); let ErrorUtils = require('ErrorUtils'); let JSTimersExecution = require('JSTimersExecution'); +let Platform = require('Platform'); let invariant = require('invariant'); let keyMirror = require('keyMirror'); @@ -318,10 +319,21 @@ class MessageQueue { if (type === MethodTypes.remoteAsync) { fn = function(...args) { return new Promise((resolve, reject) => { - self.__nativeCall(module, method, args, resolve, (errorData) => { - var error = createErrorFromErrorData(errorData); - reject(error); - }); + self.__nativeCall( + module, + method, + args, + (data) => { + // iOS always wraps the data in an Array regardless of what the + // shape of the data so we strip it out + // Android sends the data back properly + // TODO: Remove this once iOS has support for Promises natively (t9774697) + resolve(Platform.OS == 'ios' ? data[0] : data); + }, + (errorData) => { + var error = createErrorFromErrorData(errorData); + reject(error); + }); }); }; } else { diff --git a/Libraries/Utilities/Platform.android.js b/Libraries/Utilities/Platform.android.js index 2d061d4148d0f8..659d9c873a1c16 100644 --- a/Libraries/Utilities/Platform.android.js +++ b/Libraries/Utilities/Platform.android.js @@ -12,11 +12,9 @@ 'use strict'; -var AndroidConstants = require('NativeModules').AndroidConstants; - var Platform = { OS: 'android', - Version: AndroidConstants.Version, + get Version() { return require('NativeModules').AndroidConstants.Version; }, }; module.exports = Platform; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java index 84b802b28affae..5b107fd99a8d4d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java @@ -9,6 +9,8 @@ package com.facebook.react.bridge; +import javax.annotation.Nullable; + /** * Interface that represents a JavaScript Promise which can be passed to the native module as a * method parameter. @@ -22,5 +24,5 @@ public interface Promise { @Deprecated void reject(String reason); void reject(String code, Throwable extra); - void reject(String code, String reason, Throwable extra); + void reject(String code, String reason, @Nullable Throwable extra); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java index efbc4537592d52..0c1c85c9e6d614 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java @@ -50,7 +50,7 @@ public void reject(String code, Throwable extra) { } @Override - public void reject(String code, String reason, Throwable extra) { + public void reject(String code, String reason, @Nullable Throwable extra) { if (mReject != null) { if (code == null) { code = DEFAULT_ERROR; diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java b/ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java index 289a045bfdba98..357586aa13c628 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java @@ -36,8 +36,8 @@ import android.text.TextUtils; import com.facebook.common.logging.FLog; -import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.GuardedAsyncTask; +import com.facebook.react.bridge.Promise; import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReactContextBaseJavaModule; @@ -60,6 +60,9 @@ public class CameraRollManager extends ReactContextBaseJavaModule { private static final String TAG = "Catalyst/CameraRollManager"; + private static final String ERROR_UNABLE_TO_LOAD = "E_UNABLE_TO_LOAD"; + private static final String ERROR_UNABLE_TO_LOAD_PERMISSION = "E_UNABLE_TO_LOAD_PERMISSION"; + private static final String ERROR_UNABLE_TO_SAVE = "E_UNABLE_TO_SAVE"; public static final boolean IS_JELLY_BEAN_OR_LATER = Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN; @@ -112,14 +115,11 @@ public Map getConstants() { * by the MediaScanner. * * @param uri the file:// URI of the image to save - * @param success callback to be invoked on successful save to gallery; the only argument passed - * to this callback is the MediaStore content:// URI of the new image. - * @param error callback to be invoked on error (e.g. can't copy file, external storage not - * available etc.) + * @param promise to be resolved or rejected */ @ReactMethod - public void saveImageWithTag(String uri, final Callback success, final Callback error) { - new SaveImageTag(getReactApplicationContext(), Uri.parse(uri), success, error) + public void saveImageWithTag(String uri, Promise promise) { + new SaveImageTag(getReactApplicationContext(), Uri.parse(uri), promise) .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); } @@ -127,15 +127,13 @@ private static class SaveImageTag extends GuardedAsyncTask { private final Context mContext; private final Uri mUri; - private final Callback mSuccess; - private final Callback mError; + private final Promise mPromise; - public SaveImageTag(ReactContext context, Uri uri, Callback success, Callback error) { + public SaveImageTag(ReactContext context, Uri uri, Promise promise) { super(context); mContext = context; mUri = uri; - mSuccess = success; - mError = error; + mPromise = promise; } @Override @@ -147,7 +145,7 @@ protected void doInBackgroundGuarded(Void... params) { Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_PICTURES); pictures.mkdirs(); if (!pictures.isDirectory()) { - mError.invoke("External storage pictures directory not available"); + mPromise.reject(ERROR_UNABLE_TO_LOAD, "External storage pictures directory not available", null); return; } File dest = new File(pictures, source.getName()); @@ -178,14 +176,14 @@ protected void doInBackgroundGuarded(Void... params) { @Override public void onScanCompleted(String path, Uri uri) { if (uri != null) { - mSuccess.invoke(uri.toString()); + mPromise.resolve(uri.toString()); } else { - mError.invoke("Could not add image to gallery"); + mPromise.reject(ERROR_UNABLE_TO_SAVE, "Could not add image to gallery", null); } } }); } catch (IOException e) { - mError.invoke(e.getMessage()); + mPromise.reject(e); } finally { if (input != null && input.isOpen()) { try { @@ -221,12 +219,11 @@ public void onScanCompleted(String path, Uri uri) { * image/jpeg) * * - * @param success the callback to be called when the photos are loaded; for a format of the + * @param promise the Promise to be resolved when the photos are loaded; for a format of the * parameters passed to this callback, see {@code getPhotosReturnChecker} in CameraRoll.js - * @param error the callback to be called on error */ @ReactMethod - public void getPhotos(final ReadableMap params, final Callback success, Callback error) { + public void getPhotos(final ReadableMap params, final Promise promise) { int first = params.getInt("first"); String after = params.hasKey("after") ? params.getString("after") : null; String groupName = params.hasKey("groupName") ? params.getString("groupName") : null; @@ -243,8 +240,7 @@ public void getPhotos(final ReadableMap params, final Callback success, Callback after, groupName, mimeTypes, - success, - error) + promise) .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); } @@ -254,8 +250,7 @@ private static class GetPhotosTask extends GuardedAsyncTask { private final @Nullable String mAfter; private final @Nullable String mGroupName; private final @Nullable ReadableArray mMimeTypes; - private final Callback mSuccess; - private final Callback mError; + private final Promise mPromise; private GetPhotosTask( ReactContext context, @@ -263,16 +258,14 @@ private GetPhotosTask( @Nullable String after, @Nullable String groupName, @Nullable ReadableArray mimeTypes, - Callback success, - Callback error) { + Promise promise) { super(context); mContext = context; mFirst = first; mAfter = after; mGroupName = groupName; mMimeTypes = mimeTypes; - mSuccess = success; - mError = error; + mPromise = promise; } @Override @@ -309,18 +302,21 @@ protected void doInBackgroundGuarded(Void... params) { Images.Media.DATE_TAKEN + " DESC, " + Images.Media.DATE_MODIFIED + " DESC LIMIT " + (mFirst + 1)); // set LIMIT to first + 1 so that we know how to populate page_info if (photos == null) { - mError.invoke("Could not get photos"); + mPromise.reject(ERROR_UNABLE_TO_LOAD, "Could not get photos", null); } else { try { putEdges(resolver, photos, response, mFirst); putPageInfo(photos, response, mFirst); } finally { photos.close(); - mSuccess.invoke(response); + mPromise.resolve(response); } } } catch (SecurityException e) { - mError.invoke("Could not get photos: need READ_EXTERNAL_STORAGE permission"); + mPromise.reject( + ERROR_UNABLE_TO_LOAD_PERMISSION, + "Could not get photos: need READ_EXTERNAL_STORAGE permission", + e); } } }