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

[camera] 28180 fix exif data bug for first time camera use android#1241

Merged
mklim merged 4 commits intoflutter:masterfrom
idealopamp:28180_exif_data_bug_for_first_time_camera_use_android
Feb 22, 2019
Merged

[camera] 28180 fix exif data bug for first time camera use android#1241
mklim merged 4 commits intoflutter:masterfrom
idealopamp:28180_exif_data_bug_for_first_time_camera_use_android

Conversation

@idealopamp
Copy link
Contributor

@idealopamp idealopamp commented Feb 20, 2019

This is a potential fix for flutter/flutter#28180 . I would say the code is a bit more complex than I would like it to be, but I think the diff at least captures the essence of what is going wrong -- the orientationEventListener needs to be re-enabled onActivityResumed, even if it is resumed after requesting permissions (but camera.open should still not be called in this case -- otherwise for some reason the camera never successfully activates).

I took a peak at the commits for this function, and it seems like @mklim probably knows the most about the need for orientationEventLIstener.enabled to be called, while @kmorkos might know a bit more about the need to not call camera.open if the activity is being resumed because it was requestingPermission, so hoping you guys might be able to scope this out and see if the change looks okay.

Thanks!

P.S. If you do want to merge this as is, I would suggest doing a squash-merge to make one commit, since I have a couple extra commits -- one from a snafoo with running the standard code formatting and one from merging into my fork of this repo.

@idealopamp idealopamp changed the title 28180 exif data bug for first time camera use android 28180 fix exif data bug for first time camera use android Feb 20, 2019
Copy link
Contributor

@mklim mklim 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 your investigating this and submitting a fix! I have a couple minor suggestions but this approach makes sense to me. If you'd rather not make further changes please let us know and we can shepherd the PR forward on our end instead.

All our PRs get squashed on merge, so feel free to make as many commits in the branch as you need.

if (activity == CameraPlugin.this.activity) {
orientationEventListener.enable();

if (wasRequestingPermission) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit, please add braces around single return statements: https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used

if (wasRequestingPermission) {
  return;
}

requestingPermission = false;
return;
}
if (activity == CameraPlugin.this.activity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a subjective style suggestion, feel free to disregard it. I think at this point since we've got a few levels of nesting this block would be a little more readable if we used guard clauses/early returns to bail out of here when we stopped meeting our conditions for acting instead of putting the actions inside nested if blocks.

Like:

if (activity != CameraPlugin.this.activity) {
  return;
}
orientationEventListener.enable();
if (wasRequestingPermission || camera == null) {
  return;
}
camera.open(null);

https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

Like I said though this is a subjective aesthetics thing so feel free to ignore if you'd rather not.

@idealopamp
Copy link
Contributor Author

Thanks for your investigating this and submitting a fix! I have a couple minor suggestions but this approach makes sense to me. If you'd rather not make further changes please let us know and we can shepherd the PR forward on our end instead.

All our PRs get squashed on merge, so feel free to make as many commits in the branch as you need.

Thanks! I'll make the suggested changes and update the PR later today if all goes well.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@idealopamp idealopamp force-pushed the 28180_exif_data_bug_for_first_time_camera_use_android branch from 4da09ef to 181b173 Compare February 22, 2019 00:11
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@idealopamp
Copy link
Contributor Author

Hey sorry to the people who were accidentally pulled into this as reviewers -- seems that something in the way I caught up my fork of this repo caused git to treat some unrelated commits on flutter/plugins as new commits for this PR. I have since force pushed a clean branch, and it seems that googlebot is happy with the current state of affairs. I don't seem to have permission to remove the extra folks from the list of reviewers though.

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks again!

@mklim mklim added the submit queue The Flutter team is in the process of landing this PR. label Feb 22, 2019
@mklim mklim self-assigned this Feb 22, 2019
@bparrishMines bparrishMines changed the title 28180 fix exif data bug for first time camera use android [camera] 28180 fix exif data bug for first time camera use android Feb 22, 2019
@mklim mklim merged commit 443b58f into flutter:master Feb 22, 2019
Akachu pushed a commit to Akachu/flutter_camera that referenced this pull request Apr 27, 2020
…lutter#1241)

Fix an issue in android camera plugin where orientation data was not captured
the first time the camera is used.

Fixes flutter/flutter#28180
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes submit queue The Flutter team is in the process of landing this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants