-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: 업로드 화질 저하 이슈 해결 #498
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.
빅스 고생많았어요!
API 29에서도 확인해봤는데 거기도 잘되더라고요!👍
빅스가 말한 이슈들 집중해서 확인했습니다!
빅스가 말한 이수들이 해결된 것은 확인했는데,,,새로운 이슈가 발생하였습니다.
오프라인으로 말한 90도 회전 저장 이슈가 생겼습니다..
몇가지 반영안해도 되는 리뷰 달았는데
시간이 부족하니 리뷰 반영은 안하셔도 되고, 젤 중요한 90도 회전 이슈만 신경써주시면 될 것 같습니다!
val isStorageRequest = storagePermissions.any { keys.contains(it) } | ||
if (isStorageRequest) { | ||
if (permission.entries.map { it.value }.contains(false)) { | ||
showPermissionSnackbar(getString(R.string.snackbar_storage_message)) | ||
showStoragePermissionSnackBar() | ||
} else { | ||
openCamera() | ||
} | ||
return@registerForActivityResult | ||
} | ||
showPermissionSnackbar(getString(R.string.snackbar_location_message)) | ||
|
||
val isLocationRequest = locationPermissions.any { keys.contains(it) } | ||
if (isLocationRequest) { | ||
if (permission.entries.map { it.value }.contains(false)) { | ||
showLocationPermissionSnackBar() | ||
return@registerForActivityResult | ||
} | ||
setCoordinate() | ||
} | ||
} |
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.
[P5]
해당 부분을 isStorageRequest
와 isLocationRequest
으로 나눠 로직 분리 해보는 것은 어떤가요?
분리를 하게 되면 좀더 가독성이 좋아질 것 같아요!
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.
좀 이상하지만 분리해보았습니다.
actionTitle = getString(R.string.snackbar_action_title), | ||
length = Snackbar.LENGTH_LONG, |
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.
확실히 길어지니까 좋네요👍
var width = options.outWidth | ||
var height = options.outHeight | ||
var sampleSize = 1 | ||
while (true) { | ||
if (width / 2 < RESIZE || height / 2 < RESIZE) break | ||
width /= 2 | ||
height /= 2 | ||
sampleSize *= 2 | ||
} |
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.
[P5]
해당 부분을 getSampleSize
나 calculateSampleSize
로 분리할수도 있을 것 같아요!
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.
BitmapBuilder 클래스에 함수를 분리해보았습니다.
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.
정말 고생많았습니다 빅스!!
클래스 분리를 잘 해준 것 같아서 좋네요 :)
이 친구가 저희를 정말 많이 속썩이네요..😢
튼튼하게 만들기 위한 코멘트를 달아두었습니다. 확인부탁해요!
- 추가로 테스트가 깨져있습니다. 확인부탁할게요!
<uses-permission | ||
android:name="android.permission.WRITE_EXTERNAL_STORAGE" | ||
android:maxSdkVersion="32" /> |
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.
[P1]
WRITE_EXTERNAL_STORAGE 의 권한의 maxSdkVersion은 28입니다!
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.
변경하였습니다.
} | ||
setImage(requireNotNull(imageUri) { "imageUri가 null입니다" }) |
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.
[P1]
string을 상수화 해도 좋겠습니다!
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.
에러 문구를 상수화해야 할 이유가 있을까요?
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.
에러문구를 상수화 해놓으면 동반객체를 보았을 때 어떤 오류들이 발생할 수 있겠구나 한 번에 볼 수 있을 것 같아요.
사실 현재 코드가 길기 때문에 더 보기 힘든 것도 있지만 한 번에 관리하는 것이 코드의 가독성도, 리뷰에도 편한 것 같습니다. 만약 정말 에러가 발생한다고 가정했을 때 해당 문구를 어디서 사용하는지 바로가기를 통해 확인할 수도 있구요.
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.
저는 사실 에러 문구를 굳이 상수화 해야하는가? 라고 생각합니다.
보통 에러를 확인할 때 이 객체에서 어떤 에러가 일어날 수 있을까?를 생각하며 한눈에 모든 에러를 살펴보기 보다는 에러가 발생했을 때 어떤 에러가 발생했는 지를 확인한다고 생각합니다.
그렇기 때문에 굳이 한 곳에 모아놓아야하나? 라는 의문점이 듭니다.
또한 에러가 정말로 발생했을 때도 해당 에러가 발생한 곳으로 바로 이동이 가능하기 때문에 거기서 변수를 한 번 더 눌러서 확인하는 것 보다는 해당 코드에서 어떤 에러인지 바로 인지할 수 있는 것이 더 좋다고 생각합니다.
private fun openCameraWithPermission() { | ||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
return openCamera() | ||
} | ||
requestPermissionLauncher.launch(storagePermissions) | ||
} |
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.
[P1]
API 레벨 28이상인 기기에서 WRITE_STORAGE_PERMISSION을 줄 수 없기 때문에 항상 false입니다.
그래서 API 28 ~ API 32인 경우 카메라를 열 수 없습니다.
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.
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.
조금 더 자세하게 설명해보면, 현재 storagePermissions
에는 READ권한과 WRITE권한이 모두 들어가 있고, API 레벨 31인 휴대폰에서 실행한 경우 WRITE 권한이 false이기 때문에 카메라를 열 수 없습니다.
설정으로 이동하여도 READ 권한만 줄 수 있구요.
그래서 API 레벨 29 이상인 경우 WRITE 권한을 제외하고 요청하는 분기 로직이 필요해보입니다.
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.
29에서 제대로 작동하지 않는 것을 확인하고 수정했습니다!
API 레벨 28 이하만 권한을 요구함으로 해결했습니다.
API 레벨 28, 29, 31 테스트 완료했습니다.
|
||
private fun openCamera() { | ||
imageUri = createImageUri().getOrElse { | ||
Snackbar.make(binding.root, "다시 시도해주세요!", Snackbar.LENGTH_SHORT).show() |
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.
[P1]
이 부분도 상수화 하면 좋겠어요!
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.
리소스화 했습니다.
@@ -21,7 +21,7 @@ import javax.inject.Inject | |||
class UploadViewModel @Inject constructor( | |||
private val placeRepository: PlaceRepository, | |||
) : ViewModel() { | |||
private var file = FILE_EMPTY | |||
private var file: File? = null |
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.
[P3]
빅스는 빠르게 터뜨리기 vs 안전하게 처리하기 중 어떤 것을 더 선호하시나요?
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.
상황에 따라 다르지만 현재 상황에서는 빈 파일이 서버에 업로드 되는 것 보단 에러를 발생시키고 실패했다는 피드백을 유저에게 주는 것이 좋을 것 같습니다.
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.
해당 상황에서는 isFormValid로 체크하므로 터질일은 없다고 생각됩니다.
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.
저는 안전하게 처리하는 방법을 선택했던 것인데요. 이것을 선택한 이유는 서버에서 한 번 검수를 한 뒤에 데이터를 넣는 작업을 진행하고 있고, File이 없는 경우 사용자에게 피드백을 주어도 어떠한 대처를 할 수 없다고 판단했기 때문입니다.
사실 isFormValid
메서드를 통해 Null인지 확인하고 있어 터질일은 없겠지만요.
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.
저는 대처를 할 수 없더라도 유저가 등록이 되었는지 되지 않았는지를 아는게 좋다고 생각합니다.
또한 마지막 줄에서 말씀하셨듯이 해당 방법이 안전하지 않다고 생각하지 않습니다!
import com.google.android.gms.tasks.OnTokenCanceledListener | ||
import com.now.domain.model.Coordinate | ||
|
||
class DefaultLocationDelegate : LocationDelegate { |
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.
[P3]
클래스로 만들어 사용하는 것이 아니라 Activity가 이것을 상속(구현)하도록 한 이유가 있나요?
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.
해당 액티비티의 코드가 너무 길어져서 로케이션을 다루는 역할을 분리했습니다.
저도 확실한 기준을 가지고 만든 것은 아니라서 해당 방식이 아닌 유틸클래스로 분리하는 등 다른 방식을 사용해도 괜찮을 것 같습니다.
class DefaultLocationDelegate : LocationDelegate { | ||
override fun setCoordinateListener(context: Context, getLocation: (location: Location) -> Unit) { | ||
val fusedLocationClient = LocationServices.getFusedLocationProviderClient(context) | ||
fusedLocationClient.getCurrentLocation(PRIORITY_HIGH_ACCURACY, createCancellationToken()) |
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.
[P1]
이 코드에서 붉은 줄이 나오네요.. 이 메서드를 호출하는 곳에서 권한 체크를 하고 호출하는 것을 알지만 붉은 줄이 나오는 것은 좋지 않다고 생각합니다.
아마 300줄이 넘어가는 Activity의 코드를 줄이기 위해 고민한 흔적같지만,, 저는 이 로직이 Activity가 가지고 있는 것이 낫겠다는 생각이 듭니다.
다른 곳에서 코드를 줄이고 책임을 분리해보는 것을 함께 고민해보는 것은 어떨까요?
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.
저는 여전히 gms 관련 함수들을 분리하는 것이 맞다고 생각이 듭니다. (물론 이 로직뿐만 아니라 현재 액티비티에 분리해야 하는 로직이 굉장히 많긴 합니다)
단순히 뷰를 다루는 것과 뷰의 데이터를 뷰모델에 전달하는 정도를 제외하고 가능하면 액티비티에서 분리하는게 가독성 측면에서 훨씬 좋지 않을까 합니다. 현재 액티비티가 너무 많은 일을 하고 있고 가독성이 굉장히 떨어집니다. 그러므로 역할별로 나누어 분리할 수 있는 부분은 분리하는게 맞지 않을까 라고 생각합니다. (예를 들어 비트맵을 다루는 부분이나 해당 로직처럼 위치를 다루는 로직 등)
(추가적으로 저는 현재 비트맵, 파일 등을 다루는 로직 또한 액티비티가 아닌 데이터 레이어에서 이루어져야한다고 생각합니다)
지금은 시간이 없으니 우선 해당 로직은 원래대로 액티비티에 돌려놓도록 하겠습니다!
그래도 위 의견에 어떻게 생각하시는지 궁금하네요!
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.
액티비티 코드의 로직을 분리해야 한다는 의견에 동의합니다. 그러면 가독성 측면에서도 좋아질 것 같구요.
하지만 Context를 사용해야 하는 로직은 액티비티에 있어야 하는 것이 맞다고 생각이 듭니다. 그 외의 것들을 덜어낸다면 충분히 가독성 좋은 코드가 완성될 것 같아요.
추가적으로 저는 현재 비트맵, 파일 등을 다루는 로직 또한 액티비티가 아닌 데이터 레이어에서 이루어져야한다고 생각합니다
이렇게 생각하는 이유가 있나요?
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.
혹시 덜어낸다는 그 외의 것들이 무엇이 있을까요?
물론 저도 저 방식으로 덜어내는 것에 대한 확신은 없습니다. 좀 더 좋은 방식이 있을수도요!
하지만 덜어내지 않아야 할 이유가 context 때문이라면 이해가 조금 어려울 것 같습니다.
저는 해당 로직을 자세히 몰라 어떤 Context가 필요한지는 모르지만, 실제로 applicationContext는 프로젝트 곳곳에서 사용되며, activity context라 하더라도 Activity와 생명주기를 같이하는 객체에는 주지 않아야할 이유가 없다고 생각됩니다.
그렇게 사용된 예시로 제가 작성했던 BitmapBuilder
또한 contentResolver를 받고 있는데 contentResolver는 Context로부터 가져옵니다.
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.
우선 액티비티로 옮겨놓았습니다.
import android.provider.MediaStore | ||
import androidx.exifinterface.media.ExifInterface | ||
|
||
class BitmapBuilder( |
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.
[P5]
클래스 분리를 잘 하는 것 같아요.👍
Builder를 만들어서 사용하는 곳을 보면 깔끔하고 좋은 것 같다고 느껴집니다.
while (true) { | ||
if (width / 2 < resize || height / 2 < resize) break | ||
width /= 2 | ||
height /= 2 | ||
sampleSize *= 2 | ||
} |
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.
[P5]
어떤 역할을 하는 코드일까요?
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.
이미지를 리사이징하는 코드입니다. 너비 혹은 높이의 절반이 resize
수치보다 낮아질 때까지 반복하며 sampleSize
를 구합니다. 그리고 해당 sampleSize
를 적용한 options
으로 인풋 스트림을 열어 비트맵을 가져옵니다.
val matrix = Matrix() | ||
matrix.setRotate(orientation.toFloat(), (bitmap.width / 2).toFloat(), (bitmap.height / 2).toFloat()) |
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.
[P5]
어떤 역할을 하는 코드일까요?
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.
저도 급하게 가져온 코드라 동작 원리를 설명드릴 수는 없지만, 이전에 구해온 orientation을 통해 이미지를 찍은 방향에 맞게 돌려주는 역할을 합니다.
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.
코멘트 남겨두었습니다! 확인부탁할게요!
action = { | ||
val appDetailsIntent = getSettingIntent() | ||
locationSettingLauncher.launch(appDetailsIntent) | ||
}, |
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.
[P1]
이 액션은 다른 곳에서도 사용되고 있어 제가 Context의 확장함수로 만들어두었습니다. 그것을 사용하는 것은 어떨까요?
그리고 그렇게 된다면 showStoragePermissionSnackBar()
와 showLocationPermissionSnackBar()
메서드는 message의 차이만 있게 됩니다.
showPermissionSnackBar(message: String)
으로 변경하면 코드가 더 줄어들고 가독성도 좋아질 것 같아요!
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.
해당 확장함수는 확인 했지만 위에서 말씀하셨듯이 보여주는 길이가 달라져서 사용하지 못했습니다.
크롱이 만들어둔 확장함수는 전부 SHORT
이기 때문입니다.
actionTitle = getString(R.string.snackbar_action_title), | ||
length = Snackbar.LENGTH_LONG, |
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.
[P3]
저장소 권한은 LONG을 주고, 위치 권한은 LENGTH_INDEFINITE을 준 이유가 있나요?
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.
해당 부분은 PR 첫 게시글에 적혀있습니다
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.
빅스! 고생많았어요!👍
기존에 있었던 오류들도 수정되어 사진이 잘 올라가는 것도 확인했습니다!
어프로브 날리기 전 마지막으로 궁금한 부분이 있어 해당 부분 리뷰 남겼습니다!
담번엔 어프로브 갈기도록 하겠습니다~~
} | ||
|
||
private fun getImageOrientation(): Int { | ||
val inputStream = requireNotNull(contentResolver.openInputStream(imageUri)) { "Uri로 InputStream을 여는데 실패했습니다." } |
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.
오호 이렇게 로직이 변경되었군요??
궁금한 점이 있는데, openInputStream 을 사용하면 기존에 contentResolver.query를 활용해 cursor를 만들고 해당 cursor로 해당하는 인덱스의 값을 찾아주는 로직을 대체할 수 있는건가요?! 이 과정들을 한번해 간단하게 할 수 있게 해주는 메서드..?
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.
저도 자세히는 모르지만, ExifInterface
를 생성하려면 file, filePath, fileDescriptor, inputStream 중 하나가 필요합니다.
기존에는 filePath로 구현되어 있었고 권한 처리가 잘 된 28, 33 레벨에서 잘 됐습니다. 그런데 29, 31에서도 가능하도록 권한 로직을 수정한 뒤 시도해보니 29에서 ExifInterface
를 생성할 때 fileNotFoundException이 발생하며 (permission denied)라고 에러 로그가 발생한 것을 볼 수 있었습니다.
그래서 이것 저것 시도해보다가 inputStream으로 ExifInterface
를 생성했더니 성공했습니다.
궁금해서 각 생성자를 들어가 보았는데 결국 inputStream을 만들어서 사용하는 것을 볼 수 있습니다. 결국 최종적으로 inputStream이 필요하기 때문에 inputStream을 만들기 위한 filePath를 만들기 위해 긴 로직을 작성할 필요가 없어진 것입니다.
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.
오호 그랬던거군요 이해완!👍
openCamera() | ||
private val locationPermissionLauncher = | ||
registerForActivityResult(ActivityResultContracts.RequestMultiplePermissions()) { permissions -> | ||
val isGranted: Boolean = permissions.values.all { true } |
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.
[P4]
여기서 true를 it으로 바꿔도 동작을 하는데 true로 작성한 이유가 무엇인가요?
true로 작성한다면 항상 위치 권한이 부여된 것으로 판단하게 되지 않나요?! 그래서 it으로 작성해야 하는게 아닌가라는 생각이 들었는데..
잠깐의 코드리뷰를 통해 판단한 제 생각이기도 하고 이렇게 작성한 빅스의 생각과 과정이 있었을 것 같아 정말 궁금해 물어봅니다!!
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.
it이 맞습니다. 잘못 작성했네요 수정했습니다!
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.
빅스 고생많았어요!
최종적으로 로직 수정한 부분도 확인했습니다!
그럼 전 이만 approve 하겠습니다!👍
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.
추가로 변경이 필요해보이는 점이 없어 머지하도록 할게요 :)
고생했습니다 빅스!
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
두 가지 문제가 있습니다.
가로모드 문제
이게 말로 설명하기가 참 어려운데.. 내일 직접 보여드리도록하겠습니다.. 사진을 찍을 때 카메라로 넘어가는데 카메라에서는 가로모드가 가능합니다. 이때 찍고 돌아올때 휴대폰이 가로로 있으면 터집니다.
하위 버전 문제
api28 레벨의 에뮬레이터로 테스트 해본 결과 또 권한 문제로 터지네요..
일단 이 부분은 해결하지 못했고 함께 보고싶어서 이렇게 올립니다.
⏳ 작업 시간
추정 시간: 3h
실제 시간: 2h
이유: 차이가 많이 난다면 이유도 같이 적어주세요 :)