인증샷 촬영 기능 구현 #41
인증샷 촬영 기능 구현 #41
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@feature/task-certification/src/main/AndroidManifest.xml`:
- Around line 1-3: TaskCertificationScreen에서 Camera 권한을 런타임에 요청하지 않아
CaptureCamera.bind()/bindCamera() 호출 시 SecurityException이 발생할 수 있으므로,
LaunchedEffect에서 bindCamera()를 호출하기 전에 권한 체크 및 요청 흐름을 추가하세요: Jetpack Compose의
rememberPermissionState() 또는 rememberMultiplePermissionsState()를 사용해 CAMERA 권한
상태를 관리하고 권한이 허용된 경우에만 bindCamera()/CaptureCamera.bind()를 실행하며, 거부된 경우 사용자에게 권한
필요 안내(토스트/다이얼로그/권한 재요청 UI)를 표시하도록 구현하세요.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/component/CameraPreviewBox.kt`:
- Around line 51-55: The AsyncImage in CameraPreviewBox.kt currently sets
contentDescription = null, which prevents screen readers from conveying the
captured image; update the AsyncImage (identifier: AsyncImage, model:
captureStatus.uri) to provide a localized, meaningful description via
stringResource (e.g., stringResource(R.string.captured_image_description))
instead of null, and add the corresponding string resource key
(captured_image_description) to your strings.xml so the description can be read
by accessibility services.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/component/TaskCertificationTopBar.kt`:
- Around line 30-37: Add an accessible label for the close Image by wiring a
string resource into its contentDescription: replace the null contentDescription
in the Image composable (the Image call that uses ImageVector.vectorResource and
.noRippleClickable(onClickClose)) with a localized string via
stringResource(R.string.task_certification_close) and consider adding semantics
role = Role.Button (or use Modifier.semantics { role = Role.Button }) on the
same Modifier to expose it as a button to assistive tech; also add the string
resource <string name="task_certification_close">닫기</string> to strings.xml.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/model/CameraPreview.kt`:
- Around line 6-8: CameraPreview에 선언된 `@Immutable` 어노테이션이 잘못된 약속을 하므로 제거하거나 불변만
포함하는 래퍼로 변경하세요: CameraPreview 데이터 클래스에서 `@Immutable` 어노테이션을 삭제하거나 SurfaceRequest를
직접 보관하지 않고 진정으로 불변인 값(예: 식별자/설정 값)만 포함하는 새로운 불변 래퍼 클래(또는 CameraPreviewImmutable
등)를 만들어 CameraPreview는 그 래퍼를 참조하도록 변경하고 SurfaceRequest 관련 가변 동작은 해당 래퍼 밖에서
처리하세요.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt`:
- Around line 41-49: The camera-toggle UI is wired with an empty lambda for
onToggleCameraClick causing no response; either wire it to the ViewModel intent
that toggles cameras (e.g., call
viewModel.dispatch(TaskCertificationIntent.ToggleCamera) inside the
onToggleCameraClick passed to TaskCertificationScreen) or, if toggle is not
implemented yet, disable or hide the toggle control or show a “coming
soon/disabled” state instead so the button does not appear interactive; update
TaskCertificationScreen invocation accordingly.
- Around line 29-50: TaskCertificationRoute currently doesn't collect the
ViewModel's side-effect Flow so capture/failure events (e.g., capture failure)
are never shown to the user; update TaskCertificationRoute to collect the
ViewModel's sideEffect Flow (from TaskCertificationViewModel, e.g.,
viewModel.sideEffect or sideEffects) inside a lifecycle-aware coroutine
(LaunchedEffect or lifecycleScope) and handle failure events by showing a
Snackbar/Toast or passing an event into your UI (use existing
TaskCertificationScreen callbacks or a ScaffoldState) so that emitted failure
events are consumed and visible to users.
In `@feature/task-certification/src/main/res/drawable/ic_camara_rotate.xml`:
- Line 1: The drawable resource filename contains a typo: rename
ic_camara_rotate.xml to ic_camera_rotate.xml and update all references to it
(e.g., any R.drawable.ic_camara_rotate, layout XML drawables, vector usages or
style references) to R.drawable.ic_camera_rotate so the resource name and usages
stay consistent; ensure the XML root and contents remain unchanged and rebuild
resources to confirm no missing-reference errors.
🧹 Nitpick comments (14)
feature/task-certification/src/main/res/drawable/ic_camara_flash_off.xml (1)
6-17: 하드코딩 색상 대신 색상 리소스 사용 권장왜 문제인가요:
#ffffff하드코딩은 다크 모드/테마 변경 시 일관된 색상 관리가 어려워집니다.
어떻게 개선하나요: 디자인 시스템 색상(예:@color/icon_on_surface_60)을 정의해 참조하도록 바꾸는 게 좋습니다. 다른 아이콘들과 색상 톤을 맞추기 위해 리소스 통일도 함께 고려해보실래요?feature/task-certification/src/main/res/drawable/ic_camara_rotate.xml (1)
11-11: 색상 값 하드코딩 대신 테마 속성 사용 권장
strokeColor가#ffffff(흰색)으로 하드코딩되어 있습니다.왜 개선이 필요한가?
- 다크모드나 테마 변경 시 아이콘 색상이 배경과 대비되지 않을 수 있습니다
- 디자인 시스템의 색상 팔레트와 분리되어 일관성 유지가 어렵습니다
- 런타임에 색상을 동적으로 변경하기 어렵습니다
개선 방법:
- 색상 리소스 참조 방식:
@color/icon_primary또는@color/white- 테마 속성 참조 방식 (권장):
?attr/colorOnPrimary또는?attr/colorOnSurface- 또는 코드에서
ImageView.setColorFilter()또는 Compose의tint파라미터를 통해 동적으로 색상 적용참고:
- 다른 카메라 관련 아이콘들(
ic_camera_flash_on.xml,ic_camera_flash_off.xml등)도 동일한 패턴을 사용하고 있다면 일괄적으로 개선하는 것이 좋습니다- Compose에서
Icon컴포저블을 사용할 경우tint파라미터로 색상을 지정하는 방식이 더 유연합니다💡 색상 리소스 참조 방식 예시
android:strokeLineJoin="round" android:strokeWidth="2" android:fillColor="#00000000" - android:strokeColor="#ffffff" + android:strokeColor="?attr/colorOnSurface" android:strokeLineCap="round"/>또는 Compose에서 사용 시:
Icon( painter = painterResource(R.drawable.ic_camera_rotate), contentDescription = "카메라 회전", tint = MaterialTheme.colorScheme.onSurface // 테마 색상 적용 )Based on coding guidelines, 리소스 파일의 색상과 테마가 디자인 시스템에 통합되어야 합니다.
feature/task-certification/src/main/res/drawable/image.xml (1)
1-9: 하드코딩 색상과 큰 intrinsic 크기로 테마/레이아웃 영향 가능왜 문제인가:
android:fillColor="#ffffff"는 다크모드/테마 변경에 대응하지 못하고 디자인 시스템을 우회합니다. 또한441dpintrinsic 사이즈는 사용처에서 크기를 명시하지 않으면 레이아웃에 의도치 않은 크기로 반영될 수 있습니다.fillAlpha="0.01"로 거의 보이지 않아 리소스 누락처럼 보일 위험도 있습니다.어떻게 개선할까: 테마 속성(예:
?attr/colorOnSurface)이나@color/...로 대체하고, 사용처에서 항상 크기를 지정할 계획이 아니라면 기본 크기를 더 보편적인 값(예: 24dp)으로 낮추거나 의도를 드러내는 리소스명/주석을 추가하는 게 좋습니다. 이 드로어블이 마스크/플레이스홀더 용도라면 투명 색상으로 바꾸는 게 더 명확하지 않을까요?feature/task-certification/src/main/java/com/peto/task_certification/component/CameraPreviewBox.kt (1)
34-38: 코너 반경 중복값을 상수로 추출하면 유지보수성이 향상됩니다.
RoundedCornerShape(73.83.dp)이 34~38번 라인에서 두 번 반복되고 있습니다. 같은 값을 여러 곳에서 사용하면 나중에 수정할 때 누락되기 쉽고, 일관성 문제가 발생할 수 있습니다. 로컬 변수로 추출하면 한 곳에서 관리할 수 있어 훨씬 안전합니다.♻️ 제안 수정안
internal fun CameraPreviewBox( captureStatus: CaptureStatus, previewRequest: CameraPreview?, ) { + val previewShape = RoundedCornerShape(73.83.dp) Box( modifier = Modifier .fillMaxWidth() .padding(horizontal = 5.dp) .aspectRatio(1f) .border( color = GrayColor.C400, width = 2.dp, - shape = RoundedCornerShape(73.83.dp), + shape = previewShape, ).clip(RoundedCornerShape(73.83.dp)), ) {- ).clip(RoundedCornerShape(73.83.dp)), + ).clip(previewShape),feature/task-certification/src/main/res/drawable/ic_close_c100.xml (1)
6-10: 하드코딩 색상은 테마 일관성을 깨뜨릴 수 있어요
왜 문제인가: 벡터에 직접 색상을 고정하면 다크모드/테마 변경 시 색 일관성 유지가 어렵습니다.
어떻게 개선: 디자인 시스템 컬러 리소스(@color또는 ?attr)로 치환해 주세요.♻️ 개선 예시
- android:strokeColor="#ECECEC" + android:strokeColor="@color/gray_c100"feature/task-certification/src/main/res/drawable/ic_camera_shutter.xml (1)
1-16: 하드코딩 색상은 테마 대응에 취약합니다.
왜:#464646등 직접 지정 색상은 다크모드/브랜드 컬러 변경 시 일관성 유지가 어렵습니다.
어떻게:@color/...또는?attr/...로 분리해 테마/디자인 시스템에서 관리해 주세요.
리소스 리뷰 가이드에 따르면, 색상은 디자인 시스템 리소스로 관리하는 것이 권장됩니다.feature/task-certification/src/main/res/drawable/ic_camera_toggle.xml (1)
1-15: 토글 아이콘도 색상 리소스화하면 테마 대응이 쉬워집니다.
왜: 하드코딩 색상은 다크모드/브랜드 컬러 변경에 취약합니다.
어떻게:@color/...또는?attr/...로 분리해 테마에서 제어할 수 있게 해주세요.
리소스 리뷰 가이드에 따르면, 색상은 디자인 시스템 리소스로 관리하는 것이 권장됩니다.feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationSideEffect.kt (1)
5-6: 실패 원인 정보가 없으면 UI/로그 처리에 한계가 있습니다.
왜: 단순 객체만 전달되면 사용자 메시지/로깅에 필요한 원인 정보를 잃습니다.
어떻게:Throwable이나 에러 코드/메시지를 포함한 data class로 확장하는 것을 고려해 보실래요?💡 예시
sealed interface TaskCertificationSideEffect : SideEffect { - data object ImageCaptureFailException : TaskCertificationSideEffect + data class ImageCaptureFailed( + val throwable: Throwable, + ) : TaskCertificationSideEffect }feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationIntent.kt (1)
6-11: LifecycleOwner를 Intent 매개변수로 전달하는 것은 메모리 누수 위험은 없으나, 아키텍처 개선이 가능합니다.현재 구현을 확인한 결과,
bindCamera()에서 lifecycleOwner를 즉시 사용하고 ViewModel에 보관하지 않기 때문에 메모리 누수 위험은 없습니다. 다만 두 가지 개선 고려사항이 있습니다:
ViewModel의 Android 프레임워크 타입 의존성: ViewModel이
LifecycleOwner타입에 직접 의존하게 됩니다. 테스트 시 Android 프레임워크 의존성을 피하기 위해, 카메라 바인딩을 UI 레이어에서 직접 수행하거나 Intent가 아닌 별도 메커니즘으로 처리하는 방식을 고려할 수 있습니다.MVI 패턴 관점:
BindCamera는 엄밀히는 사용자 액션이 아니라 초기화/라이프사이클 작업입니다. 이러한 설정 작업이 Intent로 흐르는 것이 최적인지 검토해 보세요. 예를 들어,LaunchedEffect에서remember(lifecycleOwner) { camera.bind(...) }로 직접 처리하는 방식도 고려 가능합니다.현재 코드 구조가 안정적이라면 이는 선택적 개선 사항입니다.
feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationUiState.kt (1)
7-12: UI State가 MVI 패턴에 맞게 잘 구성되었습니다.
@Immutable어노테이션과data class를 사용하여 Compose의 recomposition 최적화를 지원하고, 불변 상태를 보장합니다.State인터페이스 구현으로 MVI 패턴을 따르고 있습니다.한 가지 고려할 점은
CameraSelector가 CameraX 라이브러리의 타입인데, 이를 UI State에 직접 노출하면 UI 레이어가 카메라 구현체에 의존하게 됩니다. 향후 확장성을 위해 자체 enum(예:LensDirection.FRONT,LensDirection.BACK)으로 추상화하고, ViewModel에서 CameraSelector로 매핑하는 방식을 고려해 보실 수 있습니다.enum class LensDirection { FRONT, BACK }현재 구현도 동작에는 문제가 없으므로, 이 부분은 향후 리팩터링 시 검토해 주세요.
feature/task-certification/src/main/java/com/peto/task_certification/camera/Camera.kt (1)
10-24: 카메라 기능을 인터페이스로 추상화한 점이 좋습니다.외부 라이브러리(CameraX)를 인터페이스로 감싸서 테스트 용이성과 유연성을 확보했습니다.
StateFlow를 통한 reactive한 surface 제공 방식도 적절합니다.다만
bind/unbind는suspend fun으로,takePicture는 콜백 기반으로 되어 있어 API 일관성 측면에서 검토가 필요합니다.takePicture도suspend fun으로 변경하고Result<Uri>를 반환하면 호출부에서 코루틴으로 일관되게 처리할 수 있습니다.♻️ 선택적 개선 제안
- fun takePicture( - onComplete: (Uri?) -> Unit, - onFailure: (ImageCaptureException) -> Unit, - ) + suspend fun takePicture(): Result<Uri>현재 콜백 방식도 동작에 문제는 없으므로, 이 부분은 코드 스타일 선호에 따라 결정하시면 됩니다.
feature/task-certification/src/main/java/com/peto/task_certification/component/CameraControlBar.kt (1)
39-62: State Hoisting이 잘 적용되었고, Preview도 제공되어 있습니다.Composable 함수가 순수 함수로 작성되었고, 클릭 이벤트가 콜백으로 호이스팅되어 MVI 패턴에 부합합니다.
접근성(Accessibility) 개선을 위해
contentDescription에 의미 있는 설명을 추가하는 것을 권장합니다. 스크린 리더 사용자가 버튼의 용도를 파악할 수 있도록 해주세요.♻️ 접근성 개선 제안
Image( imageVector = ImageVector.vectorResource(R.drawable.ic_camera_shutter), - contentDescription = null, + contentDescription = "촬영 버튼", modifier = Modifier .noRippleClickable(onCaptureClick), ) Image( imageVector = ImageVector.vectorResource(R.drawable.ic_camera_toggle), - contentDescription = null, + contentDescription = "카메라 전환", modifier = Modifier .noRippleClickable(onToggleCameraClick), )또한 첫 번째
Image(Line 39-47)는 촬영된 사진의 썸네일 표시 용도로 보이는데, 향후 촬영된 이미지를 표시하려면 동적으로 이미지 소스를 받을 수 있도록 파라미터 추가가 필요할 것 같습니다.feature/task-certification/src/main/java/com/peto/task_certification/camera/CaptureCamera.kt (1)
78-80: API 레벨 조건 확인이 필요합니다.
MediaStore.Images.Media.RELATIVE_PATH는 Android Q(API 29)에서 추가되었습니다. 현재 조건Build.VERSION.SDK_INT > Build.VERSION_CODES.P는 API 29 이상을 의미하므로 동작은 정확하지만, 가독성을 위해>= Build.VERSION_CODES.Q로 명시하는 것이 더 명확합니다.♻️ 가독성 개선 제안
- if (Build.VERSION.SDK_INT > Build.VERSION_CODES.P) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { put(MediaStore.Images.Media.RELATIVE_PATH, IMAGE_PATH) }feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationViewModel.kt (1)
46-46: 실패 처리 중복과 오타를 정리하면 유지보수성이 좋아집니다.
왜: 실패 처리 로직이onFailureCapture()와 중복되고,updateCapturedCImage오타가 혼란을 줄 수 있습니다.
어떻게: 실패 분기를onFailureCapture()로 위임하고 메서드명을 정정해 주세요.♻️ 제안 수정안
- updateCapturedCImage(it) + updateCapturedImage(it) ... - private fun updateCapturedCImage(uri: Uri?) { - uri?.let { - reduce { copy(capture = CaptureStatus.Captured(uri)) } - } ?: run { - viewModelScope.launch { - emitSideEffect(TaskCertificationSideEffect.ImageCaptureFailException) - } - } - } + private fun updateCapturedImage(uri: Uri?) { + uri?.let { + reduce { copy(capture = CaptureStatus.Captured(uri)) } + } ?: onFailureCapture() + }Also applies to: 71-79
...e/task-certification/src/main/java/com/twix/task_certification/component/CameraPreviewBox.kt
Outdated
Show resolved
Hide resolved
...certification/src/main/java/com/twix/task_certification/component/TaskCertificationTopBar.kt
Show resolved
Hide resolved
feature/task-certification/src/main/java/com/twix/task_certification/model/CameraPreview.kt
Show resolved
Hide resolved
feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt
Show resolved
Hide resolved
feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationUiState.kt`:
- Line 24: Rename the method updateCapturedCImage to updateCapturedImage in
TaskCertificationUiState (keep its implementation copy(capture =
CaptureStatus.Captured(uri)) unchanged), and update all references/call sites to
call updateCapturedImage instead of updateCapturedCImage to avoid the typo;
ensure any unit tests or usages that reference updateCapturedCImage are updated
accordingly.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt`:
- Line 38: Rename the misspelled local variable uiSate to uiState where it's
defined from viewModel.uiState.collectAsStateWithLifecycle() and update all its
usages (e.g., the follow-up usage around the original Line 53) to the new name;
specifically change the declaration "val uiSate by
viewModel.uiState.collectAsStateWithLifecycle()" to use uiState and replace any
references to uiSate elsewhere in TaskCertificationScreen.kt to uiState to
restore consistent naming.
In
`@feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationViewModel.kt`:
- Around line 76-84: The function name updateCapturedCImage is a typo—rename it
(and the corresponding TaskCertificationUiState.updateCapturedCImage method) to
updateCapturedImage everywhere to keep naming consistent; also unify coroutine
usage: if reduce is a suspend function, make updateCapturedImage a suspend
function and call reduce(...) directly, then call emitSideEffect(...) (or have
the caller invoke updateCapturedImage from viewModelScope.launch); otherwise, if
reduce is non-suspending, wrap both reduce(...) and emitSideEffect(...) in the
same viewModelScope.launch so coroutine handling is consistent (update
references to viewModelScope, emitSideEffect, reduce, and
TaskCertificationUiState.updateCapturedImage accordingly).
♻️ Duplicate comments (1)
feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt (1)
33-65: SideEffect Flow 수집이 누락되어 있습니다.왜: ViewModel에서 캡처 실패 등의 SideEffect 이벤트를 emit하더라도 Route에서 수집하지 않으면 사용자에게 오류 피드백(스낵바/토스트 등)을 제공할 수 없습니다. 코딩 가이드라인에 따르면 SideEffect는 일회성 이벤트로 분리되어 적절히 처리되어야 합니다.
어떻게:
LaunchedEffect를 사용하여 ViewModel의 sideEffect Flow를 수집하고, 실패 이벤트 발생 시 사용자에게 피드백을 제공해 주세요.♻️ 구현 예시
LaunchedEffect(Unit) { viewModel.sideEffect.collect { effect -> when (effect) { is TaskCertificationSideEffect.CaptureFailure -> { // 스낵바 또는 토스트로 사용자에게 오류 안내 } // 다른 SideEffect 처리 } } }
🧹 Nitpick comments (6)
feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationUiState.kt (1)
9-13: CameraSelector를 UI State에 직접 사용하는 것에 대해 검토가 필요합니다.현재
lens프로퍼티가CameraSelector라는 CameraX 프레임워크 클래스를 직접 참조하고 있습니다.왜 문제가 될 수 있는가:
- Compose 안정성 이슈:
CameraSelector가 Compose의 안정성(Stability) 규칙을 만족하지 않을 수 있어, 불필요한 recomposition이 발생할 수 있습니다.- 테스트 용이성 저하: UI State 테스트 시 CameraX 의존성이 필요해집니다.
- 추상화 위반: PR 설명에서 "외부 Camera 라이브러리를 인터페이스로 추상화함"이라고 언급하셨는데, State 레벨에서도 동일한 추상화 원칙을 적용하면 더 일관성 있는 설계가 됩니다.
어떻게 개선할 수 있는가:
렌즈 선택을 위한 도메인 enum을 정의하고, CameraSelector로의 변환은 Camera 구현체 내부에서 처리하는 방식을 고려해 보시겠어요?♻️ 제안하는 개선 방향
// LensType.kt enum class LensType { BACK, FRONT } // TaskCertificationUiState.kt `@Immutable` data class TaskCertificationUiState( val capture: CaptureStatus = CaptureStatus.NotCaptured, val lens: LensType = LensType.BACK, val preview: CameraPreview? = null, ) : State { fun toggleLens(): TaskCertificationUiState { val newLens = if (lens == LensType.BACK) LensType.FRONT else LensType.BACK return copy(lens = newLens) } // ... }Camera 구현체에서 LensType → CameraSelector 변환:
// CaptureCamera.kt 내부 private fun LensType.toCameraSelector(): CameraSelector = when (this) { LensType.BACK -> CameraSelector.DEFAULT_BACK_CAMERA LensType.FRONT -> CameraSelector.DEFAULT_FRONT_CAMERA }feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt (2)
41-43: LaunchedEffect 키를lifecycleOwner로 변경하는 것이 더 안전합니다.왜:
LaunchedEffect(Unit)을 사용하면 Composable이 처음 진입할 때 한 번만 실행됩니다. 만약lifecycleOwner가 변경되는 경우(예: Activity 재생성 등), 카메라가 새로운 lifecycle에 다시 바인딩되지 않을 수 있습니다.어떻게: 키를
lifecycleOwner로 변경하면 lifecycle 변경 시 자동으로 재바인딩됩니다.♻️ 개선 제안
- LaunchedEffect(Unit) { + LaunchedEffect(lifecycleOwner) { viewModel.dispatch(TaskCertificationIntent.BindCamera(lifecycleOwner)) }
80-82: 콜백 전달 시 불필요한 람다 래핑을 제거할 수 있습니다.왜:
{ onClickClose() }처럼 람다로 감싸면 매번 새로운 람다 인스턴스가 생성되어 미세하게나마 불필요한 객체 할당이 발생합니다.어떻게: 함수 참조를 직접 전달하면 더 간결하고 효율적입니다.
♻️ 간결한 코드 제안
TaskCertificationTopBar( - onClickClose = { onClickClose() }, + onClickClose = onClickClose, )feature/task-certification/src/main/java/com/peto/task_certification/model/TaskCertificationIntent.kt (1)
9-15: Intent에 LifecycleOwner 포함 여부에 대한 논의현재
BindCamera와ToggleCameraIntent에LifecycleOwner가 포함되어 있습니다. MVI 패턴에서 Intent는 사용자 액션을 순수하게 표현하는 것이 이상적인데, Android 프레임워크 클래스가 포함되면 다음과 같은 문제가 발생할 수 있습니다:
- 테스트 복잡성 증가: Intent를 테스트할 때 LifecycleOwner를 모킹해야 합니다.
- 계층 간 결합도 증가: Intent가 프레임워크에 의존하게 됩니다.
CameraX의 특성상 LifecycleOwner가 필요한 것은 이해하지만, ViewModel이 직접 LifecycleOwner를 관리하거나 Screen 레벨에서 바인딩을 처리하는 방식도 고려해 볼 수 있습니다. 현재 구현이 CameraX 요구사항을 충족하기 위한 실용적인 선택이라면, 이 부분에 대한 팀 내 논의 결과를 공유해 주시면 좋겠습니다.
feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationViewModel.kt (2)
30-44: Intent 처리 구조 검토
handleIntent가suspend함수로 정의되어 있지만, 내부에서 호출하는bindCamera,toggleCamera등은suspend함수가 아니면서 내부적으로viewModelScope.launch를 사용합니다. 이로 인해handleIntent의 suspend 컨텍스트가 활용되지 않고, 새로운 코루틴이 별도로 실행됩니다.일관성을 위해 두 가지 방향을 고려해 볼 수 있습니다:
- 내부 함수들을
suspend함수로 변경하여handleIntent의 컨텍스트 내에서 실행- 현재 구조를 유지하되, 비동기 작업이 독립적으로 실행되어야 하는 의도를 주석으로 명시
현재 구조에서 특별한 버그는 없지만, 코루틴 실행 흐름을 명확히 하면 유지보수에 도움이 됩니다.
86-89:toggleCamera를 suspend 함수로 변경하여 MVI 패턴 일관성 유지현재
toggleCamera는handleIntentsuspend 함수 내에서 호출되지만, 자신은 일반 함수로 정의되어 있습니다.reduce { toggleLens() }는 동기적으로 상태를 즉시 업데이트하고,bindCamera가 호출되므로 상태 업데이트 순서 자체는 문제없습니다.다만 MVI 패턴의 단방향 데이터 플로우를 명확히 하기 위해
toggleCamera를 suspend 함수로 변경하고, 상태 업데이트와 카메라 바인딩의 순서를 명시적으로 보장하는 것을 권장합니다. 이렇게 하면 Intent 처리의 suspend 체인이 유지되어 의도가 더욱 명확해집니다:private suspend fun toggleCamera(lifecycleOwner: LifecycleOwner) { reduce { toggleLens() } bindCamera(lifecycleOwner) }이를 통해 Intent 처리의 완전한 순서 보장과 코루틴 컨텍스트의 일관성을 확보할 수 있습니다.
...sk-certification/src/main/java/com/peto/task_certification/model/TaskCertificationUiState.kt
Outdated
Show resolved
Hide resolved
feature/task-certification/src/main/java/com/peto/task_certification/TaskCertificationScreen.kt
Outdated
Show resolved
Hide resolved
...e/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt
Outdated
Show resolved
Hide resolved
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
이거 일단 NavRoutes랑 NavGraph 구현해서 DI 컨테이너에 등록하는 거 먼저 추가해주세요! AppNavHost에서 라우트만 변경해서 실행하고 싶은데 아직 구현이 안 되어있는 것 같아요
| } | ||
|
|
||
| android { | ||
| namespace = "com.peto.task_certification" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
이거 실제 기기에서 실행하면 카메라 권한 체크도 없고 카메라를 활용하지 못하고 있어요. 권한 체크 후에 접근 가능하도록 수정해주세요!
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
뷰모델에 라이프사이클 넘기는 거 수정되면 다시 리뷰해볼게요!
| viewModel.dispatch(TaskCertificationIntent.BindCamera(lifecycleOwner)) | ||
| } |
There was a problem hiding this comment.
지금 뷰모델한테 LifecycleOwner를 넘기는 게 UI 책임을 뷰모델한테 넘기는 거라서 바꾸면 좋을 것 같아요. 이렇게 하면 뷰모델 로직 테스트도 어렵고 컴포저블보다 뷰모델 생명주기가 더 길어서 화면을 나가도 카메라가 유지되는 문제가 있을 수도 있습니다.
UI에서 카메라 bind, unbind를 책임지고 뷰모델은 상태랑 이벤트만 관리하는 쪽으로 수정해주세요!
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt
# Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt # Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt # feature/task-certification/src/main/java/com/twix/task_certification/model/TaskCertificationIntent.kt
# Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt # Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt # feature/task-certification/src/main/java/com/twix/task_certification/model/TaskCertificationIntent.kt
…ication-torch # Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt # feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt # feature/task-certification/src/main/java/com/twix/task_certification/camera/CaptureCamera.kt # feature/task-certification/src/main/java/com/twix/task_certification/model/TaskCertificationIntent.kt
|
PR을 나누기 위해서 feat/#38-task-certification 브랜치 기준으로 각각 Base brach를 분리해서 작업했는데,
브랜치가 통합되면서 Github에서 강제로 task-certification-torch 브랜치로 만든 PR 을 머지해버려서 그래서 리뷰하기 복잡할 것 같아서 이번 작업에서 수정한 내용들 정리해봤어
이 내용 때문에 따로 권한 처리를 안했던 건데 생각해보니 테스트가 불가능했군 😅
갤러리 사진 선택 기능 & 재촬영 기능 구현 PR은 |
이슈 번호
#38
리뷰/머지 희망 기한 (선택)
2025.01.29
작업내용
카메라를 사용하려면 카메라 권한이 필수인데, 이 권한을 언제 받을지에 대해 지훈이랑 이야기를 해봤어
아요는 할일 인증 화면으로 이동하기 전에 권한을 확인하고 권한이 허용됐을 때만 인증 화면으로 이동하도록
구현했다고 하는데 우리도 동일하게 진행하는건 어떻게 생각해 ?
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
할일 인증 화면 기능 요구사항