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

feat(ios): allow multiple photo selection #11867

Merged
merged 18 commits into from Sep 14, 2020

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 9.2.0 milestone Aug 5, 2020
@build build requested a review from a team August 5, 2020 01:48
@build
Copy link
Contributor

build commented Aug 5, 2020

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 5083 tests are passing.
(There are 472 skipped tests not included in that total)

Generated by 🚫 dangerJS against 7fb1bde


NSArray *types = (NSArray *)[args objectForKey:@"mediaTypes"];
#if IS_SDK_IOS_14
if ([TiUtils isIOSVersionOrGreater:@"14.0"] && ([args objectForKey:@"selectionLimit"] || [TiUtils boolValue:[args objectForKey:@"allowMultiple"] def:NO])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe always use it on iOS 14? The old image picker is now deprecated and should be replaced for iOS 14+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In new PHPickerViewController, there is no option for image editing(may be included in future releases). I think this is important feature and we can not remove it in minor release.

DebugLog(@"[ERROR] Failed to load video- %@ .", error.description);
}
dispatch_group_leave(group);
}];
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seem to be a lot of duplicate code logic here. Maybe move to own handler class / methods to clean up the business logic around this.

if (autoHidePicker) {
[self closeModalPicker:picker];
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are on a background thread here. Maybe validate if closeModalPicker and sendPickerSuccess really are performed on the main thread. In any case, there doesn't seem to be much processing here, so is a global queue / background queue necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no special handling in this delegate method for closeModalPicker and sendPickerSuccess. It is same as with old picker. In dispatch_group_notify queue is mandatory to pass.

@sgtcoolguy
Copy link
Contributor

iOS formatting is failing lint. Run 'npm run format:ios'

@build build added the docs label Aug 31, 2020
@vijaysingh-axway
Copy link
Contributor Author

@jquick-axway Can you verify doc for android multiple selection callback?

apidoc/Titanium/Media/Media.yml Show resolved Hide resolved
iphone/Classes/MediaModule.m Outdated Show resolved Hide resolved
apidoc/Titanium/Media/Media.yml Outdated Show resolved Hide resolved
@@ -1109,7 +1109,7 @@ - (void)openPhotoGallery:(id)args

NSArray *types = (NSArray *)[args objectForKey:@"mediaTypes"];
#if IS_SDK_IOS_14
if ([TiUtils isIOSVersionOrGreater:@"14.0"] && ([args objectForKey:@"selectionLimit"] || [TiUtils boolValue:[args objectForKey:@"allowMultiple"] def:NO])) {
if ([TiUtils isIOSVersionOrGreater:@"14.0"] && [TiUtils boolValue:[args objectForKey:@"allowMultiple"] def:NO]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why not to use the PHPicker by default on iOS 14 all the time? I'd love to use it from now on - the UX is amazing, also for single selections!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. In new PHPickerViewController, there is no option for image editing. I think this is important feature and we can not remove it in minor release. (I'll create a ticket to deprecate editing property, in case it is not supported by Apple in future releases).
  2. In PHPickerViewController, video file need to copy locally as per discussion https://developer.apple.com/forums/thread/652695. I think this is not the best way. May be its handling can be changed by Apple.
    So I have limited this API to multiple selection only. It will be updated in future release.

In case you want to use the API with single selection, set Ti.Media.allowMultiple = true and Ti.Media.selectionLimit = 1. But yes, a minor change required in your callback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh, thats sad! Didn't know they missed parity when updating the API. Alrighty, thanks for the clarification Vijay! 🙂

@ssekhri
Copy link

ssekhri commented Sep 11, 2020

FR Passed
On iOS 14 - Able to select multiple photo, live photo or video or a combination of these media types upto the defined selection limit.
On iOS 13.x - The selection of single item of a media type works fine as before.

Verified on:
Mac OS: 10.15.4
SDK: 9.2.0.v20200910075418
Appc CLI: 8.1.0
JDK: 11.0.4
Node: 10.17.0
Studio: 6.0.0.202005141803
Xcode: 12.0 Beta6
iPhone 7Plus(v14.0 Beta6), iPhone X(v13.6.1)

@sgtcoolguy sgtcoolguy merged commit 04b4292 into tidev:master Sep 14, 2020
@build
Copy link
Contributor

build commented Sep 14, 2020

The backport to 9_3_X failed:

The process 'git' failed with exit code 128

Check the run for full details
To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Check out the target branch
git checkout 9_3_X
# Make sure it's up to date
git pull
# Check out your branch
git checkout -b backport-11867-to-9_3_X
# Apply the commits from the PR
curl -s https://github.com/appcelerator/titanium_mobile/commit/2f2163e2f911ffa1e219892003d69fc5de6db62d.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/a95c6c9656a99300ac715d479e50094dbdf6439d.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/b70ee42d7e581b81cec9186fd179490999f0b182.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/d3cbcce165298314329eedd509ce83ff0e058860.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/42ffd028edfc10a875e103a77b5c2ef4b7f3ca00.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/062084ac6ac12b2115043d6ed1a2bb79200f11ee.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/3eff002f514d0b832acca09fdf4cc72229144ac3.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/8be6c6a401589f7792cd5537cdbd62af70a85eb4.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/7758f1290e5ddf96e6c54e286c8ac020c8565a71.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/4a26c49a68a150d69b6a8f6ec7a6c75054545ba8.patch | git am -3 --ignore-whitespace
# Push it to GitHub
git push --set-upstream origin backport-11867-to-9_3_X

Then, create a pull request where the base branch is 9_3_X and the compare/head branch is backport-11867-to-9_3_X.

sgtcoolguy pushed a commit that referenced this pull request Sep 14, 2020
* add workaround code for video loading
* updated logic for selectionLimit to work with allowMultiple true only

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

Successfully merging this pull request may close these issues.

None yet

6 participants