Skip to content

Conversation

qdrptd
Copy link
Collaborator

@qdrptd qdrptd commented Jul 19, 2025

No description provided.

@qdrptd qdrptd requested a review from a team as a code owner July 19, 2025 06:31
onNavigateOnboard: () -> Unit,
onNavigateDiaryWrite: (String) -> Unit,
) {
val courseBookDtoList = diaryListViewModel.courseBookDtoList
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 diaryListViewModel.courseBookDtoList 가 stateflow가 아니라 recompose 되나 모르겠네

Copy link
Collaborator

Choose a reason for hiding this comment

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

p3) DTO 라는 말은 여기서 등장하면 안됨
아직 도메인모델대신 CourseBookDto 를 쓰고 있어서 이름이 그렇게 된거겠지만..ㅋㅋ

Comment on lines 51 to 53
selectableCourseBookDto: List<Selectable<CourseBookDto>>?,
onClickCourseBook: (Int) -> Unit,
diaryListUiState: DiaryListUiState,
Copy link
Collaborator

@JuTaK97 JuTaK97 Jul 26, 2025

Choose a reason for hiding this comment

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

요거 selectableCourseBookDto는 uiState 에 통합 안 한 이유가 있어?
이게 항상 고민되는 부분이고 tradeoff가 있는 거긴 한데 (recomposition 비용 문제)

  • 전체 Screen의 모든 UI 요소를 나타내는 단일 UIState 데이터 클래스로 표현된 상태 주입
    vs
  • 명확히 상태 수정의 원인과 시점이 다른 Screen의 섹션이 있으면 별개 파라미터로 상태 주입

우리가 Route와 Screen을 어떻게 나눴나를 생각해 보면,

  • Route의 책임
    • Jetpack Navigation 과 관련된 부분 전부 담당
      • hilt viewmodel 생성
      • navigation이 필요한 사이드 이펙트 발생시키기 (그래서, Screen에서는 navigate 라는 이름은 없고 click 이라는 이름만 있고, Route 에서만 navigate 라는 말을 사용)
    • 상태 구독 및 주입 (viewModel의 존재를 아는건 Route뿐)
  • Screen의 책임
    • 실제 UI의 각 컴포넌트 구성 및 상태 전달
    • uiState 단일 객체에서 적절히 필드를 뽑고 계산해서 하위 컴포저블에게 전달
    • ui 이벤트 발생시키기

이를 위해서라도, UIState 데이터 클래스가 조금은 비대해지더라도 하나로 전부 가져가는 게 낫다고생각해
왜냐면 Route에서 Screen을 호출할 때 selectableCourseBookDto도 알고 diaryListUiState도 알아야 한다면, Route는 이미 UI의 세부적인 걸 알고있는 거니까. (이건 Screen만 알아야함)

Copy link
Collaborator

@JuTaK97 JuTaK97 Jul 26, 2025

Choose a reason for hiding this comment

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

거대한 단일 UIState 데이터클래스를 사용함으로써 발생하는 recomposition 비용은, skipping 이 잘 동작하리라 믿고(?) 가야하지 않을까 ㅋㅋ 우리앱의 상태가 그정도로 복잡하지 않기도 하고...

만약 "이건 좀 아닌데.." 싶을 정도로 출처와 변경의 시점과 원인이 다른 두 state가 하나의 UIState 데이터 클래스의 필드로 들어가야 한다면 정말 고민이 필요할 것 같은데, 우리가 정한 Screen 단위에서 그정도의 이질적인 state가 있을 것 같지 않기도 하고.

private val displayMessageResolver: DisplayMessageResolver,
) : ViewModel() {

var courseBookDtoList: List<CourseBookDto>? = null
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기는 null 을 초기값으로 가지는 게 아니라, UIState.Loading 을 적극 활용해보자

Copy link
Collaborator

Choose a reason for hiding this comment

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

스켈레톤 UI 효과 누가 만들어줬으면 좋겠다~ 현도가 만들어주나?

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.

에이 제가 하면 되죠

@JuTaK97 JuTaK97 merged commit 37bc7d9 into develop Sep 6, 2025
3 checks passed
@JuTaK97 JuTaK97 deleted the qdrptd/diary-list-screen branch September 6, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants