[camera] 28180 fix exif data bug for first time camera use android#1241
Conversation
Catch up to main line
…captured the first time the camera is used. flutter/flutter#28180
mklim
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
Thanks! I'll make the suggested changes and update the PR later today if all goes well. |
|
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 ℹ️ Googlers: Go here for more info. |
4da09ef to
181b173
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
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. |
…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

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.