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

[image_picker] fix bug, sometimes double click cancel button will crash #2625

Merged
merged 1 commit into from
May 19, 2020
Merged

[image_picker] fix bug, sometimes double click cancel button will crash #2625

merged 1 commit into from
May 19, 2020

Conversation

iheron
Copy link
Contributor

@iheron iheron commented Mar 31, 2020 •
edited
Loading

Description

Sometimes double click cancel button will crash.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@iheron iheron requested a review from cyanglaz as a code owner March 31, 2020 14:24
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@iheron iheron force-pushed the master branch 3 times, most recently from 432d387 to 1b2d535 Compare March 31, 2020 14:49
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@iheron
Copy link
Contributor Author

iheron commented Mar 31, 2020

@googlebot I fixed it.

cyanglaz
cyanglaz reviewed Apr 3, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It looks good modulo some nits.
Could you also add some tests for this? I think it is possible to test with XCTests.

@iheron
Copy link
Contributor Author

iheron commented Apr 8, 2020

I don't know how to write test code to reproduce the bug. If you just open the page, multiple times and quickly click cancel button, can reproduce it.

@cyanglaz
Copy link
Contributor

cyanglaz commented Apr 8, 2020

@iheron We can have a XCTest to manually trigger
- (void)imagePickerControllerDidCancel:(UIImagePickerController *)picker
while self.result is nil.

@iheron iheron force-pushed the master branch 2 times, most recently from 37b5472 to bf84104 Compare April 11, 2020 06:34
@iheron
Copy link
Contributor Author

iheron commented Apr 11, 2020

@cyanglaz I find a file ImagePickerPluginTests.m, it's not in the xcode project, I added it and added a function testPluginPickImageDeviceCancel. But I am not good at writing objective-c and XCTests. Could you please help me to complete the test? 😂

@iheron iheron force-pushed the master branch 2 times, most recently from 289d602 to 14a2547 Compare April 11, 2020 06:51
@cyanglaz
Copy link
Contributor

@iheron I would love to, but I have other priority works to do. I will eventually circle back to this but it will take a long time. :)

@iheron
Copy link
Contributor Author

iheron commented Apr 14, 2020

Thanks.

@cyanglaz cyanglaz mentioned this pull request Apr 20, 2020
Merged
13 tasks
zhongwuzw
zhongwuzw reviewed Apr 21, 2020
View reviewed changes
iheron
iheron commented Apr 22, 2020
View reviewed changes
packages/image_picker/ios/Tests/ImagePickerPluginTests.m Outdated
plugin.result = ^(id result) {

};
[plugin imagePickerControllerDidCancel:[plugin getImagePickerController]];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zhongwuzw thanks for your help. I add testPluginPickImageDeviceCancelClickMultipleTimes, is this right?

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks good. If you try to run the test without your change, it crashes, then you are good :)

@iheron iheron force-pushed the master branch 2 times, most recently from 65a9059 to 4aa6f99 Compare May 12, 2020 10:25
@iheron
Copy link
Contributor Author

iheron commented May 19, 2020

Is there anything that needs to be modified now? @cyanglaz

cyanglaz
cyanglaz approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM!

@cyanglaz cyanglaz merged commit 25a585d into flutter:master May 19, 2020
KevinTheGray added a commit to KevinTheGray/plugins that referenced this pull request May 19, 2020
@KevinTheGray
Merge commit '25a585dfeef6d2dc224fd0dddee327372de63782' into kg/video…
bd35755 
…-player-plugin-fix

* commit '25a585dfeef6d2dc224fd0dddee327372de63782':
  [image_picker] iOS: fix bug, sometimes double click cancel button will crash (flutter#2625)
  [google_maps_flutter] Add liteModeEnabled option (flutter#2449)
  Update README.md (flutter#2768)
  [url_launcher_web] Launch mailto urls in same window in Safari (flutter#2740)
  update README with enableJavaScript info (flutter#2766)
  Run publish ci check on master (flutter#2764)
  [image_picker] Add documentation for Android external storage permissions (flutter#2765)
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
@iheron @EdwinRomelta
[image_picker] iOS: fix bug, sometimes double click cancel button wil…
b63f09b 
…l crash (flutter#2625)
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
@iheron @jorgefspereira
[image_picker] iOS: fix bug, sometimes double click cancel button wil…
93fbb2b 
…l crash (flutter#2625)
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@iheron
[image_picker] iOS: fix bug, sometimes double click cancel button wil…
807de4d 
…l crash (flutter#2625)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zhongwuzw zhongwuzw zhongwuzw left review comments

@cyanglaz cyanglaz cyanglaz approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iheron @googlebot @cyanglaz @zhongwuzw