-
Notifications
You must be signed in to change notification settings - Fork 0
Update Image Upload Flow #682
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
…r selection and update layout
…ng library, Remove unused viewmodel functions, variables
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.
Pull Request Overview
This PR implements a complete overhaul of the image upload flow by replacing third-party image cropping with a custom in-app solution, adding large image handling capabilities, and fixing image orientation issues through EXIF data processing.
Key changes include:
- Custom Compose-based image cropping functionality replacing third-party library
- Large image OOM prevention through optimized bitmap loading and sampling
- EXIF-based automatic image orientation correction
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WriteViewModel.kt | Refactored image handling to use ImageAsset data class, added async bitmap loading with OOM prevention, implemented custom cropping logic |
| WriteScreen.kt | Updated UI to use AsyncImage with cache keys, added image edit functionality, removed third-party cropper integration |
| WriteNavigation.kt | Added navigation route for custom crop screen |
| ImageCropStateHolder.kt | New state management class for crop operations with gesture handling |
| ImageCropScreen.kt | New custom image cropping UI with pinch-zoom and corner drag gestures |
| ImageSamplingUtils.kt | New utility for optimized bitmap loading with inSampleSize calculation |
| ImageOrientationUtil.kt | New EXIF data processing utilities for image orientation correction |
| build.gradle | Removed third-party image cropper dependency |
| AndroidManifest.xml | Removed third-party cropper activity declaration |
| strings.xml | Added new string resource for image edit functionality |
| ic_crop.xml | Added new crop icon drawable |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| val resizedBitmap = resizeBitmap( | ||
| bitmap.cropCenterBitmap(), | ||
| POST_IMAGE_RESIZE_SIZE, | ||
| POST_IMAGE_RESIZE_SIZE | ||
| ) | ||
| val file = resizedBitmap.toFile(uploadImagePath) | ||
| bitmap.recycle() | ||
| resizedBitmap.recycle() | ||
| file |
Copilot
AI
Aug 29, 2025
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 code doesn't handle the case where BitmapFactory.decodeStream() returns null, which could cause a NullPointerException when calling bitmap.cropCenterBitmap() on line 151. Add a null check for the bitmap before processing.
| val resizedBitmap = resizeBitmap( | |
| bitmap.cropCenterBitmap(), | |
| POST_IMAGE_RESIZE_SIZE, | |
| POST_IMAGE_RESIZE_SIZE | |
| ) | |
| val file = resizedBitmap.toFile(uploadImagePath) | |
| bitmap.recycle() | |
| resizedBitmap.recycle() | |
| file | |
| if (bitmap == null) { | |
| null | |
| } else { | |
| val resizedBitmap = resizeBitmap( | |
| bitmap.cropCenterBitmap(), | |
| POST_IMAGE_RESIZE_SIZE, | |
| POST_IMAGE_RESIZE_SIZE | |
| ) | |
| val file = resizedBitmap.toFile(uploadImagePath) | |
| bitmap.recycle() | |
| resizedBitmap.recycle() | |
| file | |
| } |
presentation/src/main/java/daily/dayo/presentation/screen/write/ImageCropScreen.kt
Outdated
Show resolved
Hide resolved
| fun ContentResolver.readExifInfo(uri: Uri): ExifInfo { | ||
| openFileDescriptor(uri, "r")?.use { pfd -> | ||
| val exif = ExifInterface(pfd.fileDescriptor) | ||
| val o = exif.getAttributeInt(ExifInterface.TAG_ORIENTATION, ExifInterface.ORIENTATION_NORMAL) | ||
| return when (o) { |
Copilot
AI
Aug 29, 2025
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.
If openFileDescriptor() returns null, the function will fall through to return the default ExifInfo at line 31, but this behavior should be more explicit. Consider handling the null case explicitly or documenting this fallback behavior.
presentation/src/main/java/daily/dayo/presentation/screen/write/WriteScreen.kt
Show resolved
Hide resolved
| uris.map { uri -> | ||
| val exifInfo = applicationContext.contentResolver.readExifInfo(uri) | ||
| ImageAsset(uriString = uri.toString(), exifInfo = exifInfo) | ||
| } |
Copilot
AI
Aug 29, 2025
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.
[nitpick] Reading EXIF information is an I/O operation that should be performed on a background thread. Since this is already wrapped in withContext(Dispatchers.IO), the current implementation is correct, but consider adding error handling for potential I/O exceptions when reading EXIF data.
| private val _cropProperties = mutableStateOf(CropProperties()) | ||
| val cropProperties: State<CropProperties> = _cropProperties | ||
|
|
||
| private val imageAspectRatio = originalBitmap.height.toFloat() / originalBitmap.width | ||
| val imageWidth = containerSize.width.toFloat() | ||
| val imageHeight = imageWidth * imageAspectRatio | ||
| val imageTop = (containerSize.height - imageHeight) / 2f | ||
| private val maxCropSize = min(imageWidth, imageHeight) | ||
|
|
||
| init { | ||
| val initialSize = min(imageWidth, imageHeight) * 0.8f | ||
| _cropProperties.value = CropProperties( | ||
| cropSize = initialSize, | ||
| cropOffset = Offset( | ||
| x = (imageWidth - initialSize) / 2f, | ||
| y = imageTop + (imageHeight - initialSize) / 2f | ||
| ) | ||
| ) | ||
| } |
Copilot
AI
Aug 29, 2025
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.
[nitpick] The _cropProperties is initialized with an empty CropProperties() but then immediately overwritten in the init block. Consider initializing it directly with the computed initial values to avoid the unnecessary intermediate state.
| private val _cropProperties = mutableStateOf(CropProperties()) | |
| val cropProperties: State<CropProperties> = _cropProperties | |
| private val imageAspectRatio = originalBitmap.height.toFloat() / originalBitmap.width | |
| val imageWidth = containerSize.width.toFloat() | |
| val imageHeight = imageWidth * imageAspectRatio | |
| val imageTop = (containerSize.height - imageHeight) / 2f | |
| private val maxCropSize = min(imageWidth, imageHeight) | |
| init { | |
| val initialSize = min(imageWidth, imageHeight) * 0.8f | |
| _cropProperties.value = CropProperties( | |
| cropSize = initialSize, | |
| cropOffset = Offset( | |
| x = (imageWidth - initialSize) / 2f, | |
| y = imageTop + (imageHeight - initialSize) / 2f | |
| ) | |
| ) | |
| } | |
| private val imageAspectRatio = originalBitmap.height.toFloat() / originalBitmap.width | |
| val imageWidth = containerSize.width.toFloat() | |
| val imageHeight = imageWidth * imageAspectRatio | |
| val imageTop = (containerSize.height - imageHeight) / 2f | |
| private val maxCropSize = min(imageWidth, imageHeight) | |
| private val _cropProperties = mutableStateOf( | |
| run { | |
| val initialSize = min(imageWidth, imageHeight) * 0.8f | |
| CropProperties( | |
| cropSize = initialSize, | |
| cropOffset = Offset( | |
| x = (imageWidth - initialSize) / 2f, | |
| y = imageTop + (imageHeight - initialSize) / 2f | |
| ) | |
| ) | |
| } | |
| ) | |
| val cropProperties: State<CropProperties> = _cropProperties |
yuni-ju
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.
- 이미지 편집 시 핀치 줌으로 편리하게 크롭이 잘 되는 것 너무 좋습니다~!
- 다만 한 번 편집 후 재편집 시 original 이미지로 돌아가 수정할 수 없는 점은 따로 말씀드렸지만 다음에 논의해보기 위해 적어 놓아요
- 대용량 이미지에 대응하여 큰 비트맵을 효율적으로 로드할 때 inJustDecodeBounds를 쓸 수 있다는 것을 알게 되었습니다 👍 inSampleSize 기법 써주신 것 좋습니다.
- 이미지 정보 이용해서 회전 처리하는거 헷갈리는데 잘 처리해주셨네요 !! 😁
- 비트맵 쓰고 메모리 해제하는 부분들과 캐시 키에 수정시간도 추가한 것도 확인 했습니다
| .memoryCacheKey(cacheKey) | ||
| .diskCacheKey(cacheKey) |
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.
ImageRequest 시 memoryCacheKey와 diskCacheKey를 둘 다 쓰는 이유가 있는지 궁금합니다!
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.
커스텀 캐시키를 사용하기 때문에, 동일한 캐시키를 사용함으로써 메모리 캐시와 디스크 캐시의 일관성 유지를 통해 안정적으로 동일한 이미지를 보여주기 위함입니다
…e/ImageCropScreen.kt Co-authored-by: Copilot <[email protected]>
작업 사항
구현 내용
구현 세부사항
Custom Image Cropper
대용량 이미지 로드
inJustDecodeBounds = true/false참고