-
Notifications
You must be signed in to change notification settings - Fork 1
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
[refactor] 이름 및 패키지 정리, Type-Safe Navigation 적용 #168
base: develop
Are you sure you want to change the base?
Conversation
picklist -> favorite/mypick 으로 패키지 분리
안드로이드 라이브러리 의존성 삭제
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아주 크나큰 공사를 하셨군요,, 고생하셨습니다!👍👍
저는 실행했을 때 안 되는 부분은 없는 것 같네요
기존에 PickListScreen에서 관리하던 것을 FavoriteScreen과 MyPickScreen으로 분리하고 PickListScreen은 사용하지 않는 것으로 변경하신 거죠? 두 스크린의 공통된 부분을 common>picklist로 옮기신 것 같은데 그 과정에서 편집 모드가 날아간 것 같더라구요ㅜ 이 부분도 추가해주실 수 있나용
이후에 이 부분이 완전히 정리되면 picklist 패키지는 삭제해도 될 것 같습니다
기존에는 등록한 픽과 픽 보관함을 하나의 Screen으로 관리했기에 뷰모델이 PickListViewModel 하나였는데 두 개의 Screen으로 분리되며 FavoriteViewModel과 MyPickListViewModel로 나뉘었기에 편집 모드를 반영하려고 한다면 PickListViewModel 안의 내용을 두 개의 뷰모델에 모두 넣어줘야 합니다. 그런데 반영해야 하는 함수들 중에서는 중복되는 것들이 많을 거라 생각합니다.
이미 두 뷰모델에서 중복되어 사용되는 함수는 common>picklist의 Extension에 빼두셨더라구요!👍 이렇게 중복되어 사용되는 함수를 모두 빼두는 것도 좋은 방법이라 생각하지만 혹시 공통 로직을 관리하는 부모 뷰모델을 만들어보는 건 어떤가요?
@HiltViewModel
class FavoriteListViewModel @Inject constructor(
// ...
) : ViewModel() {
private var favoriteList: List<Pick> = emptyList()
private fun setList(order: Order) {
_pickListUiState.value = favoriteList.setOrderedList(order)
}
}
@HiltViewModel
class MyPickListViewModel @Inject constructor(
// ...
) : ViewModel() {
private var myPickList: List<Pick> = emptyList()
private fun setList(order: Order) {
_pickListUiState.value = myPickList.setOrderedList(order)
}
}
현재 뷰모델에도 공통된 부분이 있는 것 같은데 편집 모드 부분을 옮기게 된다면 이렇게 중복된 코드가 더 많아질 것입니다. 그래서 리스트를 부모 뷰모델로 옮겨서 공통 부분을 관리하는 부모 뷰모델을 만드는 건 어떤지 제안해봅니당
@Serializable | ||
data object Setting : ProfileRoute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProfileRoute의 세팅이라고 보면 Setting이라는 이름도 나쁘지 않아 보이는데 처음에 Setting을 봤을 때 어디로 가는 루트지? 라는 생각을 잠시 했었습니다.
혹시 ProfileRoute를 MyPageRoute 요런 걸로 변경하고 Setting을 EditProfile이나 Profile로 변경하는 건 어떤 것 같나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Favroite과 MyPicks 분리를 머지 전에 진행해서 꼬여버렸네요 하핳
말씀하신대로 부모 뷰모델을 만드는게 좋을 것 같아요! 반영하겠습니다.
이름도 확실히 실제 화면의 기능과 상이한 것 같네요! 수정하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.picklist 패키지에 있는 PickListViewModel이 상위 뷰모델 역할을 하도록 수정하였습니다.
추상 클래스로 구현 후 FavoriteListViewModel과 MyPicksViewModel에서 상속받아 구현하고 있습니다.
모든 메소드는 PickListViewModel 에서 정의될 수 있도록 했고
그 과정에서 favorite과 mypicks에서 사용되는 유스케이스 몇개를 추상화 시켰어요
유저페이지에서 파생되는 화면들과 루트 이름 수정했습니다
프로필사진과 메뉴가 나열된 화면은 Profile -> UserInfo
이름 설정은 EdifProfile
알림 설정도 EditNotificationSetting 으로 변경했습니다
(자꾸 멋대로 이름 막 바꾸는 것 같네요 앞으로 상의하고 하겠습니다 ㅠ)
확인부탁드려요!
픽상세화면에서 뒤로 가기 시 무조건 지도로 돌아가더라구요 고칠 예정입니다! |
화면 별 기능이 더 직관적으로 드러나도록 변경
화면 별 기능이 더 직관적으로 드러나도록 변경
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존의 네비게이션보다 경로가 더 직관적이고, 파일도 분리해주셔서 좋네요!
type-safe한 인자 전달 넘 좋습니다. 대대적인 수정하시느라 정말 고생하셨습니다! 😄👍👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 저도 common/picklist와 picklist패키지에 동일한 파일이 들어가있는 것을 보았는데 민주님이 말씀해주신 대로 하나로 정리되면 삭제해도 될 것 같아요.
이때 개인적으로 common내부에 너무 많은 파일을 담기보다는 picklist패키지에 남겨두는 것이 좋을 것 같습니다!
private val searchSongsUseCase: SearchSongsUseCase, | ||
savedStateHandle: SavedStateHandle, | ||
getLastLocationUseCase: GetLastLocationUseCase, | ||
private val fetchSongsUseCase: FetchSongsUseCase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchSongsUseCase는 현재 사용되지 않는 코드인 것 같습니다. 제거해도 될 것 같아요!
) { | ||
val song = createPickViewModel.selectedSong ?: DEFAULT_SONG | ||
// val song = createPickViewModel.selectedSong ?: DEFAULT_SONG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는다면 지워지지 않은 주석이 있습니닷!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 파일 detail 패키지로 수정 필요합니다.
PickDetailScreen( | ||
pickId = pickId, | ||
playerServiceViewModel = playerServiceViewModel, | ||
onProfileClick = onProfileClick, | ||
onBackClick = onBackClick, | ||
onDeleted = onDeleted, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
navigatePickDetail
이 MapNavigation
에 존재해서 프로필 화면이나 내가 담은 픽 화면에서 PickDetail화면으로 이동 후 뒤로가기를 누르면 모두 지도 화면으로 이동합니다.
다른 화면들의 onBackClick은 popBackStackIfNotMap
로 되어있는 것과 달리 mapNavGraph
를 통해 onBackClick = navigator::navigateMap
로 되어있어서 발생하는 문제인 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miller198 한시간 넘게 리뷰하느라 그동안 코멘트 추가하신걸 못봤네요..!!! 😅😄
#️⃣연관된 이슈
📝작업 내용 및 코드
아직 미완성입니다. 에러 해결중변경된 내비게이션에 대한 내용 -> 여기
💬리뷰 요구사항(선택)
드디어 기어나온 PR
같이 에러 해결 해주세요
앱이 시작되면 MainViewModel에서 이 유스케이스가 실행되면서
DataStore에 있는 UserId 를 가져옵니다.
만약 DataStore에 저장된 UserId가 없다면 (앱 최초실행) MainViewModel 의
에서 createUser()를 실행해서 파베에 유저를 추가하고
저장된 UserIde가 있다면 fetchUser()를 해서 파베에서 유저정보를 받아와서
LocalDataSource의 _currentUser 변수에 저장합니다.
그런데 지금 fetchUser() 분기에서 _currentUser에 저장하는 게 안되고 있어서
위의 사진과 같은 에러 로그가 나와요
최초실행해서 createUser()할때는 또 잘되서 앱이 잘 실행되요ㅡㅜ 왜그럴까욧
250123 추가
앱 데이터 지우고 들어갔을때 화면 이동은 잘 됩니다
검색 -> 등록 화면 넘어가는거만 빼구요
BottomNavigation 코드를 위와 같이 바꾸었는데
여기서 원래 userId가 없었거든요 이게 생겨서 나타나는 문제같습니다 더 알아볼게요
250124
위에 쓴 내용이 원인 맞네요
해당 부분 수정하고 잘 실행 됩니다.
이제 내비게이션 동작이 모두 완벽히 되도록 진행중입니다
현재 Search -> Create 로 넘어갈때 새로 노래 정보를 불러오는게 아니라 Song 인스턴스를 전달하는 방식으로 하고 있는데
이게 잘 안되네요
250125
내비게이션 모두 수정 완료 했습니다.
실행 해보시고 안되는 부분 있으면 알려주세요