Share extension Android and iOS implementation #54354
Conversation
…-fork into Guccio163/shareExtension
…-fork into Guccio163/shareExtension
…-fork into brtqkr/wire-up-share-extension-ios
…-fork into brtqkr/wire-up-share-extension-ios
…are-mansion-labs/expensify-app-fork into brtqkr/wire-up-share-extension-ios
| return ( | ||
| <MoneyRequestParticipantsSelector | ||
| iouType={CONST.IOU.TYPE.SUBMIT} | ||
| onFinish={() => {}} |
There was a problem hiding this comment.
MoneyRequestParticipantsSelector is a memoized component, and passing an anonymous function makes it rerender always, because its reference will change
| callback.invoke(e.toString(), null) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I noticed a lot of components with missing last line 😄
| @@ -0,0 +1,19 @@ | |||
| package com.expensify.chat.intentHandler | |||
There was a problem hiding this comment.
Minor thing but Java package names are typically all lowercase 😄
| package com.expensify.chat.intentHandler | |
| package com.expensify.chat.intenthandler |
| (intent.getParcelableExtra<Uri>(Intent.EXTRA_STREAM))?.let { fileUri -> | ||
| if (fileUri == null) { | ||
| return | ||
| } |
There was a problem hiding this comment.
I think this is redundant, since ?.let should make sure that fileUri exists
| val sharedPreferences = context.getSharedPreferences(IntentHandlerConstants.preferencesFile, Context.MODE_PRIVATE) | ||
| val editor = sharedPreferences.edit() | ||
| editor.putString(IntentHandlerConstants.shareObjectProperty, shareFileObject.toString()) | ||
| editor.apply() |
There was a problem hiding this comment.
I think this is duplicated in handleTextPlanIntent, can we extract it to a separate method?
| route: { | ||
| params: {reportID}, | ||
| }, |
There was a problem hiding this comment.
Is this common pattern across the App to deconstruct params like that?
| if (unknownUserDetails) { | ||
| optimisticReport.reportID = unknownUserDetails?.accountID?.toString() ?? ''; | ||
| displayReport.participantsList = [{...unknownUserDetails, displayName: unknownUserDetails.login, accountID: unknownUserDetails.accountID ?? CONST.DEFAULT_NUMBER_ID}]; | ||
| } | ||
|
|
||
| if (!onyxReport || onyxReport?.ownerAccountID === 0) { | ||
| const participants = onyxReport | ||
| ? ReportUtils.getDisplayNamesWithTooltips( | ||
| OptionsListUtils.getPersonalDetailsForAccountIDs( | ||
| Object.keys(onyxReport?.participants ?? {}) | ||
| .map((p) => parseInt(p ?? '', 10)) | ||
| .filter((u) => u !== currentUserID), | ||
| personalDetails, | ||
| ), | ||
| false, | ||
| ) | ||
| : displayReport.participantsList; | ||
| const participant = participants?.filter((u) => u.accountID !== currentUserID).at(0); | ||
| displayReport.text = participant?.displayName; | ||
| displayReport.alternateText = participant?.login; | ||
| } |
There was a problem hiding this comment.
Has it to be done on each render? It seems like manipulating data that will be displayed so it looks like asking for troubles. It's also interesting that react-compiler eslint rule isn't complaining about it.
| option.isDisabled = true; | ||
| option.selected = false; | ||
| option.isSelected = false; |
There was a problem hiding this comment.
This rather should be set in createOption
| width?: number; | ||
| }; | ||
|
|
||
| type FileObject = Partial<File | ImagePickerResponse>; |
There was a problem hiding this comment.
Is this type necessary? You only use file.name in this component and create object of that type maunaly
| const file = useMemo<FileObject | undefined>(() => { | ||
| const originalFileName: string = source.split('/').pop() ?? ''; | ||
| return originalFileName | ||
| ? { | ||
| name: originalFileName, | ||
| } | ||
| : undefined; | ||
| }, [source]); |
There was a problem hiding this comment.
I think you don't have to construct object with one property here. Something like this works better I think:
const fileName = source.split('/').pop() ?? '';
Even useMemo is not necessary because it's not heavy operation
| override fun toString(): String { | ||
| return Gson().toJson(this) |
There was a problem hiding this comment.
Do we need to add library for handling json for this line? Wouldn't it be easier just to do something like:
override fun toString(): String {
return "{\"content\": ${content}, \"mimeType\": ${mimeType}}"
| for (file in files) { | ||
| file.delete() | ||
| } |
There was a problem hiding this comment.
I think we should add a check if files exist, otherwise in case of some problems we may get a NullPointerException
| } | ||
|
|
||
| fun clearInternalStorageDirectory(context: Context) { | ||
| val internalStorageDirectory = File(context.filesDir.absolutePath, directoryName) |
There was a problem hiding this comment.
Maybe we can reuse the getInternalStorageDirectory here?
| * @return The absolute path of the image | ||
| */ | ||
| fun copyUriToStorage(uri: Uri?, context: Context): String? { | ||
| lateinit var resultingPath: String |
There was a problem hiding this comment.
Is lateinit necessary here? I think we could just return the correct value
|
|
||
| // Check if filename contains "text_to_read" | ||
| if ([fileName containsString:@"text_to_read"]) { | ||
| NSError *fileError = nil; |
There was a problem hiding this comment.
| NSError *fileError = nil; | |
| NSError *fileError; |
|
|
||
| NSTimeInterval timestampInterval = [[NSDate date] timeIntervalSince1970] * 1000; | ||
| NSString *timestamp = [NSString stringWithFormat:@"%.0f", timestampInterval]; | ||
| NSString *identifier = [NSString stringWithFormat:@"%@_%@", (unsigned long)timestamp, filePath]; |
There was a problem hiding this comment.
I think that unsigned long interpolation isn't needed here. i may be wrong though 😄
|
|
||
| RCT_EXPORT_MODULE(RCTShareActionHandlerModule); | ||
|
|
||
| RCT_EXPORT_METHOD(processFiles:(RCTResponseSenderBlock)callback) |
There was a problem hiding this comment.
I think this method is pretty long and has many responsibilities. Could we split it a bit for the readability?
| NSURL *sharedFilesFolderPathURL = [groupURL URLByAppendingPathComponent:ShareExtensionFilesKey]; | ||
| NSString *sharedFilesFolderPath = [sharedFilesFolderPathURL path]; | ||
|
|
||
| [defaults setObject:NULL forKey:ShareExtensionFilesKey]; |
There was a problem hiding this comment.
NULL in Objective-C is used rather to inform that there is no pointer. In this case we should use nil instead, or even better:
| [defaults setObject:NULL forKey:ShareExtensionFilesKey]; | |
| [defaults removeObjectForKey:ShareExtensionFilesKey]; |
| else if let url = data as? NSURL, let fileData = NSData(contentsOf: url as URL) { | ||
| guard let filename = url.lastPathComponent else { | ||
| completion(.CouldNotLoad) | ||
| return | ||
| } | ||
| if let fileFinalPath = self.saveFileToFolder(folder: folder, filename: filename, fileData: fileData) { | ||
| completion(nil) | ||
| } | ||
| else { | ||
| os_log("Skipping file %@, failed to save", type: .error, String(describing: filename) as CVarArg) // Safe typecasting | ||
| completion(.CouldNotLoad) | ||
| } | ||
| // Try to get file as UIFile. This is the case when extension is run from screenshot editor. | ||
| } | ||
| else if let file = data as? UIImage, let fileData = file.pngData() as NSData? { | ||
| let filename = "shared_image.png" | ||
| if let fileFinalPath = self.saveFileToFolder(folder: folder, filename: filename, fileData: fileData) { | ||
| completion(nil) | ||
| } | ||
| else { | ||
| os_log("Skipping file %@, failed to save", type: .error, filename as CVarArg) | ||
| completion(.CouldNotLoad) | ||
| } | ||
| } | ||
| else { | ||
| os_log("Received data of unhandled type", type: .error) | ||
| completion(.URLError) | ||
| } |
There was a problem hiding this comment.
I see some data repetition that could be avoided 😄
Details
This PR introduces the implementation of Share Extension for mobile apps both for Android and iOS.
Fixed Issues
$#48788
$#48789
PROPOSAL:
Tests
Share test:
Submit test:
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop