-
Notifications
You must be signed in to change notification settings - Fork 620
feat: orientation lock #725
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
Conversation
568da25 to
e5afd81
Compare
scarlac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but the code needs to be reviewed more thoroughly. If you used AI to write this, please make sure you fully understand and remove extraneous code, dead code, and you don't accidentally modify existing code. In my experience, all the AIs are not good at retaining existing functionality when asking for modifications.
bfa0047 to
e918091
Compare
|
You're right, I've used some AI assistance while coding this, and it resulted in some unnecessary code. I've cut down the PR to the essentials. I've tested it, and it seems to be working with this bare minimum approach. The only thing I've noticed while testing is that the camera detects even the slightest tilts on Android as an orientation change. Not sure if that's the best approach, but that's a topic for another day. |
af8881e to
6e5c480
Compare
6e5c480 to
47284db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 modes fail to capture orientation nuance. If this mode is to be supported it needs to be supported for all 4 orientations + auto, not just 2 + auto.
I also still see misuse of interface orientation. Part of reason this library still exists, is because I recognized that using UI orientation is incorrect. You must use hardware orientation as a base, otherwise you are not capturing the correct orientation.
I'm sorry to say, but based on this, there is still an excessive amount of code for the functionality, and some of it still relies on decisions that break current APIs.
I don't want to reject it purely on the AI use, but I suspect you'd have done better yourself, and it would have been more consistent and less back and forward between us.
Instead, I find that I don't really trust this PR at all at this point. It puts a lot of pressure on the maintainer to catch AI mistakes, obvious and non-obvious.
Just because it runs doesn't mean it'll make for a good contribution.
I am rejecting this PR, and not doing further reviews of it, but hopefully you'll take this as the heartfelt feedback it is: I recommend that you focus on your own code intent first, based on available APIs. Formulate a plan yourself, look at the APIs yourself, know what code you want to appear yourself, write a lot of it yourself, but use AI as a fancy auto complete. It's still immensely helpful. It'll make better code for this particular library.
Summary
In one of our apps, we encountered an issue with a screen that was limited to landscape orientation, providing landscape-specific photography capabilities.
While the screen orientation was staying in landscape mode, pictures were coming out as portraits. This was especially frustrating for Android users, who were trying to take landscape pictures of something on their desk from the top, but the Android device was detecting it as a portrait, while the photos were coming out as landscapes. It also seems like the phone is detecting one orientation, and the camera is using a different one, but that's another story.
This code adds the ability to specify the camera orientation you would like to use:
I've mainly used the existing codebase to implement this feature.
How did you test this change?
I thoroughly tested these changes on these devices:
Tested taking photos in each orientation mode while the device was rotated to different positions and verified that existing camera functionality remains unchanged.
Simulator support is not implemented, as it seems the simulator is simulating image parameters from the screen, which is a bit harder to trick. Not sure if that is needed, but I'm willing to take a deeper look into that if needed.
This should not introduce any breaking changes.