Skip to content
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

Merged
merged 30 commits into from
Nov 8, 2023

Conversation

briandr97
Copy link
Collaborator

@briandr97 briandr97 commented Oct 17, 2023

📌 관련 이슈

🛠️ 작업 내용

  • ActivityContactResults의 TakePhotoPreview에서 TakePhoto로 api를 변경한다.
  • api 변경에 따라 콜백 반환값이 달라지므로 해당 변화를 반영한다.
  • 리사이징 기능 추가

🎯 리뷰 포인트

두 가지 문제가 있습니다.

  1. 가로모드 문제
    이게 말로 설명하기가 참 어려운데.. 내일 직접 보여드리도록하겠습니다.. 사진을 찍을 때 카메라로 넘어가는데 카메라에서는 가로모드가 가능합니다. 이때 찍고 돌아올때 휴대폰이 가로로 있으면 터집니다.

  2. 하위 버전 문제
    api28 레벨의 에뮬레이터로 테스트 해본 결과 또 권한 문제로 터지네요..
    일단 이 부분은 해결하지 못했고 함께 보고싶어서 이렇게 올립니다.

Process: com.now.naaga, PID: 10517
java.lang.SecurityException: Permission Denial: writing com.android.providers.media.MediaProvider uri content://media/external/images/media from pid=10517, uid=10086 requires android.permission.WRITE_EXTERNAL_STORAGE, or grantUriPermission()
 at android.os.Parcel.createException(Parcel.java:1950)
 at android.os.Parcel.readException(Parcel.java:1918)
 at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:183)
 at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:135)
 at android.content.ContentProviderProxy.insert(ContentProviderNative.java:476)
 at android.content.ContentResolver.insert(ContentResolver.java:1587)
 at com.now.naaga.presentation.upload.UploadActivity.createImageUri-d1pmJ48(UploadActivity.kt:123)
 at com.now.naaga.presentation.upload.UploadActivity.openCamera(UploadActivity.kt:210)
 at com.now.naaga.presentation.upload.UploadActivity.requestPermissionLauncher$lambda$4(UploadActivity.kt:66)
 at com.now.naaga.presentation.upload.UploadActivity.$r8$lambda$6iXFuysMCt3ohHGRwh62fqePNaQ(Unknown Source:0)
 at com.now.naaga.presentation.upload.UploadActivity$$ExternalSyntheticLambda1.onActivityResult(Unknown Source:4)
 at androidx.activity.result.ActivityResultRegistry.dispatchResult(ActivityResultRegistry.java:406)
 at androidx.activity.ComponentActivity$2$1.run(ComponentActivity.java:209)
 at android.os.Handler.handleCallback(Handler.java:873)
 at android.os.Handler.dispatchMessage(Handler.java:99)
 at android.os.Looper.loop(Looper.java:193)
 at android.app.ActivityThread.main(ActivityThread.java:6669)
 at java.lang.reflect.Method.invoke(Native Method)
 at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
 at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

image

⏳ 작업 시간

추정 시간: 3h
실제 시간: 2h
이유: 차이가 많이 난다면 이유도 같이 적어주세요 :)

@briandr97
Copy link
Collaborator Author

briandr97 commented Oct 18, 2023

변경이 생겼습니다!!
우선 이전 버전을 위해 STORAGE에 대한 WRITE 권한이 추가되었습니다. (API 레벨 28인 에뮬레이터에서 테스트 완료했습니다)
다음으로 권한과 스낵바 관련 로직이 변경되었는데, 변경된 이유는 다음과 같습니다.

발생했던 이슈

  • 위치 권한을 수락했는데 로딩이 사라지지 않는다.
  • 위치 권한 스낵바의 버튼을 눌러서 설정에서 권한을 허락했는데 돌아와도 로딩이 사라지지 않는다.
  • 스낵바의 시간이 너무 짧아서 유저가 인지하기 어렵다.

해결 방법

  • 위치 권한을 수락했을 때는 로딩이 사라지도록 PermissionLauncher에서 위치 권한이 수락됐는지 확인하는 로직을 추가하고 로직이 수락되면 setCoordinate를 호출해 로티가 사라질 수 있도록 하였습니다.
  • 카메라 권한의 경우 설정에 갔다 오면 다시 버튼을 누르는 방식이라 문제가 없지만 위치 권한의 경우 설정에 다녀오면 자동으로 권한 체크를 한 번 더 해줘야 합니다. 그래서 설정에 가는 런처를 따로 만들어서 해당 런처의 콜백이 발생했을 때 setCoordinate를 다시 호출해주었습니다.
  • 스낵바의 유지 시간이 너무 짧아 이동하기 버튼을 누를 생각이 들기 전에 사라집니다. 그래서 위치권한의 경우 누르지 않으면 없어지지 않도록, 카메라 권한의 경우 긴 시간을 바꿔주었습니다!

Copy link
Collaborator

@hyunji1203 hyunji1203 left a 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도 회전 이슈만 신경써주시면 될 것 같습니다!

Comment on lines 62 to 80
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()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
해당 부분을 isStorageRequestisLocationRequest 으로 나눠 로직 분리 해보는 것은 어떤가요?
분리를 하게 되면 좀더 가독성이 좋아질 것 같아요!

Copy link
Collaborator Author

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 길어지니까 좋네요👍

Comment on lines 264 to 272
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
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
해당 부분을 getSampleSizecalculateSampleSize 로 분리할수도 있을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BitmapBuilder 클래스에 함수를 분리해보았습니다.

Copy link
Collaborator

@krrong krrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 고생많았습니다 빅스!!
클래스 분리를 잘 해준 것 같아서 좋네요 :)
이 친구가 저희를 정말 많이 속썩이네요..😢
튼튼하게 만들기 위한 코멘트를 달아두었습니다. 확인부탁해요!

  • 추가로 테스트가 깨져있습니다. 확인부탁할게요!

Comment on lines 11 to 13
<uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE"
android:maxSdkVersion="32" />
Copy link
Collaborator

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입니다!

Copy link
Collaborator Author

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입니다" })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1]
string을 상수화 해도 좋겠습니다!

Copy link
Collaborator Author

@briandr97 briandr97 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러 문구를 상수화해야 할 이유가 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러문구를 상수화 해놓으면 동반객체를 보았을 때 어떤 오류들이 발생할 수 있겠구나 한 번에 볼 수 있을 것 같아요.
사실 현재 코드가 길기 때문에 더 보기 힘든 것도 있지만 한 번에 관리하는 것이 코드의 가독성도, 리뷰에도 편한 것 같습니다. 만약 정말 에러가 발생한다고 가정했을 때 해당 문구를 어디서 사용하는지 바로가기를 통해 확인할 수도 있구요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 사실 에러 문구를 굳이 상수화 해야하는가? 라고 생각합니다.
보통 에러를 확인할 때 이 객체에서 어떤 에러가 일어날 수 있을까?를 생각하며 한눈에 모든 에러를 살펴보기 보다는 에러가 발생했을 때 어떤 에러가 발생했는 지를 확인한다고 생각합니다.
그렇기 때문에 굳이 한 곳에 모아놓아야하나? 라는 의문점이 듭니다.
또한 에러가 정말로 발생했을 때도 해당 에러가 발생한 곳으로 바로 이동이 가능하기 때문에 거기서 변수를 한 번 더 눌러서 확인하는 것 보다는 해당 코드에서 어떤 에러인지 바로 인지할 수 있는 것이 더 좋다고 생각합니다.

Comment on lines 219 to 224
private fun openCameraWithPermission() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
return openCamera()
}
requestPermissionLauncher.launch(storagePermissions)
}
Copy link
Collaborator

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인 경우 카메라를 열 수 없습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왼쪽은 공식문서, 오른쪽은 api 레벨 28인 에뮬레이터에서 업로드를 실행한 사진입니다.

공식문서에 따르면 30부터는 READ, 29에선 READ or WRITE, 28이하에선 모두 필요한 것으로 보입니다.
티라미수 분기는 크롱이 기존에 작성해놓은 코드라 이유가 있을 것이라 생각하고 그대로 사용하였습니다.
(아마 33부터는 해당 권한들이 필요없는거겠죠?)

우선 크롱이 어떤 이유로 말씀하신지 아직 완전히 이해는 못했지만 28 기기에선 잘 작동하는 것을 확인했습니다.

image image

Copy link
Collaborator

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 권한을 제외하고 요청하는 분기 로직이 필요해보입니다.

Copy link
Collaborator Author

@briandr97 briandr97 Nov 6, 2023

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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1]
이 부분도 상수화 하면 좋겠어요!

Copy link
Collaborator Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3]
빅스는 빠르게 터뜨리기 vs 안전하게 처리하기 중 어떤 것을 더 선호하시나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상황에 따라 다르지만 현재 상황에서는 빈 파일이 서버에 업로드 되는 것 보단 에러를 발생시키고 실패했다는 피드백을 유저에게 주는 것이 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 상황에서는 isFormValid로 체크하므로 터질일은 없다고 생각됩니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 안전하게 처리하는 방법을 선택했던 것인데요. 이것을 선택한 이유는 서버에서 한 번 검수를 한 뒤에 데이터를 넣는 작업을 진행하고 있고, File이 없는 경우 사용자에게 피드백을 주어도 어떠한 대처를 할 수 없다고 판단했기 때문입니다.
사실 isFormValid메서드를 통해 Null인지 확인하고 있어 터질일은 없겠지만요.

Copy link
Collaborator Author

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3]
클래스로 만들어 사용하는 것이 아니라 Activity가 이것을 상속(구현)하도록 한 이유가 있나요?

Copy link
Collaborator Author

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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1]
이 코드에서 붉은 줄이 나오네요.. 이 메서드를 호출하는 곳에서 권한 체크를 하고 호출하는 것을 알지만 붉은 줄이 나오는 것은 좋지 않다고 생각합니다.
아마 300줄이 넘어가는 Activity의 코드를 줄이기 위해 고민한 흔적같지만,, 저는 이 로직이 Activity가 가지고 있는 것이 낫겠다는 생각이 듭니다.
다른 곳에서 코드를 줄이고 책임을 분리해보는 것을 함께 고민해보는 것은 어떨까요?

Copy link
Collaborator Author

@briandr97 briandr97 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 여전히 gms 관련 함수들을 분리하는 것이 맞다고 생각이 듭니다. (물론 이 로직뿐만 아니라 현재 액티비티에 분리해야 하는 로직이 굉장히 많긴 합니다)
단순히 뷰를 다루는 것과 뷰의 데이터를 뷰모델에 전달하는 정도를 제외하고 가능하면 액티비티에서 분리하는게 가독성 측면에서 훨씬 좋지 않을까 합니다. 현재 액티비티가 너무 많은 일을 하고 있고 가독성이 굉장히 떨어집니다. 그러므로 역할별로 나누어 분리할 수 있는 부분은 분리하는게 맞지 않을까 라고 생각합니다. (예를 들어 비트맵을 다루는 부분이나 해당 로직처럼 위치를 다루는 로직 등)
(추가적으로 저는 현재 비트맵, 파일 등을 다루는 로직 또한 액티비티가 아닌 데이터 레이어에서 이루어져야한다고 생각합니다)

지금은 시간이 없으니 우선 해당 로직은 원래대로 액티비티에 돌려놓도록 하겠습니다!
그래도 위 의견에 어떻게 생각하시는지 궁금하네요!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

액티비티 코드의 로직을 분리해야 한다는 의견에 동의합니다. 그러면 가독성 측면에서도 좋아질 것 같구요.
하지만 Context를 사용해야 하는 로직은 액티비티에 있어야 하는 것이 맞다고 생각이 듭니다. 그 외의 것들을 덜어낸다면 충분히 가독성 좋은 코드가 완성될 것 같아요.

추가적으로 저는 현재 비트맵, 파일 등을 다루는 로직 또한 액티비티가 아닌 데이터 레이어에서 이루어져야한다고 생각합니다

이렇게 생각하는 이유가 있나요?

Copy link
Collaborator Author

@briandr97 briandr97 Oct 19, 2023

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로부터 가져옵니다.

Copy link
Collaborator Author

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
클래스 분리를 잘 하는 것 같아요.👍
Builder를 만들어서 사용하는 곳을 보면 깔끔하고 좋은 것 같다고 느껴집니다.

Comment on lines +27 to +32
while (true) {
if (width / 2 < resize || height / 2 < resize) break
width /= 2
height /= 2
sampleSize *= 2
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
어떤 역할을 하는 코드일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지를 리사이징하는 코드입니다. 너비 혹은 높이의 절반이 resize 수치보다 낮아질 때까지 반복하며 sampleSize를 구합니다. 그리고 해당 sampleSize를 적용한 options으로 인풋 스트림을 열어 비트맵을 가져옵니다.

Comment on lines +66 to +67
val matrix = Matrix()
matrix.setRotate(orientation.toFloat(), (bitmap.width / 2).toFloat(), (bitmap.height / 2).toFloat())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
어떤 역할을 하는 코드일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 급하게 가져온 코드라 동작 원리를 설명드릴 수는 없지만, 이전에 구해온 orientation을 통해 이미지를 찍은 방향에 맞게 돌려주는 역할을 합니다.

Copy link
Collaborator

@krrong krrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트 남겨두었습니다! 확인부탁할게요!

Comment on lines +179 to +182
action = {
val appDetailsIntent = getSettingIntent()
locationSettingLauncher.launch(appDetailsIntent)
},
Copy link
Collaborator

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) 으로 변경하면 코드가 더 줄어들고 가독성도 좋아질 것 같아요!

Copy link
Collaborator Author

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3]
저장소 권한은 LONG을 주고, 위치 권한은 LENGTH_INDEFINITE을 준 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 PR 첫 게시글에 적혀있습니다

Copy link
Collaborator

@hyunji1203 hyunji1203 left a 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을 여는데 실패했습니다." }
Copy link
Collaborator

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로 해당하는 인덱스의 값을 찾아주는 로직을 대체할 수 있는건가요?! 이 과정들을 한번해 간단하게 할 수 있게 해주는 메서드..?

Copy link
Collaborator Author

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를 만들기 위해 긴 로직을 작성할 필요가 없어진 것입니다.

Copy link
Collaborator

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 }
Copy link
Collaborator

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으로 작성해야 하는게 아닌가라는 생각이 들었는데..
잠깐의 코드리뷰를 통해 판단한 제 생각이기도 하고 이렇게 작성한 빅스의 생각과 과정이 있었을 것 같아 정말 궁금해 물어봅니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it이 맞습니다. 잘못 작성했네요 수정했습니다!

Copy link
Collaborator

@hyunji1203 hyunji1203 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빅스 고생많았어요!
최종적으로 로직 수정한 부분도 확인했습니다!
그럼 전 이만 approve 하겠습니다!👍

Copy link
Collaborator

@krrong krrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 변경이 필요해보이는 점이 없어 머지하도록 할게요 :)
고생했습니다 빅스!

@krrong krrong merged commit d2adc40 into dev_android Nov 8, 2023
3 checks passed
@krrong krrong deleted the feat/#496-imageQuality branch November 8, 2023 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants