PDF MVI 리팩토링 #29

Guri999 · 2025-01-07T12:26:28Z

#️⃣연관된 이슈

ex) #27

📝작업 내용

MVI 로 상태관리

뷰모델을 작성하여 MVI 로 PDF 화면의 상태를 관리합니다
Intent는 Topbar의 표시, 페이지 변경, 페이지 렌더링, 초기화로 구별하였습니다

Koin 설정

Koin을 사용해 의존성 관리를 하기위해 추가하였습니다

💬리뷰 요구사항(선택)

리베이스 하는 와중 커밋이 꼬인것 같습니다
Refactor: TopBarState onBodyPress -> show 커밋에 뷰모델생성 작업

Chore: 안쓰는 람다 제거 커밋에 뷰모델에 맞춰 수정된 화면 작업로그가 있습니다

(cherry picked from commit 91e5fa5)

f-lab-dean · 2025-01-07T13:04:44Z

feature/pdf/src/main/java/kr/co/pdf/model/UiState.kt

+import kotlin.coroutines.EmptyCoroutineContext
+
+internal data class UiState(
+    val scope: CoroutineScope = CoroutineScope(EmptyCoroutineContext),


코루틴 스코프는 UI가 유지해야할 상태가 아닐 것 같습니다.
코루틴 스코프가 Ui의 상태인 이유가 무엇인가요?

TopBar에서 사용할 코루틴 스코프를 뷰모델스코프로 받기위해 만들었습니다..

fun show() { currentJob?.cancel() currentJob = scope.launch { topBarVisible = true delay(3000) topBarVisible = false } }

탑바에서 이런 로직을 수행하기 위해 뷰모델 스코프를 받으려고 했는데 뷰모델에서 전달해야 하지 않을까요?
아니면 UiState에서 scope가 아닌 topBarState에 스코프를 전달할까요?

TopBarState는 다른 화면에서 사용할 수 도 있다 판단해서 분리했는데 뷰모델에서 저 로직들을 작업하는게 좋을까요??

ViewModelScope가 UI의 "상태"에 포함되는 것이 구조적으로 옳은지를 먼저 판단해야 할 것 같습니다. 단순히 TopBar에서 로직을 수행해야 하니 ViewModelScope가 전달되어야 한다는 것은 올바르지 못한 접근인 것 같습니다. TopBar의 로직을 수행해야해서 코루틴 스코프가 필요하다면 Composable에서 rememberCoroutineScope 을 통해 코루틴 스코프를 생성해서 전달해도 되니까요.

UI의 상태는 "화면에 표현해야할 앱의 데이터" 라는 정의에 따라 데이터 중심으로 설계해보시면 좋겠습니다. 또한 코루틴 스코프를 UI 상태로써 사용할 때 발생할 수 있는 문제점에 대해 고민해보고, 특히 리컴포지션과 연관지어 설계해보세요.

PDF 화면 구성 및 PDF 렌더링 #27 (comment) < 이 코멘트에서 언급되었듯이, TopBar를 생성할 때 잘못된 CoroutineScope를 통해 생성되는 경우를 대비할 수 없는 구조로 보입니다. 이 부분을 고려해주세요.

show 함수를 suspend 함수로 만들고 호출하는 쪽에서 CoroutineScope를 제어하는 구조로 가는 방법, 코루틴을 사용하지 않고 특정 시간 이후에 topBarVisible를 변경할 수 있는 타이머 구현 등의 방법으로 대체가 가능합니다.

다른 화면에서 사용할 수 있는 공통 컴포넌트라면 TopBarState를 공통화해서 공유하는 것 보단 저러한 View 로직을 수행하는 TopBar 컴포저블 함수를 만들어서 공통으로 사용하는것이 좋을 것 같습니다.

f-lab-dean · 2025-01-07T13:05:10Z

app/src/main/java/kr/co/seedocs/SeeDocs.kt

 import timber.log.Timber

-@HiltAndroidApp


힐트를 제거하고 코인으로 변경한 이유가 무엇인가요?

KMP로의 확장성, 새로운 기술스택 추가하기 위해 Koin을 사용했습니다

KMP로의 확장성이 현재 프로젝트의 목적 또는 범위에 부합한가요?

새로운 기술스택은 어떤 것이 있을까요?

현재 프로젝트의 범위에 부합하진 않지만 추후에 추가적인 작업을 할 수 있다고 생각했습니다
새로운 기술스택은 Hilt가 아닌 Koin을 사용하는것을 생각했습니다

프로젝트 시작 전의 설계나 계획을 바꾸는 것은 실제 프로덕트에서는 매우 제한적인 작업입니다. 특히 기술 스택을 변경하는 것은 더욱 그렇습니다.

Koin으로 개발한 경험이 기존에 없으셨었나요? 제 기억이 가물가물..

개발을 할 때에는 많이 미래지향적인 것은 바람직하지 않습니다. YAGNI 원칙을 떠올려보면 좋겠습니다.

어떠한 이유로 설계, 계획이 바뀌는 것은 기존의 것이 잘못되었음을 의미하기도 합니다. 이러한 상황이 반복되지 않도록 셀프 피드백을 해주세요.

이 코멘트는 완료하겠습니다.

f-lab-dean · 2025-01-07T13:07:16Z

feature/pdf/src/main/java/kr/co/pdf/model/UiIntent.kt

+
+import android.graphics.pdf.PdfRenderer
+
+internal sealed class UiIntent {


UiIntent, UiState와 같이 특정 View를 의미하는 이름이 아닌 이유는 이 모듈내에 이 화면만 있어서 그런걸까요?
이 모듈내에 만약 다른화면이 추가되면 구분을 위해 변경이 필요할 듯 싶은데 PdfViewIntent, PdfViewState 와 같이 명확한 이름을 하는것은 어떠신가요?

internal로 다른 화면에서 접근을 제한하기 때문에 괜찮다 생각했습니다

비슷한 예로 Compose에서 Preview를 작성할때 private로 만들고

PdfScreenPreview 와 같은 식이 아닌 private fun Preview 와 같은 식으로 작성하는 등

외부에서 접근을 제한한다면 괜찮다고 생각했습니다

네 그래서 제가 가정을 하고 질문드린 것은 "모듈내에 만약 다른화면이 추가되면" 입니다.
한 모듈에는 무조건 하나의 화면만 존재하는 정책인가요?

internal은 해당 모듈내에서는 public이나 마찬가지이기 때문에, Composable을 private로 선언해서 파일 외에서는 접근을 불가능하게 하는 것과는 별개의 상황으로 보입니다.

현재 계획상으론 모듈내에 하나의 화면만 존재하게 하려해서 이렇게 지었는데

공통된 Base가 될 인터페이스로 저이름을 쓰고 화면에 맞춰서 모델 명을 변경하는게 좋을것 같습니다

넵, 지금 프로젝트는 규모가 작지만, 큰 규모의 프로젝트에서 모듈당 화면 하나만 가져야 하는 정책이라면 모듈이 수도없이 많아질 것 같습니다.
모듈을 어떻게 설계하는 것이 좋을지 고민해보는 것도 좋을 것 같습니다. :)

이 코멘트는 완료하겠습니다.

f-lab-dean · 2025-01-07T13:07:39Z

feature/pdf/src/main/java/kr/co/pdf/PdfViewModel.kt

+import kr.co.pdf.model.UiState
+import kr.co.util.PdfToBitmap
+
+internal class PdfViewModel : ViewModel() {


만약 PdfViewModel에서 UI로 이벤트를 전달해야 하는 경우엔 어떻게 할 수 있을까요?

Model -> View 방향으로 영향을 주니 _uiState를 변경할 것 같습니다

다이얼로그나 토스트 노출 등의 이벤트라면 어떤가요?

다이얼로그는 Model에 Boolean 프로퍼티를 추가해주고 Intent를 받아 변경해주는식으로
토스트 노출은 인텐트를 받고 sharedFlow로 처리할 것 같습니다

�다음 아티클을 참고해보면 좋겠습니다.
https://medium.com/myrealtrip-product/android-mvi-79809c5c14f0

MVI 아키텍처에서 "상태"에 포함시킬 수 없는 데이터를 Side Effect로 처리하고 있습니다.
그리고 이것은 네트워크 요청, 다이얼로그 노출, 토스트 노출과 같은 것을 포함합니다.

이 아티클에 소개된 내용을 보시고 왜 Side Effect가 MVI 아키텍처가 필요한지 이해해보면 좋을 것 같습니다.

f-lab-dean · 2025-01-07T13:09:05Z

feature/pdf/src/main/java/kr/co/pdf/model/UiState.kt

+    val scope: CoroutineScope = CoroutineScope(EmptyCoroutineContext),
+    val bitmaps: Map<Int, Bitmap> = emptyMap(),
+    val topBarState: TopBarState = TopBarState.create(scope),
+    val currentPage: Int = 1,


currentPage는 View에서 사용하지 않는것 같은데 상태에 추가된 이유가 있나요?

수정했습니다

f-lab-dean

프로젝트 진행을 위해 승인하겠습니다.
코멘트를 꼭 확인하시고 고민해보시면 좋겠습니다.

f-lab-dean · 2025-01-08T08:37:31Z

core/ui/src/main/java/kr/co/ui/base/BaseMviViewModel.kt

+        }
+    }
+
+    protected fun update(


공유) reduce 라는 용어를 종종 쓰곤 합니다. :)

좋은것 같습니다 머지전 수정하겠습니다

f-lab-dean · 2025-01-08T08:43:04Z

feature/pdf/src/main/java/kr/co/pdf/PdfViewModel.kt

+                }
+            }
+
+            update {


Job 종료 후에 어떤 행위가 필요하다면 invokeOnCompletion을 활용할 수 있습니다.
https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-job/invoke-on-completion.html

수정하겠습니다

f-lab-dean · 2025-01-08T08:43:38Z

feature/pdf/src/main/java/kr/co/pdf/PdfViewModel.kt

+        launch {
+            pdfToBitmap?.renderPage(page)
+
+            pdfToBitmap?.bitmap?.collect { bitmaps ->


renderPage가 호출될 때 마다 이 collect Job이 계속 등록될 것 같은데, 중복으로 collect가 호출되었을때 문제는 없나요?

init에서 collect하는 방식으로 변경했습니다

f-lab-dean · 2025-01-08T08:48:13Z

feature/pdf/src/main/java/kr/co/pdf/PdfViewModel.kt

+        renderer: PdfRenderer,
+        topBarState: TopBarState,
+    ) {
+        pdfToBitmap = PdfToBitmap(renderer)


ViewModel에 주입되는 형태가 아니라 내부에서 직접 생성되고 유지되는 객체는 테스트를 어렵게 만들 수 있습니다.
이 부분을 고려해보면 좋겠습니다.

렌더러가 path에 따라 달라지기 때문에 di로 주입하긴 힘들다고 생각했는데 따로 클래스를 만들어 렌더러만 만드는 클래스를 캡슐화해야만 가능할 것 같아요..

sonarqubecloud · 2025-01-08T10:38:35Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Guri999 added 4 commits January 7, 2025 20:52

Refactor: TopBarState onBodyPress -> show

28981d9

Chore: 안쓰는 람다 제거

dd3db45

Rebase

33e0673

Feat: Koin 설정

414e37d

(cherry picked from commit 91e5fa5)

Guri999 self-assigned this Jan 7, 2025

f-lab-dean reviewed Jan 7, 2025

View reviewed changes

Guri999 added 4 commits January 7, 2025 23:08

Fix: currentPage TopBar 적용

1837ea0

Refactor: ui 상태 scope 제거

6ef782b

Chore: UiIntent UiState 이름 변경

50ce34b

Refactor: BaseViewModel 작성

a224f7f

f-lab-dean approved these changes Jan 8, 2025

View reviewed changes

Guri999 added 3 commits January 8, 2025 19:20

Refactor: Job 종료 후 작업 invokeOnCompletion

9951db8

Refactor: update -> reduce

07e9fe9

Refactor: bitmap collect 위치 변경

5d7ee0f

Guri999 merged commit 72375a2 into main Jan 8, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDF MVI 리팩토링 #29

PDF MVI 리팩토링 #29

Guri999 commented Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025

Guri999 Jan 7, 2025

Guri999 Jan 7, 2025

f-lab-dean Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025

Guri999 Jan 7, 2025

f-lab-dean Jan 7, 2025

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025

f-lab-dean Jan 7, 2025

Guri999 Jan 7, 2025

f-lab-dean Jan 7, 2025 •

edited

Loading

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025

f-lab-dean Jan 7, 2025

Guri999 Jan 7, 2025

f-lab-dean Jan 7, 2025

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025

Guri999 Jan 7, 2025

f-lab-dean left a comment

f-lab-dean Jan 8, 2025

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025

Guri999 Jan 8, 2025

f-lab-dean Jan 8, 2025

Guri999 Jan 8, 2025

sonarqubecloud bot commented Jan 8, 2025


		import android.graphics.pdf.PdfRenderer

		internal sealed class UiIntent {

PDF MVI 리팩토링 #29

PDF MVI 리팩토링 #29

Conversation

Guri999 commented Jan 7, 2025 • edited Loading

#️⃣연관된 이슈

📝작업 내용

MVI 로 상태관리

Koin 설정

💬리뷰 요구사항(선택)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-dean Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-dean Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-dean Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-dean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

Guri999 commented Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 8, 2025 •

edited

Loading