refactor: 로그인 뷰 Compose 마이그레이션 및 모듈 분리 #666

s9hn · 2025-05-01T06:53:11Z

📌𝘐𝘴𝘴𝘶𝘦𝘴

closed refactor: 로그인 뷰 Compose 마이그레이션 및 모듈 분리 #660

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

로그인 뷰를 구현하는:feature:signin 모듈이 추가되었습니다.
- 기존 뷰 Compose로 마이그레이션
- DotIndicator 써드파티 라이브러리 제거 및 커스텀 DotIndicator 구현
- 기존 핸들러-루퍼를 통한 뷰페이저 자동 전환 기능 -> Flow + Channel로 리팩터링
- Compose Lifecycle safe한 Flow Event 수집용 확장함수 구현
확장 함수를 포함하는 :core:common 모듈이 추가되었습니다.

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

KakaoTalk_Video_2025-05-01-16-00-08.mp4

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

토큰 리팩터링 하려다가 그냥 로그인 기능을 a..z 만들고 있네요.
현재 Compose로 구현된 SignInScreen은 기존 LoginActivity 환경에서 테스트 했습니다.
모든 기능 구현이 완료되고, QA까지 완료하면 기존 뷰와 교체하겠습니다.

release: 1.2.0 버전 업데이트(to develop)

…o feat/660

m6z1

굿 깔끔하네요!

m6z1 · 2025-05-01T13:58:31Z

...re/signin/src/main/java/com/into/websoso/feature/signin/component/OnboardingDotsIndicator.kt

+        List(pagerState.pageCount) { index ->
+            val distance = (index - currentPage) - offsetFraction
+            val clamped = distance.absoluteValue.coerceIn(0f, 1f)
+            val isSelected = clamped < 0.5f


얘는 상수화하면 좋을 것 같아요 👍

yeonjeen

수고많으셨습니다!!! 앞으로 마이그레이션 하는 거 두근두근하네요 ㅎ

yeonjeen · 2025-05-02T00:51:15Z

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt

+        when (event) {
+            is ScrollToPage -> {
+                val nextPage = (pagerState.currentPage + 1) % pagerState.pageCount
+                pagerState.animateScrollToPage(nextPage)


c: 루트에서 이벤트 핸들링을 해도 괜찮기는 하지만 ui 상태 로직이 포함되어 있는만큼 역할을 더 명확히 하기 위해 별도 함수로 분리하는 것은 어떨까요?

pagerState는 Composable 내에서만 호출가능합니다! 어떤식으로 분리할 수 있을까요?

말씀해주신 것처럼pagerState.animateScrollToPage()가 Coposable 내에서만 호출 가능하더라도
이벤트 핸들러 자체를 @composable 함수로 분리하고 pagerState를 파라미터로 넘겨주는 방식이라면 분리 자체는 여전히 유효할 것 같아요.
예를 들어 HandleUiEvent처럼 Composable 함수로 추출해두면 Route의 책임을 좀 더 명확히 나눌 수 있을 것 같아서요!

아, 계산로직을 빼라는건줄 알았는데 이벤트 처리 로직 자체를 말한거군요. 이해했습니다!
Route의 책임이 무엇인가요?

제가 생각하는 Route의 책임은 ViewModel과 UI를 연결해주는 연결고리이자 조립자 역할이라고 생각합니다.
ViewModel에서 내려준 상태나 이벤트를 바인딩하고 이를 UI에 전달하는 것이 핵심 책임이라고 보는데요!

pagerState를 Route에서 바인딩하는 것 자체는 자연스러울 수 있지만 그 내부에서 상태를 직접 조작하는 로직까지 함께 포함되면
Route가 UI 구성뿐 아니라 동작 제어까지 맡게 되어 책임이 모호해질 수 있다고 생각했습니다!

그래서pagerState.animateScrollToPage() 같은 UI 상태 변화 로직은 Composable 함수 내에서 분리 처리하는 쪽이 구조상 더 명확할 것 같다는 생각을 했습니다!

오호 샐리의 생각을 알 수 있어서 좋네요
그럼 샐리는 onClick, scroll 등 람다를 통해 Compose의 state API들을 조작해주어야할 때 어디서 해주나요?
(뷰페이저, 무한스크롤 위치 등)

수정사항

흔히 XXXRoute로 네이밍하는 컴포저블의 책임에 대해 조금 다른 관점을 제안드립니다.
기존 XXXScreen 함수는 수 많은 UI 상태와 UI 컴포넌트가 강하게 결합되어 있었습니다.
이 때 발생하는 가장 큰 문제점은 Compose의 가장 큰 장점 중 하나인 프리뷰와 테스트의 어려움입니다.
내가 조립한 UI의 프리뷰, UI State 분기에 따른 테스트를 구성하기 위해선 UI 상태와 뷰의 강한 결합을 끊어줘야합니다.
결과적으로 공식문서에도 다루듯이 Stateless한 컴포저블과 Stateful한 컴포저블로 분리하는 패턴이 생기게 되었습니다.
이렇게 되면 Screen 함수는 어떠한 상태도 들고 있지 않으므로, 외부에서 상태를 주입함으로써 훨씬 testable한 구조를 이루게 됩니다(상태에 따른 프리뷰도 덤).

그럼 다시 돌아와서 Route의 역할은 무엇이냐고 했을 때, Route는 ViewModel에서 내려주는 상태를 바인딩하고 이를 UI에 전달하는 조립자이자, 해당 화면의 전역적인 상태를 관리하는 일종의 Stateholder라고 생각합니다.
하나의 컴포저블을 하나의 객체로 대치할 수 있다고 생각하는데요,
그렇다면 SignInRoute라는 객체가 UIState라는 '상태'를 가졌을 때, 해당 객체 내부에서 '상태'를 수정하는 행위는 전혀 어색하지 않습니다. 이는 마치 뷰 모델의 '_uiState, _name' 등의 상태를 'fun updateUiState(), updateName()' 함수 등을 통해 상태를 조작하는행위와도 같습니다.
해당 객체는 외부로부터 메시지를 받고, 메시지에 따라 스스로의 상태를 업데이트할지 말지 결정하며, 조작된 상태는 stateless한 Screen에게 온전히 전달됩니다.

저도 기존에 UI 상태 객체들을 생각없이 호이스팅해왔는데, 이런 리뷰를 통해서 Compose Component와 밀접한 UI 상태들을 해당 컴포저블 내부나 최소한의 공유 범위로 한정하는 것이 더 좋은 것 같습니다.

샐리덕에 저도 오랫동안 고민하고 답을 내릴 수 있었습니다! 감사함다

결론:

UI 컴포넌트와 밀접한 상태는 해당 컴포넌트와 모아두는 것이 더 좋은 것 같다.

이벤트 핸들링 및 UI 상태 조작을 Stateful한 Route에서 수행하는 것은 Route 책임이 맞다고 생각한다.

말씀하신 것처럼 Route를 하나의 Stateholder로 보고 UI 상태와 밀접한 로직을 함께 관리하는 것도 응집도 측면에서 매우 자연스럽다는 생각이 들었어요!
특히 ViewModel과의 유사성을 들어 설명해주신 부분이 인상 깊었습니다:)

Route가 해당 화면의 상태를 보유하고 조작하는 건 자연스럽다고 생각하지만
그 안에서 처리하는 이벤트 로직이 너무 길어지거나 여러 로직이 섞이기 시작하면 응집도가 떨어진다고 느껴서 함수로 분리하는 걸 제안드렸어요!

즉 '이벤트 핸들링을 분리하자'는 건 Route가 책임지면 안 된다는 의미보다는 Route의 가독성과 역할을 명확히 하기 위한 구조적 제안이었습니다!

좋은 인사이트 공유해주셔서 감사드리고 앞으로도 이런 부분은 상황에 맞게 유연하게 가져가면 좋을 것 같아요 :)

yeonjeen · 2025-05-02T00:53:35Z

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInViewModel.kt

+                while (true) {
+                    delay(ONBOARDING_TRANSITION_PERIOD)
+                    _uiEvent.send(UiEffect.ScrollToPage)


c: 저는 그동안 자동스크롤은 composable에서 처리하는 걸 많이 봤어서 뷰모델에 있는게 개인적으로 어색하게 느껴집니다! 혹시 뷰모델에서 구현하신 이유가 있으신가요?

해당 코드는 자동스크롤 코드가 아닙니다!
뷰모델 스코프의 코루틴을 생성하고, 해당 코루틴에서 일정주기에 맞게 외부로 이벤트 객체를 전파하는 코드입니다.
여전히 현재 코드도 SignInRoute composable 함수 내부에서 자동스크롤하고 있습니다

yeonjeen · 2025-05-02T00:54:25Z

.../signin/src/main/java/com/into/websoso/feature/signin/component/OnboardingHorizontalPager.kt

+    R.drawable.img_login_1,
+    R.drawable.img_login_2,
+    R.drawable.img_login_3,
+    R.drawable.img_login_4,


a: 그때 이거 임포트 한다고 하지 않으셨나요 ㅎㅎ

junseo511

작업 고생 많으셨습니다 👍

junseo511 · 2025-05-05T01:52:10Z

core/common/src/main/java/com/into/websoso/core/common/extensions/FlowExtensions.kt

+@Composable
+fun <T> Flow<T>.collectAsEventWithLifecycle(
+    lifecycle: Lifecycle = LocalLifecycleOwner.current.lifecycle,
+    minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
+    context: CoroutineContext = EmptyCoroutineContext,
+    onEvent: (T) -> Unit,
+) {


c: 해당 함수의 사용 목적과 방법을 도큐먼트 주석으로 나타내주면 좋을 것 같아요 👍

junseo511 · 2025-05-05T01:56:14Z

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt

+import com.into.websoso.feature.signin.component.SignInButtons
+
+@Composable
+fun SignInRoute(signInViewModel: SignInViewModel = hiltViewModel()) {


c: Route와 Screen을 한 파일에 두신 이유가 있을까요? Route는 각자의 Screen에서 관리하는 것보다 navigation 내부에 루트 모델을 두는게 추후 확장성 측면에서 나아보이네요 🤔 Route를 어떤 형태로 두고 필요한 인자를 넘길지 논의가 필요할 것도 같구요 ‼️

오.. 어째서 루트 모델을 모아두는게 확장성 측면에서 나아보이나요?
명칭만 Route로 볼뿐 사실상 Public한 SignInScreen함수로, 해당 스크린 상태와 이벤트를 전역적으로 관리합니다.
현재 구성된 Route를 한 파일로 모은다면, 해당 파일에서 N개의 뷰모델 주입과 수백개의 상태관리, 수십개의 모듈, 모델 참조를 가지게 됩니다.
Route를 어떻게 이해하고 계신지 궁금해요.
내비게이션에서 사용하는 Route를 생각하신거라면, 네이밍에서 온 혼동같습니다.

네이밍에 혼동이 있을듯하여 public SignInScreen, private SignInScreen 함수로 네이밍 통일했습니다.

junseo511 · 2025-05-05T01:57:29Z

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt

+        Spacer(modifier = Modifier.weight(weight = 1f))
+        OnboardingHorizontalPager(
+            pagerState = pagerState,
+            isScroll = isScroll,
+            onScrollChanged = onScrollChanged,
+        )
+        Spacer(modifier = Modifier.weight(weight = 1f))
+        OnboardingDotsIndicator(pagerState = pagerState)
+        Spacer(modifier = Modifier.height(height = 32.dp))
+        SignInButtons(onClick = { })
+        Spacer(modifier = Modifier.height(height = 24.dp))


c: Spacer 보다 padding이 권장되는 이유를 찾아보시면 좋을 것 같아요! Modifier를 적극 활용하는 방향이 좋아보여요 :)

Spacer보다 padding이 권장되는 출처가 어디인가요?
렌더링 속도, 노드 등록 등을 포함해 정말 padding이 성능적으로 Spacer보다 우월하다고 자신할 수 있나요? :)

Spacer보다 padding이 권장되는 출처가 어디인가요? 렌더링 속도, 노드 등록 등을 포함해 정말 padding이 성능적으로 Spacer보다 우월하다고 자신할 수 있나요? :)

저도 몇가지 문서를 한 번 찾아보았는데, 유의미한 성능에서의 차이는 거의 없다고 하네요 🤔
심지어 생성 순서에 따라 성능이 바뀌기도 하는 불규칙성까지 보이는,,,ㄷ

컴포넌트 사이 사이의 여백을 둘 때는 padding, 컴포넌트 최외곽쪽 마진을 둘 때는 Spacer를 사용하는게 좋은 것 같네요 👍

말씀해주신대로 각 컴포넌트의 역할은 또렷히 정해져 있습니다!
https://foso.github.io/Jetpack-Compose-Playground/foundation/layout/spacer/
특히나, Spacer와 같은 경우 한번 트리에 등록되고 나면 리컴포지션 될 일이 없습니다.
추가로 명시적인 역할을 돕기도하구요.
때에 따라선, padding 보다 렌더링 시간이 느리기도, 어떤 경우엔 효율적이기도 합니다.
padding은 고정값이며, Spacer는 화면의 비율에 맞춰지기도 합니다.
그치만 얘기해주신대로, 하위 컴포넌트에서 무분별하게 사용하면 문제가 될 수 있을 것 같습니다.
이 부분에 대해선 팀 내부에서 얘기나눠보면 좋을 것 같아요 b

junseo511 · 2025-05-05T01:58:44Z

...re/signin/src/main/java/com/into/websoso/feature/signin/component/OnboardingDotsIndicator.kt

+private data class DotState(
+    val width: Dp,
+    val color: Color,
+    val isSelected: Boolean,
+)
+
+private data class DotColors(
+    val selectedColor: Color = Color.White,
+    val unselectedColor: Color = Color.Black,
+)


a: 상태를 프라이빗하게 명시해준 것 좋은 것 같아요 👍

m6z1 and others added 12 commits April 27, 2025 17:44

build: 1.2.0 버전 업데이트

5ab13f7

Merge pull request #655 from Team-WSS/release/1.2.0

61aa29c

release: 1.2.0 버전 업데이트(to develop)

build: feature:signin 모듈 추가

4e63f32

feat: SignInButtons 구현

56000e5

feat: OnboardingHorizontalPager 구현

41ba2cf

feat: OnboardingDotsIndicator 구현

e5e5bcf

build: common 모듈 추가

d6104fc

feat: collectAsEventWithLifecycle 구현

d1ca5d3

feat: SignInViewModel 구현

b5fea0b

feat: SignInScreen 구현

d463eca

build: 앱 모듈 의존성 롤백

4f2e841

Merge branch 'develop' of https://github.com/Team-WSS/WSS-Android int…

c15c9b9

…o feat/660

s9hn added 🍯 [FEAT] 새로운 기능을 개발합니다. ♻️ [REFACTOR] 기존 코드를 리팩토링합니다. ⚖️ 재상 세훈 전지적 세훈 시점 🚧 [BUILD] 빌드 업무 수정, 패키지 매니저 수정합니다. labels May 1, 2025

s9hn self-assigned this May 1, 2025

github-actions bot requested review from junseo511, m6z1 and yeonjeen May 1, 2025 06:53

s9hn changed the title ~~Feat/660~~ refactor: 로그인 뷰 Compose 마이그레이션 및 모듈 분리 May 1, 2025

m6z1 approved these changes May 1, 2025

View reviewed changes

yeonjeen approved these changes May 2, 2025

View reviewed changes

s9hn added 4 commits May 2, 2025 17:55

build: common 패키지 이름 수정

e4703c2

refactor: 영역 임계값 상수화

99594b0

refactor: common 패키지 변경 및 onEvent suspend 키워드제거

3442c4d

refactor: pagerState 분리 및 scroll 이벤트 전달 로직 수정

4aa14ed

junseo511 reviewed May 5, 2025

View reviewed changes

junseo511 approved these changes May 5, 2025

View reviewed changes

docs: FlowExtensions collectAsEventWithLifecycle 주석 생성

d10693c

s9hn added 2 commits May 7, 2025 00:10

refactor: pager 이벤트 로직 롤백

ac6025f

refactor: SignInRoute -> SignInScreen 네이밍 수정

2496d5a

s9hn merged commit 4a2ab68 into feat/657 May 6, 2025

m6z1 deleted the feat/660 branch August 11, 2025 15:08

refactor: 로그인 뷰 Compose 마이그레이션 및 모듈 분리 #666

refactor: 로그인 뷰 Compose 마이그레이션 및 모듈 분리 #666

Uh oh!

Conversation

s9hn commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

Uh oh!

m6z1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeonjeen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junseo511 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

s9hn commented May 1, 2025 •

edited

Loading

s9hn May 2, 2025 •

edited

Loading

s9hn May 3, 2025 •

edited

Loading

s9hn May 2, 2025 •

edited

Loading

s9hn May 5, 2025 •

edited

Loading

s9hn May 5, 2025 •

edited

Loading