-
Notifications
You must be signed in to change notification settings - Fork 128
Image Resizing #3162
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
base: master
Are you sure you want to change the base?
Image Resizing #3162
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3162 +/- ##
============================================
- Coverage 69.05% 68.79% -0.27%
Complexity 1378 1378
============================================
Files 287 287
Lines 7146 7174 +28
Branches 719 724 +5
============================================
Hits 4935 4935
- Misses 1822 1850 +28
Partials 389 389
🚀 New features to boost your workflow:
|
app/src/main/java/org/groundplatform/android/ui/common/BindingAdapters.kt
Show resolved
Hide resolved
app/src/main/java/org/groundplatform/android/ui/common/BindingAdapters.kt
Show resolved
Hide resolved
.transform(RotateUsingExif(uri, view.context)) | ||
.into(view) | ||
} catch (exception: OutOfMemoryError) { |
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.
We don't need a custom message here.
override fun transform(source: Bitmap): Bitmap { | ||
val orientation = getOrientationFromExif(uri) | ||
val rotateDegrees = getRotationDegrees(orientation) | ||
return rotateBitmap(source, rotateDegrees) | ||
|
||
val resizedBitmap = resizeBitmapIfNeeded(source, maxWidth, maxHeight) |
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.
This single responsibility (see https://en.wikipedia.org/wiki/Single-responsibility_principle) of this transformation is to rotate the image. Having it also resize would be surprising. Instead, create a separate ResizeTransformation and chain the two to achieve the desired result.
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 update the title of the PR to describe the user visible effect merging this change will cause. e.g. "Resize photos when showing inline thumbnail to OOM"
Fixes #3161
@shobhitagarwal1612 PTAL?