dpogue commented on code in PR #942:
URL:
https://github.com/apache/cordova-plugin-camera/pull/942#discussion_r2722377374
##########
src/ios/CDVCamera.m:
##########
@@ -362,20 +347,35 @@ - (void)picker:(PHPickerViewController*)picker
didFinishPicking:(NSArray<PHPicke
// Check if it's a video
if ([pickerResult.itemProvider
hasItemConformingToTypeIdentifier:UTTypeMovie.identifier]) {
- [pickerResult.itemProvider
loadFileRepresentationForTypeIdentifier:UTTypeMovie.identifier
completionHandler:^(NSURL * _Nullable url, NSError * _Nullable error) {
+ // Writes a copy of the data to a temporary file. This file will
be deleted
+ // when the completion handler returns. The program should copy or
move the file within the completion handler.
+ [pickerResult.itemProvider
loadFileRepresentationForTypeIdentifier:UTTypeMovie.identifier
+
completionHandler:^(NSURL * _Nullable url, NSError * _Nullable error) {
if (error) {
- CDVPluginResult* result = [CDVPluginResult
resultWithStatus:CDVCommandStatus_ERROR messageAsString:[error
localizedDescription]];
+ NSLog(@"CDVCamera: Failed to load video: %@", [error
localizedDescription]);
+ CDVPluginResult* result = [CDVPluginResult
resultWithStatus:CDVCommandStatus_IO_EXCEPTION
+
messageAsString:[NSString stringWithFormat:@"Failed to load video: %@", [error
localizedDescription]]];
[weakSelf.commandDelegate sendPluginResult:result
callbackId:callbackId];
weakSelf.hasPendingOperation = NO;
return;
}
- dispatch_async(dispatch_get_main_queue(), ^{
- NSString* videoPath = [weakSelf createTmpVideo:[url path]];
- CDVPluginResult* result = [CDVPluginResult
resultWithStatus:CDVCommandStatus_OK messageAsString:videoPath];
- [weakSelf.commandDelegate sendPluginResult:result
callbackId:callbackId];
- weakSelf.hasPendingOperation = NO;
- });
+ // The temporary file provided by PHPickerViewController is
deleted when the completion
+ // handler exits. The file has to be copied in this thread,
otherwise it will be gone.
+ NSString* videoPath = [weakSelf createTmpVideo:[url path]];
+
+ // Send Cordova plugin result back, which must be done on the
main thread
Review Comment:
I think this comment is still wrong. The part about "must be done on the
main thread" is not true, and we removed the code that was forcing it happened
on the main thread (because it's not necessary)
##########
src/ios/CDVCamera.m:
##########
@@ -394,46 +393,40 @@ - (void)picker:(PHPickerViewController*)picker
didFinishPicking:(NSArray<PHPicke
NSString *assetIdentifier = pickerResult.assetIdentifier;
dispatch_async(dispatch_get_main_queue(), ^{
- [weakSelf processPHPickerImage:image
assetIdentifier:assetIdentifier callbackId:callbackId options:pictureOptions];
+
+ // Fetch metadata if asset identifier is available
+ if (assetIdentifier) {
+ PHFetchResult *result = [PHAsset
fetchAssetsWithLocalIdentifiers:@[assetIdentifier] options:nil];
+ PHAsset *asset = result.firstObject;
+
+ if (asset) {
+ PHImageRequestOptions *imageOptions =
[[PHImageRequestOptions alloc] init];
+ imageOptions.synchronous = YES;
+ imageOptions.networkAccessAllowed = YES;
+
+ [[PHImageManager defaultManager]
requestImageDataAndOrientationForAsset:asset
+
options:imageOptions
+
resultHandler:^(NSData *_Nullable imageData, NSString *_Nullable
dataUTI, CGImagePropertyOrientation orientation, NSDictionary *_Nullable info) {
+ NSDictionary *metadata = imageData ? [weakSelf
convertImageMetadata:imageData] : nil;
+ dispatch_async(dispatch_get_main_queue(), ^{
+ [weakSelf finalizePHPickerImage:image
+ metadata:metadata
+ callbackId:callbackId
+
options:pictureOptions];
+ });
+ }];
+ return;
+ }
+ }
Review Comment:
Does all of this work need to happen on the main thread, or just the call to
`finalizePHPickerImage` below?
##########
src/ios/CDVCamera.m:
##########
@@ -865,21 +804,116 @@ - (void)resultForImage:(CDVPictureOptions*)options
- (CDVPluginResult*)resultForVideo:(NSDictionary*)info
{
NSString* moviePath = [[info objectForKey:UIImagePickerControllerMediaURL]
absoluteString];
+
// On iOS 13 the movie path becomes inaccessible, create and return a copy
if (IsAtLeastiOSVersion(@"13.0")) {
Review Comment:
We should replace this with a `if (@available(iOS 13.0, *))` check, rather
than `IsAtLeastiOSVersion` (which is deprecated)
##########
src/ios/CDVCamera.m:
##########
@@ -865,21 +804,116 @@ - (void)resultForImage:(CDVPictureOptions*)options
- (CDVPluginResult*)resultForVideo:(NSDictionary*)info
{
NSString* moviePath = [[info objectForKey:UIImagePickerControllerMediaURL]
absoluteString];
+
// On iOS 13 the movie path becomes inaccessible, create and return a copy
if (IsAtLeastiOSVersion(@"13.0")) {
- moviePath = [self createTmpVideo:[[info
objectForKey:UIImagePickerControllerMediaURL] path]];
+ moviePath = [self copyFileToTemp:[[info
objectForKey:UIImagePickerControllerMediaURL] path]];
}
+
return [CDVPluginResult resultWithStatus:CDVCommandStatus_OK
messageAsString:moviePath];
}
-- (NSString*)createTmpVideo:(NSString*)moviePath
+/**
+ Generates a unique temporary file path for a file extension.
+
+ The filename is prefixed with `cdv_photo_` and suffixed with the provided
+ file extension. A UNIX timestamp (seconds since 1970) is used to ensure
+ uniqueness between calls.
+
+ Threading: Safe to call from any thread. Uses NSTemporaryDirectory() and
+ does not perform any I/O; it only constructs a path string.
+
+ @param fileExtension The desired file extension without a leading dot
+ (for example, "jpg", "png", or the original video
+ extension like "mov").
+
+ @return An absolute path string within the app's temporary directory,
+ e.g.
`/var/mobile/Containers/Data/Application/<UUID>/tmp/cdv_photo_<timestamp>.jpg`.
+
+ @discussion The returned path is not created on disk. Callers are responsible
+ for writing data to the path and handling any errors.
+
+ @note Only files whose names start with `cdv_photo_` are cleaned up by the
+ plugin's `cleanup:` method.
+ **/
+- (NSString*)tempFilePathForExtension:(NSString*)fileExtension
+{
+ // Return a unique file name like
+ //
`/var/mobile/Containers/Data/Application/<UUID>/tmp/cdv_photo_<timestamp>.jpg`.
+ return [NSString stringWithFormat:
+ @"%@/%@%lld.%@",
+ [NSTemporaryDirectory() stringByStandardizingPath],
+ CDV_PHOTO_PREFIX,
+ (long long)[[NSDate date] timeIntervalSince1970],
+ fileExtension];
+}
+
+- (NSString*)copyFileToTemp:(NSString*)filePath
+{
+ NSFileManager* fileManager = [[NSFileManager alloc] init];
+ NSString* tempFilePath = [self tempFilePathForExtension:[filePath
pathExtension]];
+ NSError *error = nil;
+
+ // Copy file to temp directory
+ BOOL copySuccess = [fileManager copyItemAtPath:filePath
toPath:tempFilePath error:&error];
+
+ if (!copySuccess || error) {
+ NSLog(@"CDVCamera: Failed to copy file from %@ to temporary path %@.
Error: %@", filePath, tempFilePath, [error localizedDescription]);
+ return nil;
+ }
+
+ // Verify the copied file exists
+ if (![fileManager fileExistsAtPath:tempFilePath]) {
+ NSLog(@"CDVCamera: Copied file does not exist at temporary path: %@",
tempFilePath);
+ return nil;
+ }
+
+ return [[NSURL fileURLWithPath:tempFilePath] absoluteString];
+}
+
+/**
+ Called by JS camera.cleanup()
+ Removes intermediate image files that are kept in temporary storage after
+ calling camera.getPicture.
+*/
+- (void)cleanup:(CDVInvokedUrlCommand*)command
{
- NSString* moviePathExtension = [moviePath pathExtension];
- NSString* copyMoviePath = [self tempFilePath:moviePathExtension];
+ // empty the tmp directory
NSFileManager* fileMgr = [[NSFileManager alloc] init];
- NSError *error;
- [fileMgr copyItemAtPath:moviePath toPath:copyMoviePath error:&error];
- return [[NSURL fileURLWithPath:copyMoviePath] absoluteString];
+ NSError* err = nil;
+ BOOL hasErrors = NO;
+
+ // clear contents of NSTemporaryDirectory
Review Comment:
Would be better if this comment mentioned that it only clears contents that
match the prefix
##########
src/ios/CDVCamera.m:
##########
@@ -865,21 +804,116 @@ - (void)resultForImage:(CDVPictureOptions*)options
- (CDVPluginResult*)resultForVideo:(NSDictionary*)info
{
NSString* moviePath = [[info objectForKey:UIImagePickerControllerMediaURL]
absoluteString];
+
// On iOS 13 the movie path becomes inaccessible, create and return a copy
if (IsAtLeastiOSVersion(@"13.0")) {
- moviePath = [self createTmpVideo:[[info
objectForKey:UIImagePickerControllerMediaURL] path]];
+ moviePath = [self copyFileToTemp:[[info
objectForKey:UIImagePickerControllerMediaURL] path]];
}
+
return [CDVPluginResult resultWithStatus:CDVCommandStatus_OK
messageAsString:moviePath];
}
-- (NSString*)createTmpVideo:(NSString*)moviePath
+/**
+ Generates a unique temporary file path for a file extension.
+
+ The filename is prefixed with `cdv_photo_` and suffixed with the provided
+ file extension. A UNIX timestamp (seconds since 1970) is used to ensure
Review Comment:
Is seconds granular enough to ensure uniqueness? Not sure if we support
selecting multiple files, but there's a chance those could generate filenames
that collide?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
