-
Notifications
You must be signed in to change notification settings - Fork 9.8k
image_picker fixes: file suffix, permissions redirect, video resize #600
Conversation
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.
Thanks for the contribution and the attention to quality!
} | ||
|
||
private void handleChoosePictureResult(int resultCode, Intent data) { | ||
private void handleChooseMediaResult(int resultCode, Intent data, boolean isResizable) { |
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.
Please consider breaking this method and the ones below in two to avoid the boolean argument. E.g. handleChooseImageResult
and handleChooseVideoResult
. I'd rather suffer a few lines of repeated code between the image vs video cases than having multiple methods whose boolean argument reveal that each does two things.
It doesn't seem unlikely that the image vs video case might drift further apart in the future.
Hi @wreppun, thanks for contributing; have you had a chance to consider the review feedback? |
Hey, yes, thanks for the feedback. I don't mind breaking those up -- I should be able to look at this and turn something around on Tuesday. |
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
Published |
This PR is for three bugs relating to image picker. I found the latter two when fixing the first. I'm happy to split this up if that's easier.
.png
extension. I'm not 100% sure that.jpg
and.mp4
are correct -- I couldn't find an authoritative source -- but that's what most information seemed to point towards.