Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation property attributes - #6890

Merged
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
jmagman:image-picker-quality
Jan 3, 2023
Merged

[image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation property attributes#6890
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
jmagman:image-picker-quality

Conversation

@jmagman

@jmagman jmagman commented Dec 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Fix memory attributes of FLTPHPickerSaveImageToPathOperation NSNumber properties to strong, not assign
  • Add UI tests that exercise the quality setting, confirm it crashes without this fix.
  • Use XCUIElementQuery subscripting to find elements instead of label predicates.
  • Minor cleanup of NSNumber boxing.
  • Remove < iOS 14 dead code in one of the UI tests.

Fixes flutter/flutter#117670

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@jmagman
jmagman requested a review from cyanglaz as a code owner December 28, 2022 20:52
@jmagman jmagman self-assigned this Dec 28, 2022
jmagman
jmagman commented Dec 28, 2022
View reviewed changes
Comment thread ..._picker/image_picker_ios/example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
BuildableIdentifier = "primary"
BlueprintIdentifier = "BE7AEE6B26403C46006181AA"
BuildableName = "RunnerUITestiOS14.xctest"
BlueprintName = "RunnerUITestiOS14"

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was removed at some point.

Comment thread .../image_picker/image_picker_ios/example/ios/RunnerTests/PickerSaveImageToPathOperationTests.m
* Creates a mock picker result using NSItemProvider.
*
* @param itemProvider an item provider that will be used as picker result
* @param identifier local identifier of the asset

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
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 such parameter.

Comment thread ...ages/image_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m

XCUIElement *imageFromGalleryButton =
[self.app.otherElements elementMatchingPredicate:predicateToFindImageFromGalleryButton];
self.app.otherElements[@"image_picker_example_from_gallery"].firstMatch;

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Prefer subscripting over predicates for easier reading.

Comment thread ...ages/image_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m
@(kElementWaitingTime));
}

XCTAssertTrue(imageFromGalleryButton.exists);

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't need to check that it exists, waitForExistenceWithTimeout right above does this already.

Comment thread ...ages/image_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m
[self launchPickerAndCancel];
}

- (void)launchPickerAndCancel {

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Remove unnecessary utility method.

Comment thread ...ages/image_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m
}

XCUIElement *pickButton = [self.app.buttons elementMatchingPredicate:predicateToFindPickButton];
if (quality != nil) {

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This tests the fix, and crashes on master without it.

Comment thread ...age_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromLimitedGalleryUITests.m

- (void)testSelectingFromGallery {
// Test the `Select Photos` button which is available after iOS 14.
if (@available(iOS 14, *)) {

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use API_AVAILABLE in the method signature instead.

Comment thread ...age_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromLimitedGalleryUITests.m
XCUIElement *aImage;
if (@available(iOS 14, *)) {
aImage = [self.app.scrollViews.firstMatch.images elementBoundByIndex:1];
} else {

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This else was dead code, this only runs on iOS 14.

Comment thread ...age_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromLimitedGalleryUITests.m
// Find an image and tap on it to have access to selected photos.
if (@available(iOS 14, *)) {
aImage = [self.app.scrollViews.firstMatch.images elementBoundByIndex:1];
} else {

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same, dead code.

Comment thread packages/image_picker/image_picker_ios/ios/Classes/FLTPHPickerSaveImageToPathOperation.m
Comment on lines +16 to +18
@property(strong, nonatomic) NSNumber *maxHeight;
@property(strong, nonatomic) NSNumber *maxWidth;
@property(strong, nonatomic) NSNumber *desiredImageQuality;

@jmagman jmagman Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix.

vashworth
vashworth approved these changes Dec 28, 2022
View reviewed changes

@vashworth vashworth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

stuartmorgan-g
stuartmorgan-g approved these changes Jan 2, 2023
View reviewed changes

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 3, 2023
@auto-submit
auto-submit Bot merged commit f9dda6a into flutter-team-archive:main Jan 3, 2023
@jmagman
jmagman deleted the image-picker-quality branch January 3, 2023 21:03
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 4, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 4, 2023
@engine-flutter-autoroll
f9dda6a27 [image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation …
432e769 
…property attributes (flutter-team-archive/plugins#6890)
auto-submit Bot pushed a commit to flutter/flutter that referenced this pull request Jan 4, 2023
@engine-flutter-autoroll
Roll Plugins from e85f8ac1502d to f9dda6a27b79 (3 revisions) (#117972)
231855f 
* 6df3ef23f [in_app_pur] Add screenshots to pubspec.yaml (flutter-team-archive/plugins#6540)

* 42f8093c2 [google_maps_flutter] Fixed minor syntax error in the README.md (flutter-team-archive/plugins#6909)

* f9dda6a27 [image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation property attributes (flutter-team-archive/plugins#6890)
loic-sharma pushed a commit to fluttergithubbot/flutter that referenced this pull request Jan 6, 2023
@engine-flutter-autoroll @loic-sharma
Roll Plugins from e85f8ac1502d to f9dda6a27b79 (3 revisions) (flutter…
a6b2795 
…#117972)

* 6df3ef23f [in_app_pur] Add screenshots to pubspec.yaml (flutter-team-archive/plugins#6540)

* 42f8093c2 [google_maps_flutter] Fixed minor syntax error in the README.md (flutter-team-archive/plugins#6909)

* f9dda6a27 [image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation property attributes (flutter-team-archive/plugins#6890)
esouthren pushed a commit to esouthren/flutter that referenced this pull request Jan 15, 2023
@engine-flutter-autoroll @esouthren
Roll Plugins from e85f8ac1502d to f9dda6a27b79 (3 revisions) (flutter…
cd9f3fc 
…#117972)

* 6df3ef23f [in_app_pur] Add screenshots to pubspec.yaml (flutter-team-archive/plugins#6540)

* 42f8093c2 [google_maps_flutter] Fixed minor syntax error in the README.md (flutter-team-archive/plugins#6909)

* f9dda6a27 [image_picker_ios] Fix FLTPHPickerSaveImageToPathOperation property attributes (flutter-team-archive/plugins#6890)
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jan 16, 2023
@esouthren @guidezpl
@engine-flutter-autoroll @Jasguerrero @darrenaustin @asashour @goderbauer @gnprice @CicadaCinema @thkim1011 @Renzo-Olivares Renzo Olivares @srawlins @peixinli @moffatman @mraleph @christopherfujino @fluttergithubbot @camsim99 @LongCatIsLooong @drewroengoogle @jason-simmons @nehalvpatel @gmackall @mackall-work @M97Chahboun @a-wallen a-wallen @morriskurz @LucasXu0 @jmagman @helin24 @TahaTesser @nshahan @yjbanov @bleroux @bdero @loic-sharma Jonah Williams @Time1ess @sigurdm @rydmike @dependabot @isaacscha @andrewkolos @QuncCccccc @joshualitt @awesometic @parlough @IvoB1987 @godofredoc @bernaferrari @IchordeDionysos @HansMuller @vashworth
M3 Button padding adjustments (#118449