Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[강원대 안드로이드 주민철] 4주차 과제 스텝1 #43
base: joominchul
Are you sure you want to change the base?
[강원대 안드로이드 주민철] 4주차 과제 스텝1 #43
Changes from 9 commits
8ae483b
7c76c97
ba17eaf
4c2c111
c69b181
2613a7d
daefd35
4bc4784
c4ad1b7
91bb445
9a521a2
7e68e17
668d100
b7891d4
f224649
cd1e93e
988f859
244c9fd
505088c
66d2aac
5949ab8
6049de6
3ad3f78
556aef4
82b07ff
829170b
21f5470
17e03c3
1b75544
da92697
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
View를 lazy 초기화 하면 어떤 에러가 발생할 수 있을까요?
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.
아직 초기화 되지 않은 시점에서 사용이 되면 UninitializedPropertyAccessException이 발생할 수 있습니다. 또한 액티비티가 소멸된 후에도 뷰가 참조되어 메모리 누수가 발생할 수도 있습니다.
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.
리팩토링 완료 했습니다.
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.
현재 작성하신 Diff Callback이 어떤 역할을 하는지 설명 부탁드립니다.
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.
상태가 변경되었다를 어떻게 판단할 수 있을까요?
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.
areItemsTheSame와 areContentsTheSame 함수로 판단할 수 있는데, areItemsTheSame 함수는 아이템이 같은 주소에 할당이 되어 있는지를 판단하고, areContentsTheSame 함수는 그 내용이 같은지를 판단합니다.
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.
areItemsTheSame은 true인데 areContentsTheSame이 false인 경우도 있을까요?
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.
콜백을 묶지 마세요! 콜백 메서드가 여러개가 함께 실행되어야 한다 -> 비즈니스 로직입니다.
판단하는 로직을 View에 두지 마세요
documentCliced 라고 하는 인스턴스 변수를 계속 변경하고 계시는데 객체간 통신은 이런 방식으로 하는게 아닙니다 ㅜㅜ
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.
잘 이해가 가질 않습니다...ㅠ
다시 setOnClickListener에서 콜백을 호출하게 해야 할까요?
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.
Adapter 내에서 place를 클릭 하면 addWord 하고, sendDocumentInfo 하고 외부 객체의 인스턴스 변수인 documentClicked에 접근해 값을 설정해주는 이 일련의 동작들이 모두 비즈니스 로직이라는 말씀을 드리려고 했던거였어요.
비즈니스 로직이라 생각되는 부분들은 모두 ViewModel로 옮기셔야 합니다.
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.
DocumentAdapter 내에 있던 placeClicked 함수를 리팩토링하여 뷰모델 객체를 만들고, 뷰모델로 관련 로직을 옮겼습니다.
뷰모델 내에서는 callback의 onWordAdded와 onDocumentInfoSet 함수를 호출하게 하였으며, 기존 documentClicked 변수는 뷰모델 내 라이브 데이터로 변경하였습니다.
그로 인해 MapActivity에 새로이 라이브데이터 observe를 만들어 검색 결과가 클릭이 되면 라벨(마커)를 생성하고, 그렇지 않으면 제거하게끔 하였습니다.
DocumentAdapter에서 뷰모델 객체를 생성할 때
val viewModel = MainViewModel(MyApplication())
이렇게 하는 게 맞는지, 또한 뷰모델 내 placeClicked 함수에서 callback 함수들을 호출하는 것이 맞는지 검토 부탁드립니다.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.
네 사실 documentClicked는 UI 상태와 관련된 데이터잖아요. 그래서 View를 구성하기 위한 데이터를 관장하는 ViewModel로 그 로직을 옮기신 부분 잘 구현하셨습니다. 다만 아직 부족한 부분이 있는데요. 사용자와 상호작용 하는 부분과 UI State를 어떻게 View와 바인딩 해야 하는지 순서에 대해 이해하실 필요가 있습니다. 아래 설명드리는 순서대로 설계해보세요!
사용자가 RecyclerView 내에서 아이템을 클릭하는 상호작용을 했다고 가정했을 때
근데 구현하신걸 보면 클릭 이벤트를 Adapter에 추가해서 Adapter에서 ViewModel을 생성 후 ViewModel의 onPlaceClicked 호출 하고 ViewModel에서는 또 Activity에 구현된 Callback을 통해 View에 구현된 로직 실행하고 또 그 구현 로직들이 ViewModel을 호출하고 있는데 이러면 일원화가 안되는 문제가 있죠.
서면으로 답변 드리기 어려운 부분이 있으니 멘토링 신청해주시면 코드 같이 보면서 설명드리겠습니다