patch ImagePickerIOS to return video width and height #2

victorias · 2019-07-10T08:00:38Z

Currently ImagePickerIOS does not return a width or height for videos selected in the Photo Library. This change patches our RN repo to grab the width and height data from the PHAsset and return it to JS through ImagePickerIOS.

This code was shamelessly stolen from Expo, who already handles this case:
https://github.com/expo/expo/blob/master/packages/expo-image-picker/ios/EXImagePicker/EXImagePicker.m#L297

detects movies

mrkcsc · 2019-07-10T17:49:20Z

Libraries/CameraRoll/RCTImagePickerManager.m

+    PHFetchResult<PHAsset *> *assets = [PHAsset fetchAssetsWithALAssetURLs:@[[info valueForKey:UIImagePickerControllerReferenceURL]] options:nil];
+    if (assets.count > 0) {
+      PHAsset *videoAsset = assets.firstObject;
+      width = @(videoAsset.pixelWidth);


micro-nit: height before width as that order is matched above in 148 for consistency.

mrkcsc

LGTM - should we attempt to upstream this?

Is there a reason why they would not have done this to begin with (eg: perf reason) or is this simply an omission.

Fanghao · 2019-07-10T19:03:45Z

Libraries/CameraRoll/RCTImagePickerManager.m

@@ -17,6 +17,8 @@
 #import <React/RCTRootView.h>
 #import <React/RCTUtils.h>

+@import Photos;


dont think we need this since we already #import <Photos/Photos.h> earlier?

PHFetchResult and PHAsset depends on this and if we don't have it the compiler can't find the definitions :\ idk tbh but a quick google said we had to do this

Fanghao · 2019-07-10T19:09:43Z

react native already started leaning their core by migrating lots deps into react-native-community including image picker.

@adgarcia attempted to migrate to react-native-community/react-native-image-picker during the rn upgrade but cannot fix the mini carousel problem from the new Photo frameworks instead of ALAssetsLibrary iirc? maybe she can add more inputs.

Fairly certain RN wont accept this change to the core repo any more. But we should try to upstream to community once we migrate to react-native-community/react-native-image-picker

Libraries/CameraRoll/RCTImagePickerManager.m

adgarcia · 2019-07-10T23:20:20Z

Does this deal w/ media picked from the carousel?

adgarcia · 2019-07-10T23:34:35Z

react native already started leaning their core by migrating lots deps into react-native-community including image picker.

@adgarcia attempted to migrate to react-native-community/react-native-image-picker during the rn upgrade but cannot fix the mini carousel problem from the new Photo frameworks instead of ALAssetsLibrary iirc? maybe she can add more inputs.

Fairly certain RN wont accept this change to the core repo any more. But we should try to upstream to community once we migrate to react-native-community/react-native-image-picker

fwiw, the latest react-native-community/camera-roll-manager returns the original filename, which (theoretically?) should work with what we have

Summary: This implements proposal #2 in our State architecture doc: https://fb.quip.com/bm2EAVwL7jQ5 Problem description: see the text in the comment of TreeStateReconciliation.h Solution: see also comments in TreeStateReconciliation.h. Changelog: [internal] Reviewed By: mdvacca Differential Revision: D19617329 fbshipit-source-id: 845fb5fe27f2591be433b6d77799707b3516fb1a

Summary: Previous iteraton of this diff that was reverted is D30678341 (facebook@8009459). With the power of selects, we can move the base AppleTVOS flags into the regular base Apple flags. While I'm here, drop the third `p` in `get_application_apppletvos_flags()` as it's driving me insane. Note - This puts get_visibility_option() on all Apple builds. I believe this is the right thing to do as everything except macOS static libraries already do it, and it shouldn't affect binaries. Changelog: [Internal] Reviewed By: aniketmathur Differential Revision: D30868627 fbshipit-source-id: 86441ff40db15dd7cb3ac800d248ce1e074c2773

…ake #2) Differential Revision: D30868627 (facebook@abd0f38) Original commit changeset: 86441ff40db1 fbshipit-source-id: eb040f8174f8f0f05943dcd5ae1e2077318810ff

Summary: In D30104853 (facebook@e35a963), we added fix to the issue where touches on the child view that is outside of its parent view's boundary are not registered. The only exception to that is if the parent view has overflow style `overflow: hidden`. In that case, touches happen outside of the parent view should be ignored. {F686521911} That fix works, but it increases the complexity for the DFS algorithm used to find the touch target in two ways: 1. Before we only traverse views that contain the touch event point. If the touch event point is outside of the view, we won't step in and traverse that part of the tree. This is actually what caused the initial problem. The fix removed that boundary check (where `isTransformedTouchPointInView` used to do) and push it later. This increases the number of tree traversal a lot. 2. The check for `overflow: hidden` is happened after we find a potential target view, which means we've spent time to find the target view before we decide if the target view is even a candidate. This diff aims to update for the #2 item above. Since we are checking the style of the parent view, not the target view, it's not necessary to check that after we find the target view. We could check the parent view and if it has `overflow: hidden` on it, any pointer event that are not in the boundary can be ignored already. Changelog: [Internal][Android] Reviewed By: mdvacca, ShikaSD Differential Revision: D33079157 fbshipit-source-id: c79c2b38b8affb9ea0fd25b5e880b22466ab7ed9

Summary: VirtualizedList would more gracefully handle out of range cells than VirtualizedList_EXPERIMENTAL, which treats it as an invariant violation. D39244112 (facebook@7aa203b) attempted to fix an issue where recalculation of cells around viewport can include out of range cells, but it is still showing up later. This change adds a bounds check to the remaining branch we control, and an assertion that `computeWindowedRenderLimits` is not returing something out of range to discover if that is the cause. Reviewed By: yungsters Differential Revision: D39267445 fbshipit-source-id: 64c99da28b5b01ef61784079b586e355f73764a1

Summary: Pull Request resolved: facebook#41466 ## Context In open source, all apps use the same turbomodulemanager delegate (i.e: the default delegate). This diff introduces the buck infra that makes the oss default delegate work for meta apps. Concretely, we are going to make React Native use the same delegate for **all** Meta apps. Each Meta app will: 1. At build time, generate a unique TMProvider map 2. At app init time, initialize the default delegate with the TMProvider map. ## Implementation **Step #1:** At build time, generate a unique TMProvider map **Insight:** Buck genrules can accept, as input, the output of a buck query. So, here's how we get this done: 1. Buck query (i.e: input to Genrule): Given the app's deps, query all the schemas in the app. 2. Genrule: Read the schemas to generate the TMProvider map. The TMProvider map will also contain **all** the app's C++ module codegen. Concretely: 1. This diff introduces a macro: rn_codegen_appmodules(deps). 2. rn_codegen_appmodules(deps) generates appmodules.so, which contains the TMProvider map. **Step #2:** At app init time, initialize the default delegate with the TMProvider map. This is how we'll initialize the DefaultTurboModuleManagerDelegate: 1. DefaultTurboModuleManagerDelegate will load appmodules.so during init. 2. When loaded, appmodules.so will assign the code-generated TMProvider map to DefaultTurboModuleManagerDelegate. ## Impact This should allow us to: 1. Get one step closer to getting rid of the `js1 build turbomodule-manager-delegates --target <app>` script 3. Remove the TurboModuleManagerDelegate from React Native's public API. (Because we use one delegate for all React Native apps in Meta and OSS) Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D50988397 fbshipit-source-id: 0ca5dec14e2dae89ec97f5d39a182c7937c5c7bf

…tion for existing view (facebook#51294) Summary: Pull Request resolved: facebook#51294 changelog: [internal] Fix a crash where a node that is supposed to be culled doesn't get visited because culling context is not updated. The differentiator would generate a create instruction for a view that already exists. Stack trace for the crash: ``` * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT * frame #0: 0x0000000111740874 libsystem_kernel.dylib`__pthread_kill + 8 frame #1: 0x00000001117aa2ec libsystem_pthread.dylib`pthread_kill + 264 frame #2: 0x0000000180171ea8 libsystem_c.dylib`abort + 100 frame #3: 0x00000001802b0144 libc++abi.dylib`abort_message + 128 frame #4: 0x000000018029fe4c libc++abi.dylib`demangling_terminate_handler() + 296 frame #5: 0x000000018006f220 libobjc.A.dylib`_objc_terminate() + 124 frame #6: 0x00000001375d1964 INFRAFramework`meta_terminate() + 5468 frame #7: 0x00000001802af570 libc++abi.dylib`std::__terminate(void (*)()) + 12 frame #8: 0x00000001802b2498 libc++abi.dylib`__cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) + 32 frame #9: 0x00000001802b2478 libc++abi.dylib`__cxa_throw + 88 frame #10: 0x0000000180093904 libobjc.A.dylib`objc_exception_throw + 384 frame #11: 0x0000000180e6999c Foundation`-[NSAssertionHandler handleFailureInFunction:file:lineNumber:description:] + 268 frame #12: 0x000000031a3bcfc8 XPLAT_6_Framework`-[RCTComponentViewRegistry dequeueComponentViewWithComponentHandle:tag:] + 528 frame #13: 0x000000031a3ccdec XPLAT_6_Framework`RCTPerformMountInstructions(std::__1::vector<facebook::react::ShadowViewMutation, std::__1::allocator<facebook::react::ShadowViewMutation>> const&, RCTComponentViewRegistry*, RCTMountingTransactionObserverCoordinator&, int) + 356 frame #14: 0x000000031a3ccc7c XPLAT_6_Framework`-[RCTMountingManager performTransaction:]::$_1::operator()(facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&) const + 80 frame #15: 0x000000031a3ccc20 XPLAT_6_Framework`decltype(std::declval<-[RCTMountingManager performTransaction:]::$_1&>()(std::declval<facebook::react::MountingTransaction const&>(), std::declval<facebook::react::SurfaceTelemetry const&>())) std::__1::__invoke[abi:ne190102]<-[RCTMountingManager performTransaction:]::$_1&, facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&>(-[RCTMountingManager performTransaction:]::$_1&, facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&) + 40 frame #16: 0x000000031a3ccbc8 XPLAT_6_Framework`void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ne190102]<-[RCTMountingManager performTransaction:]::$_1&, facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&>(-[RCTMountingManager performTransaction:]::$_1&, facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&) + 40 frame #17: 0x000000031a3ccb94 XPLAT_6_Framework`std::__1::__function::__alloc_func<-[RCTMountingManager performTransaction:]::$_1, std::__1::allocator<-[RCTMountingManager performTransaction:]::$_1>, void (facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&)>::operator()[abi:ne190102](facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&) + 44 frame #18: 0x000000031a3cba1c XPLAT_6_Framework`std::__1::__function::__func<-[RCTMountingManager performTransaction:]::$_1, std::__1::allocator<-[RCTMountingManager performTransaction:]::$_1>, void (facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&)>::operator()(facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&) + 44 frame #20: 0x000000032f219804 XPLAT_1_Framework`std::__1::function<void (facebook::react::MountingTransaction const&, facebook::react::SurfaceTelemetry const&)>::operator()(this=0x000000016d4f0c78, __arg=0x000000016d4f0a10, __arg=0x000000016d4f0978) const at function.h:989:10 frame #21: 0x000000032f219668 XPLAT_1_Framework`facebook::react::TelemetryController::pullTransaction(this=0x00000003f4680f00, willMount=0x000000016d4f0c98, doMount=0x000000016d4f0c78, didMount=0x000000016d4f0c58) const at TelemetryController.cpp:39:3 frame #22: 0x000000031a3c5b28 XPLAT_6_Framework`-[RCTMountingManager performTransaction:] + 544 frame #23: 0x000000031a3c5864 XPLAT_6_Framework`-[RCTMountingManager initiateTransaction:] + 456 frame #24: 0x000000031a3c5240 XPLAT_6_Framework`__42-[RCTMountingManager scheduleTransaction:]_block_invoke + 308 frame #25: 0x0000000131f81b84 BOTTOMFramework`__RCTExecuteOnMainQueue_block_invoke + 40 frame #26: 0x000000018017c788 libdispatch.dylib`_dispatch_call_block_and_release + 24 frame #27: 0x0000000180197278 libdispatch.dylib`_dispatch_client_callout + 12 frame #28: 0x00000001801b2fcc libdispatch.dylib`_dispatch_main_queue_drain.cold.7 + 24 frame #29: 0x000000018018c1c4 libdispatch.dylib`_dispatch_main_queue_drain + 1184 frame #30: 0x000000018018bd14 libdispatch.dylib`_dispatch_main_queue_callback_4CF + 40 frame #31: 0x0000000180427fec CoreFoundation`__CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12 frame #32: 0x00000001804229f8 CoreFoundation`__CFRunLoopRun + 1920 frame #33: 0x0000000180421e3c CoreFoundation`CFRunLoopRunSpecific + 536 frame #34: 0x0000000190f62d00 GraphicsServices`GSEventRunModal + 164 frame #35: 0x0000000185bcec98 UIKitCore`-[UIApplication _run] + 796 frame #36: 0x0000000185bd3064 UIKitCore`UIApplicationMain + 124 frame #37: 0x0000000115fbf0bc PRODUCTFramework`main + 200 frame #38: 0x00000001114293d8 dyld_sim`start_sim + 20 frame #39: 0x0000000111506b4c dyld`start + 6000 ``` Reviewed By: rubennorte Differential Revision: D74654157 fbshipit-source-id: 9181bcd28524c71d0ca4620bd630dc0baa172386

patch ImagePickerIOS to also give a width and height if it

8208ee5

detects movies

victorias requested review from adgarcia and Fanghao July 10, 2019 08:00

mrkcsc reviewed Jul 10, 2019

View reviewed changes

mrkcsc approved these changes Jul 10, 2019

View reviewed changes

Fanghao reviewed Jul 10, 2019

View reviewed changes

Fanghao approved these changes Jul 10, 2019

View reviewed changes

Libraries/CameraRoll/RCTImagePickerManager.m Outdated Show resolved Hide resolved

clean up

cc8e2e3

victorias changed the base branch from 0.60.0-discord to 0.60.1-discord July 11, 2019 05:12

victorias merged commit f06d12c into 0.60.1-discord Jul 11, 2019

victorias deleted the patch/video-widths branch July 11, 2019 05:14

victorias restored the patch/video-widths branch July 11, 2019 17:47

victorias deleted the patch/video-widths branch July 11, 2019 17:55

victorias mentioned this pull request Jul 11, 2019

patch imagepickerios video widths #3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

patch ImagePickerIOS to return video width and height #2

patch ImagePickerIOS to return video width and height #2

Uh oh!

victorias commented Jul 10, 2019

Uh oh!

mrkcsc Jul 10, 2019

Uh oh!

mrkcsc left a comment

Uh oh!

Fanghao Jul 10, 2019

Uh oh!

victorias Jul 10, 2019

Uh oh!

Fanghao commented Jul 10, 2019

Uh oh!

Uh oh!

adgarcia commented Jul 10, 2019

Uh oh!

adgarcia commented Jul 10, 2019

Uh oh!

Uh oh!

patch ImagePickerIOS to return video width and height #2

patch ImagePickerIOS to return video width and height #2

Uh oh!

Conversation

victorias commented Jul 10, 2019

Uh oh!

mrkcsc Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

mrkcsc left a comment

Choose a reason for hiding this comment

Uh oh!

Fanghao Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

victorias Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

Fanghao commented Jul 10, 2019

Uh oh!

Uh oh!

adgarcia commented Jul 10, 2019

Uh oh!

adgarcia commented Jul 10, 2019

Uh oh!

Uh oh!