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

[MOD/#266] 추천, 검색, 프로필, 결제, 타임라인 / StateFlow로 변경 #294

Merged
merged 41 commits into from
Oct 12, 2023

Conversation

Marchbreeze
Copy link
Member

@Marchbreeze Marchbreeze commented Oct 9, 2023

⛳️ Work Description

  • 추천친구 stateFlow로 변경
  • 검색 stateFlow로 변경
  • 프로필 stateFlow로 변경
  • 결제 stateFlow로 변경
  • 타임라인 stateFlow로 변경
  • 타임라인 UiState 대신 LoadState 활용
  • 타임라인 PagingSource와 Repository 분리
  • 검색 뷰와 추천친구 뷰 데이터 분리 적용
  • 검색 Paging3 적용 -> keyword 별로 대응을 못하겟서요 ...
  • 온보딩 친구추가 Paging3 적용 -> 민주꺼 머지 받으면 차차 해볼게요 ...
  • 프로필 뷰 유저 정보 객체화
  • 프로필 뷰 선택된 유저 정보 객체화
  • fadeIn 시점 수정

📢 To Reviewers

  • 리팩토링 부분보다, Flow 관련 뷰모델 & 옵저버 부분쪽 자세히 봐주시고 수정 부탁드립니다 ㅜ

@Marchbreeze Marchbreeze added MOD ✅ 코드 수정 및 내부 파일 수정 PULL REQUEST 🚀 pull reqeust 날리기 상호 🍀 Sangho's Task labels Oct 9, 2023
@Marchbreeze Marchbreeze added this to the 코드 리팩토링 milestone Oct 9, 2023
@Marchbreeze Marchbreeze self-assigned this Oct 9, 2023
@Marchbreeze Marchbreeze requested a review from a team as a code owner October 9, 2023 11:13
Copy link
Member

@b1urrrr b1urrrr 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 +28 to +29
fun getLookListWithPaging(): Flow<PagingData<LookModel>> {
return lookRepository.getLookList()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun getLookListWithPaging(): Flow<PagingData<LookModel>> {
return lookRepository.getLookList()
fun getLookListWithPaging(): Flow<PagingData<LookModel>> = lookRepository.getLookList()

일케도 쓸 수 잇어요

Copy link
Member Author

Choose a reason for hiding this comment

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

성능 상 다른 건 없고 문법의 차이만 있나요 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

코틀린스러운 문법이죠

"click_go_shop",
JSONObject().put("shop_button", "cta_main"),
)
setClickGoShopAmplitude("cta_main")
Copy link
Member

Choose a reason for hiding this comment

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

시간 나면 상수화도 부탁드립니다!!ㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

앰플리튜드도 상수화 다하셨...나..요...?

Copy link
Contributor

Choose a reason for hiding this comment

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

원래라면 하긴합니둥

@@ -35,26 +33,29 @@ class ProfileViewModel @Inject constructor(
private val payRepository: PayRepository
) : ViewModel() {

private val _getState = MutableLiveData<UiState<ProfileUserModel>>()
val getState: LiveData<UiState<ProfileUserModel>> = _getState
private val _getUserDataState = MutableStateFlow<UiState<ProfileUserModel>>(UiState.Empty)
Copy link
Member

Choose a reason for hiding this comment

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

UiState 초기값으로는 Empty보다 Loading이 더 적합하지 않을까여?

Copy link
Member Author

Choose a reason for hiding this comment

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

Loading일 때 shimmer나 lottie를 적용해둔 부분이 가끔씩 있어서 ... 중복 실행 방지를 위해 아무런 로직 설정이 없는 empty로 해뒀습니닷

Comment on lines +156 to +158
lifecycleScope.launch {
viewModel.getUserDataState.collectLatest { state ->
when (state) {
Copy link
Member

Choose a reason for hiding this comment

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

굿굿티비!

&& layoutManager is LinearLayoutManager
&& layoutManager.findLastVisibleItemPosition() == adapter.itemCount - 1
) {
if (!binding.rvProfileFriendsList.canScrollVertically(1) && layoutManager is LinearLayoutManager && layoutManager.findLastVisibleItemPosition() == adapter.itemCount - 1) {
Copy link
Member

Choose a reason for hiding this comment

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

복잡해보이는데 함수화를 하는 것은 어떨까여?

Copy link
Member Author

Choose a reason for hiding this comment

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

호오...

Comment on lines +176 to +182
is UiState.Failure -> {
showShimmerScreen()
yelloSnackbar(
requireView(),
getString(R.string.recommend_error_school_friend_connection),
)
}
Copy link
Member

Choose a reason for hiding this comment

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

StateFlow는 중복된 값을 처리하지 않는다고 이해했는데 그러면 두 번 연속 친구 불러오기에 실패해도 초기 한 번만 처리되지 않을까요...?

Copy link
Member Author

Choose a reason for hiding this comment

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

오....작동은 하던데... UiState에 담긴 모델값이 달라서 다른 값으로 인식을 했던 걸까요..?
@kkk5474096 도와줘욧
아니면 이런 경우에 flowWithLifecycle로 onEach 사용하면 되는건가..?

Copy link
Contributor

Choose a reason for hiding this comment

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

이게 실패했을때 스와이프로 재시도했을경우 인가요?
그렇다면 다시 서버통신할때 UiState를 loading 상태로 변경하고 값이 떨어진다면 success or failure로 갈테니 해결할 수 있습니둥

Comment on lines +42 to +45
companion object {
const val LOOK_PAGE_SIZE = 10
const val LOOK_POSITION_TO_PAGE = 0.1
}
Copy link
Member

Choose a reason for hiding this comment

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

가독성 미쳤따이

Copy link
Member Author

Choose a reason for hiding this comment

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

아좌잣~

Copy link
Contributor

@minju1459 minju1459 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 !

setPullToScrollColor(R.color.grayscales_500, R.color.grayscales_700)
}
lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.CREATED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

페이징은 백그라운드에서 실행되지 않으므로 repeatOnLifecycle가 사용된 것 맞나요??

Copy link
Member Author

Choose a reason for hiding this comment

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

페이징은 백그라운드 쓰레드에서 실행이 됩니닷 ! 다만 이부분은 pull-to-refresh를 설정하는 과정에서 launchWhenCreated의 용도로 사용했습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

Lifecycle.StateSTARTED 가 아닌 CREATED로 한 이유가 있을까요? 이유가 궁금해서

private val _getState = MutableLiveData<UiState<ProfileUserModel>>()
val getState: LiveData<UiState<ProfileUserModel>> = _getState
private val _getUserDataState = MutableStateFlow<UiState<ProfileUserModel>>(UiState.Empty)
val getUserDataState: StateFlow<UiState<ProfileUserModel>> = _getUserDataState.asStateFlow()
Copy link
Contributor

Choose a reason for hiding this comment

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

asStateFlow()를 쓰지 않아두 상관은 없다고 배웠는데 혹시 쓰신 이유가 따로 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

아뇨..! ㅋㅋㅋㅋㅋㅋ 일단 쓰다보니깐 그냥 쓰고 있었네여 ..

private val _getPurchaseInfoState = MutableLiveData<UiState<ResponsePurchaseInfoModel?>>()
val getPurchaseInfoState: LiveData<UiState<ResponsePurchaseInfoModel?>> = _getPurchaseInfoState
private val _getPurchaseInfoState = MutableLiveData<UiState<PayInfoModel?>>()
val getPurchaseInfoState: LiveData<UiState<PayInfoModel?>> = _getPurchaseInfoState
Copy link
Contributor

Choose a reason for hiding this comment

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

entity가독성이 좋네요 ,,!

Copy link
Contributor

@kkk5474096 kkk5474096 left a comment

Choose a reason for hiding this comment

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

flow 생명주기 알도록 변경해주세용

setPullToScrollColor(R.color.grayscales_500, R.color.grayscales_700)
}
lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.CREATED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lifecycle.StateSTARTED 가 아닌 CREATED로 한 이유가 있을까요? 이유가 궁금해서

adapter.addItemList(friendsList)
}
lifecycleScope.launch {
viewModel.getFriendListState.collectLatest { state ->
Copy link
Contributor

Choose a reason for hiding this comment

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

flow는 뷰의 생명주기를 알지 못합니다
그래서 전에는 repeatOnLifeCycle 에서 started를 사용해서 구현했었는데
flowWithLifeCycle이라는 확장함수가 위에 내용을 담고 있어서 flowWithLifeCycle을 사용하고 있습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

추가로 LiveData는 Android에서 나온 라이브러리로 뷰의 생명주기를 알고 있어서 위와 같은 작업이 필요 없었죠

viewModel.myTotalFriends.value =
viewModel.myTotalFriends.value?.toInt()?.minus(1).toString()
lifecycleScope.launch {
viewModel.deleteFriendState.collectLatest { state ->
Copy link
Contributor

Choose a reason for hiding this comment

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

위 내용과 동일합니다
lifeCycle이 종료되어도 계속 수집해서 메모리 누수가 있을수 있어용

@Marchbreeze Marchbreeze merged commit 2a4425c into develop Oct 12, 2023
1 check passed
@b1urrrr b1urrrr deleted the mod/#266-flow-paging3 branch October 19, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MOD ✅ 코드 수정 및 내부 파일 수정 PULL REQUEST 🚀 pull reqeust 날리기 상호 🍀 Sangho's Task
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[MOD] 추천, 검색, 프로필, 결제, 타임라인 / StateFlow로 변경
4 participants