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

[Linus_Josh] 미션 1 ~ 4단계 구현 #25

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

[Linus_Josh] 미션 1 ~ 4단계 구현 #25

wants to merge 33 commits into from

Conversation

junseokseo9306
Copy link
Collaborator

@junseokseo9306 junseokseo9306 commented Mar 25, 2022

리사이클러 뷰 구현

  • 리사이클러 뷰를 통해서 사진 가져오기 기능 완성

퍼미션 뷰 구현

  • 디바이스에 저장된 이미지 파일에 접근할 수 있는 권한 요청
  • 사용자가 권한 요청 2번 거부시 처리 포함

리스트 어뎁터 구현

  • 리스터 어뎁터 및 DiffUtil 구현

이미지 불러오기 - 코루틴 구현

  • 앱 바에 버튼 추가한다.
  • 코틀린 코루틴을 통해서 이미지 다운로드

MVVM 적용

  • 데이터 바인딩 적용
  • 뷰 + 뷰모델 적용 (뷰 모델에서 데이터 불러오기 기능 추가 -> 향후 데이터 레이어에서 URL 이미지 불러오기 및 저장 기능으로 변경)

결과

4

4조 페어 프로그래밍 룰

cherrylime69 and others added 30 commits March 21, 2022 21:46
[Linus] RecyclerView 구현 완료
고생하셨습니다
[Linus_Josh] 리사이클러뷰 + 색깔 랜덤 기능 합치기
고생하셨습니다.
[미션 2_ 권한요청] 권한요청하기 위한 레이아웃 구현
[조시] 퍼미션 2번 거절 구현입니다.
- contentResolver를 통해 저장소에 저장된 이미지 id와 uri 값 가져오기 구현
[미션 3단계] AdaptList를 활용하여 이미지 로드 및 표시하기 완료
[조시] json 파일 수정 입니다.
[미션 4] 코루틴으로 이미지 추출해서 화면에 표시하기 구현 완료
- 권한 승인이 있으면 바로 Photo 화면으로 이동
- 권한을 1번 거절했다면 스낵바로 권한이 필요함을 알려줌
- 권한을 2번 이상 거절했다면 설정창으로 가도록 구현
…되는 기능 구현

- Dispatchers.IO는 네트워크 입출력을 할 때
- Dispatchers.Default는 데이터를 수정할 때
- Dispatchers.Main은 UI를 갱신할 때 사용함
[미션 4] permission UI 개선 및 코루틴으로 이미지 불러오기 구현
- DataBinding을 활용하여 DoodleAdapter 리팩토링
[MVVM 모델] MVVM 모델 적용을 위한 리팩토링
[조시] MVVM 모델 적용 및 리드미 업데이트
@junseokseo9306 junseokseo9306 added the review-android Further information is requested label Mar 25, 2022
@junseokseo9306 junseokseo9306 requested a review from ivybae March 25, 2022 08:47
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.

@junseokseo9306 @cherrylime69 수고하셨습니다~
코드리뷰 규칙도 정해서 해보신것도 너무 좋네요~
코멘트 내용도 확인해주세요~

private val _images = MutableLiveData<List<JsonImage>>()
val images: LiveData<List<JsonImage>> get() = _images

fun extractDataFromJson(adapter: DoodleAdapter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 이 메서드는 Activity에서 호출하는 대신에 ViewModel의 init 블럭에서 실행하는 건 어떨까요? getDownloadedImages에서 데이터 _images를 업데이트하고, Activity에서는 images를 통해 UI를 업데이트 하고 있는 것으로 보이는데요. 공개하지 않아도 된다면, 외부 객체에게는 감추는 방향으로 구현하는 게 어떨까 싶어서요~

Copy link
Collaborator

Choose a reason for hiding this comment

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

오, 그 생각을 못했습니다. 아마 DoodleAdapter를 DoodleAcitivity에서 생성하고 있어서 DoodleActivity에서 호출했던 것 같습니다. 다시 보니 리팩토링 과정 중에 getDownloadedImages 메서드가 더 이상 DoodleAdapter를 필요하지 않아 activity에서 호출하지 않아도 될 것 같습니다.

ActivityResultContracts.RequestPermission()
) { isGranted: Boolean ->
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
when {
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 좋습니다, 권한 flow의 모든 케이스를 구현하셨네요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

네 :D 감사합니다

import com.example.app.R
import com.google.android.material.snackbar.Snackbar

class PermissionActivity : AppCompatActivity() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 권한을 얻는 Activity를 따로 구현하는 방법은 약간 아쉬움이 남기도 합니다. 앨범에 저장되어 있는 사진을 보여주는 화면이 권한을 얻어오는 구현까지 담당하는 것이 어떨까 싶어서요. 지금과 같은 구현이라면 권한이 필요한 화면은 항상 권한을 처리하는 Activity를 한벌 더 구현하게 되는거죠?

Copy link
Collaborator

Choose a reason for hiding this comment

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

안 그래도 고민했었는데 확실히 하나의 액티비티에서 처리하는게 더 좋은 것 같습니다. 이에 대한 구현은 승인이 있다면 권한을 보여주는 레이아웃을 Gone 처리하는 식으로 하면 될 것 같은데 이번주에 여유가 있다면 시도해보겠습니다 ㅜ


class GalleryViewModel(application: Application) : AndroidViewModel(application) {

var mediaStoreImageList = mutableListOf<GalleryImage>()
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 외부에 공개하는 변수라면 immutable이 좋겠네요~

Copy link
Collaborator

@cherrylime69 cherrylime69 Mar 29, 2022

Choose a reason for hiding this comment

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

네! 수정하고 다음부터 외부에 공개되는지 등 한번 더 생각하도록 하겠습니다.


class GalleryActivity : AppCompatActivity() {

private var mediaStoreImageList = mutableListOf<GalleryImage>()
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 멤버변수를 추가하게 된 이유가 무엇일까요? galleryViewModel.mediaStoreImageList을 그대로 사용해도 될 것 같아서요~

Copy link
Collaborator

Choose a reason for hiding this comment

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

GalleryActivity의 다른 메서드에서 mediaStoreImageList를 많이 사용하여 멤버 변수로 사용하였는데, 리팩토링하면서 메서드를 adapter 등 다른 패키지 및 클래스에 옮겼는데 이 멤버 변수는 미처 수정하지 못한 것 같습니다. 수정하도록 하겠습니다.

mediaStoreImageList = galleryViewModel.mediaStoreImageList

val myRV = findViewById<RecyclerView>(R.id.recycler_view)
val myAdapter = GalleryAdapter(mediaStoreImageList)
Copy link
Contributor

Choose a reason for hiding this comment

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

@junseokseo9306 @cherrylime69 그리고 이 구현에서 mediaStoreImageList가 모두 구성되기 전에 GalleryAdapter로 전달되는 이슈는 없으셨나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

사진을 40장만 남겨두고 다시 실행해봤는데, 모두 정상적으로 표시되어 따로 이슈는 발견하지 못하였습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-android Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants