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

[ Team2 (Jay&Stitch) ] recyclerview 클릭 이벤트 + MVVM 패턴 적용 #18

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

taewooyo
Copy link
Collaborator

@taewooyo taewooyo commented Mar 24, 2022

  1. 리사이클러뷰 아이템 클릭
  • 체크 박스 클릭 후 스크롤 할 시 다시 확인해보면 해제 되는 현상 발생
  • photoCheckBox.setOnCheckedChangeListener(null) 로 해결
  1. 이미지 선택 경우 다시 이미지를 그려주기 위해 adapter에게 알림
  • 선택을 위해 LongClickListener 이벤트 설정
  • 체크 박스 클릭 뿐 아니라 imageview 클릭 시 체크박스 선택을 위해 onClickListener 이벤트 설정
  • notifyDataSetChanged() 호출
  1. MVVM 패턴 적용
  • view, model, viewmodel로 관심사 분리
  • 추후 계획 : 데이터바인딩 적용

resolved #19
resolved #21

Question

  • 선택 모드를 들어가서 다시 뷰를 그릴 때, submitList를 쓰게 되면,

    현재 보고 있는 화면은 체크박스가 보이지 않는데 스크롤 내렸다가 올리면 체크박스가 보입니다.

    하지만, notifyDataSetChanged를 쓰면 바로 모든 것이 갱신됩니다. Photo 값의 mode 값이 바뀌는데 바로 바뀌지 않는 것이 궁금합니다.

Copy link
Contributor

@ivybae ivybae left a comment

Choose a reason for hiding this comment

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

@bn-tw2020 @shim506 수고하셨습니다!
질문해주신 내용은 피드백 드린 내용을 참고하셔서 구현을 다시 수정해보시면 좋겠습니다~

val photo = getItem(position)
holder.bind(getItem(position))

holder.itemView.setOnLongClickListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bn-tw2020 @shim506 이 코드는 holder의 itemView를 꺼내서 Listener를 할당하고 있네요~
bind 메서드 내부로 이동시키는 편이 낫지 않을까요? 어떻게 생각하세요? 바로 아래 코드도 마찬가지입니다.

Copy link
Collaborator

@shim506 shim506 Mar 25, 2022

Choose a reason for hiding this comment

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

다음 PR 에 수정내용 반영해서 보내겠습니다.

}
holder.itemView.setOnClickListener {
if (photo.mode == EDIT) {
currentList[position].isChecked = true
Copy link
Contributor

@ivybae ivybae Mar 24, 2022

Choose a reason for hiding this comment

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

@bn-tw2020 @shim506 Edit모드를 구현한 방식이 수정이 필요해 보입니다.

  1. 작성하신 코드의 흐름을 정리해보면 Long Click -> List 모든 원소의 mode 값 변경, false를 반환해서 이벤트를 소비하지 않고 -> ClickListener에서 EDIT 모드일 때 isChecked를 바꾸도록 처리한 부분은 이해하기가 너무 어려워 보여요. 단순한 기능인데도요.
  • Long Click이 발생했을 때 EDIT으로 변경하고 isChecked 까지 변경하는 것은 어려울까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. ListAdapter를 상속받았는데도 notifyDataSetChanged()를 호출하게 되는 점은 아쉽습니다.
    이부분은 1번 내용을 수정하고 개선하실 수 있다고 생각합니다.
    그리고 saveListener를 전달하신 것처럼 clickListener 혹은 longClickListener 구현을 생성자로 전달하는 방식도 생각해볼 수 있다고 생각합니다. 다른 구현방식도 있을테니 한번 고민해보시면 좋겠습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

1-1 . 위와 같이 구현한 이유는

  • 아이템 롱클릭을 통해 EDIT 모드로 전체 아이템이 선택할 수 있는 UI로 변할 수 있게 햇고
  • 체크박스 영역에서의 일반 클릭(숏클릭)뿐 아니라 이미지 영역 위에서의 일반 클릭 통해 해당 아이템을 선택할 수 있게했습니다.
    롱클릭안에 모든 구현을 넣을 경우 일반 클릭(숏클릭)으로는 해당 이미지를 선택할 수 없게되기에 다음과 같이 구현했습니다.
    @step4me

Copy link
Contributor

Choose a reason for hiding this comment

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

@shim506 왜 이렇게 구현하신지는 알죠~ 제가 수정 의견을 드렸던 이유는 이벤트는 LongClick으로 발생했는데 데이터 변경 처리를 바로하지 않고 다른 이벤트 처리기로 이동시키는 부분이 아쉬웠기 때문입니다~

@taewooyo taewooyo merged commit b5e46b4 into codesquad-members-2022:develop Mar 25, 2022
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.

- MVVM 패턴으로 전환 - 리사이클러 뷰 클릭 이벤트
3 participants