-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Image_picker] fix rotation regression on Android. #2362
Conversation
|
@mklim Could you take a look at this one? Should be pretty straight forward. |
mklim
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.
The previous PR was fixing a crash though, wasn't it? Do we want to completely revert it to fix the rotation issue and then re-introduce the crash? I think it may be better to try to root cause the issue and forward fix it in this case, especially since this has been landed for awhile now.
We didn't completely revert it in this PR. We only revert the part that caused the rotation issue. Which also won't reintroduce the crash. This PR remove the copy file part if we don't have to do anything with the image (scaling, compression, etc). |
|
@cyanglaz how could we test this PR? There are no test_driver calls (I guess it's not feasible to pick image in a automated way) plus the example has no fields / configuration for As a first start, could you possibly add some sort of configuration UI to the example so that manual tests could be done? |
mklim
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.
LGTM pending testing discussion.
@ened there is a small change to the Robolectric ImageResizerTest.java that checks part of the behavior in this PR and meets our minimum testing requirement, but it's not really catching the main rotation regression like you've pointed out. I'm not sure of a good way to test this either. Flutter driver would be ideal if we could make that work, but I suspect getting a working test here may not be totally feasible. I think the best compromise would be to add something to ImageResizerTest that actually catches the rotation issue, but IIRC that's not really going to be possible either because Robolectric itself stubs out Bitmap with dummy data values that always return the same thing regardless of what "real" images you try to create with the code under test. @cyanglaz do you have any ideas for how to test the rotation regression specifically?
|
In my mind, the pragmatic way would be to implement a Android instrumentation (not unit) test, that copies a set of pre-defined assets onto a device. The test then runs a ImageResizer implementation against those and checks the output file (like a golden test, really). Secondly, the e2e test should have a way to assert the correct ImageResizer implementation is called. All in all quite complicated so perhaps easier to solve the problem first and add a way for developer-users to manually test the function. That is still a lot better than anything we have now. |
ened
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.
LGTM
mklim
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.
LGTM
| context: context, | ||
| builder: (context) { | ||
| return AlertDialog( | ||
| title: Text('TextField in Dialog'), |
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.
nit: This title isn't really descriptive. "Set image size" maybe?
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.
good find! Thanks. I updated it Add optional parameters
Description
A regression was introduced in #2293 where the image file seems to lose rotation when not scaled. Reverting to previous approach: directly return the original image file path solves the issue.
The root cause of why image file would lose rotation after compressing is unknown.
Related Issues
flutter/flutter#45905
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.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?