Skip to content

[산군] 뷰 챌린지 미션 2단계 제출합니다.#48

Merged
chws0508 merged 24 commits into
woowacourse:s9hnfrom
s9hn:step2
Oct 11, 2023
Merged

[산군] 뷰 챌린지 미션 2단계 제출합니다.#48
chws0508 merged 24 commits into
woowacourse:s9hnfrom
s9hn:step2

Conversation

@s9hn

@s9hn s9hn commented Oct 10, 2023

Copy link
Copy Markdown

안녕하세요 스캇 ! 생각보다 좀 막혀서 늦어졌네요 ㅎㅎ..
변경사항이 좀 많습니다! 빠르게 리뷰하기엔 힘드실 것 같아서 가능한 빨리 천천히 해주세요bb

기능 요구 사항

그림판 앱에 기능을 추가한다.

  • 화면에 사각형, 원 모양을 그릴 수 있도록 구현한다.
    • 사용자가 자유롭게 크기나 위치를 조정할 수 있다.
  • 특정 영역의 요소를 지울 수 있는 지우개를 구현한다.
  • 브러쉬(펜, 사각형, 원, 지우개) 종류를 자유롭게 선택해서 그릴 수 있다.

선택 요구 사항

  • 전체 그림을 지우는 기능을 추가한다.
  • 취소(undo), 재실행(redo)을 구현한다.
image

위와 같은 전략패턴 구조로 되어있습니다!

각 상태값들은 뷰모델에서 관리하고 있습니다.

  • 조금 개선이 필요한 부분은 eraser와 같은 사용하지않는 함수들이 몇몇있는데, 어떻게 해결해야할지 방법을 모르겠군요..
    • 냅둬도 괜찮을까요?
    • Painter만 받기엔,, 지우개는 Tools..여서 고민이네요
  • 데이터바인딩은 일부러 사용하지 않았습니다!

얼마남지않았네요 남은기간 화이팅합시다!!

ui는 못생겨도 양해바랍니다.

@s9hn s9hn changed the title [산군] [산군] 뷰 챌린지 미션 2단계 제출합니다. Oct 10, 2023

@chws0508 chws0508 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

안녕하세요 산군!
선택미션까지 모두 다하시다니 대단하십니다...!
전략패턴을 사용한 점과, 추상화를 잘 사용하신 것 같아서 인상깊었습니다!
산군의 요청대로, step 3에서 더 자세한 리뷰를 드리겠습니다.
step 3를 구현하실 때, 고민할만한 사항들을 남겼으니 확인 부탁드려요!
(+현재 취소 구현에 문제가 있는 것 같아요 ㅠㅠ)

Comment thread app/src/main/java/woowacourse/paint/MainViewModel.kt
Comment on lines +27 to +34
private var tool: Tool = NORMAL_PEN
private var paintingBackup: MutableList<List<Line>> = mutableListOf()

private val _painting: MutableLiveData<List<Line>> = MutableLiveData(listOf())
val painting: LiveData<List<Line>> get() = _painting

private val _tools: MutableLiveData<Tools> = MutableLiveData(setNormalPen())
val tools: LiveData<Tools> get() = _tools

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

구성 변경에 대응하기 위해서, Tool 과 라인을 가지고 있는 것일 까요?? 개인적으로 Tool 과 Line 을 가지고있는 주체는 PaintingView 가 되야하지 않을까 하는 생각이 드네요. 어떻게 생각하시나요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

저도 처음엔 drawingPaper(paintingView)에 painting과 tools을 두었는데요

  1. 현구조의 각 tools의 구현체에서 drawingPaper에 변경사항을 알리기가 어려웠다.
  2. 내가 설계한 drawingPaper에서 drawingPaper는 단순히 레이아웃에 존재하는 위젯에 불과한데, 위젯이 모든 값을 가지고 있는게 맞을까?
  3. 단순히 하나의 위젯으로 본다면, 해당 위젯에 들어갈 상태값들은 뷰모델에서 관리하는게 맞지않나? (+ 구성변경도 대응해야하네)

라는 플로우로 뷰모델을 두고, 상태값들을 뷰모델로 옮기게 되었습니다!

미션을 진행하면서 customView는 그저 우리가 만드는 하나의 위젯에 불과하며, 가능한 가볍게 작성하고 싶단 생각이 들었는데 스캇은 어떻게 생각하시나요

@chws0508 chws0508 Oct 16, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • 음 이건 개인적으로 저는 CustomView는 다른 View 들과 마찬가지로, CustomView 자체가 상태를 가지고 외부에서는 CustomView의 API를 활용하는 느낌으로 사용해야 한다고 생각해요. 이렇게 해야 어떤 뷰에서든 재활용이 가능하다고 생각합니다. 지금 현 상태에서는 ViewModel 이 DrawPager라는 CustomView의 내부 구현까지 제어하고 있다는? 느낌이 드네요.

  • 음, 쉽게 저는 DrawPager를 EditText 느낌으로 생각했어요. EditText를 사용여 텍스트를 입력할 때, 우리가 ViewModel 에서 값을 변경한 후에 EditText에 텍스트를 넣어서 업데이트 하지 않고, EditText에서 텍스트를 입력하면, EditText 자체가 그 텍스트의 변경을 감지해서 상태를 업데이트 하는 것처럼 말이죠.

  • 추가로,저도 미션하다가 배웠는데, EditText는 ViewModel 없이도 구성변경에 대처하도록 설계가 되어 있습니다. EditText 자체에서 onRestore 어쩌구 함수에서 복구를 하고있더라구요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

오 editText에 관한건 처음알았네요
추가로 스캇의 색다른견해도 이해가 가네요! 좋은 의견 감사합니다!

Comment on lines +116 to +121
val Factory: ViewModelProvider.Factory = viewModelFactory {
initializer {
MainViewModel()
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

인자가 없는 ViewModel 에 대해서는 Factory를 만들 필요 없을 것 같은데, 특별히 만드신 이유가 있을까요??

@s9hn s9hn Oct 11, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

해당방식의 Factory 사용은 인자 유무를 떠나 객체의 역할 및 책임도 관련있다 생각합니다!
우리가 Activity간 Intent를 교환할 때, 자신의 Intent를 넘겨주는 것, 리사이클러뷰 뷰홀더에선 자신의 바인딩 혹은 뷰홀더를 건네주는것처럼요!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

개인적으로 저는 ViewModel 을 생성할 때, Factory 인자를 전달하지 않으면, 기본 Factory 즉 인자가 없는 ViewModel을 생성하구나 라는 것을 알고있는 상태이고, 팀적으로 모두 학습이 된 상태라면, 굳이 만들 필요가 있을까? 라는 생각이 드네용

산군이 제시한 예시에서는 모두 코드 작성이 무조건 필요한 상태이지만, 이 경우에는 작성할 필요가 없다고 생각되서요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ㅋㅋ뭐 사실 별 생각없이 습관처럼 적던 코드였음다! b

Comment thread app/src/main/java/woowacourse/paint/MainViewModel.kt

import woowacourse.paint.paintBoard.Line

interface Tools : Painter {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

복수형 s 를 붙이신 이유가 있을까요??

@s9hn s9hn Oct 11, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

도구들의 추상화라 어떤 도구일지몰라 Tools로 했는데, 생각해보니 Tool이란 이름 자체가 추상화긴하네요..
Enum class의 Tool과도 이름이 겹칠 수 있을 것 같은데 추천해주실만한 네이밍이 있으십니까..

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Enum을 ToolType으로 하고, Tool을 사용할 것 같아요!

Comment thread app/src/main/java/woowacourse/paint/paintBoard/tools/Tools.kt
}

override fun startPainting(pointX: Float, pointY: Float) {
onSave(line)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

그림을 시작할 때, save 해주는이유가 있을까요??

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

아 이것도 질문하는걸 깜빡했네요
finishPainting에서 save를 해주게되면,
첫번째 그림은 손가락 포인터? 커서?를 따라오지않고 손을 떼면 그제서야 그림이 생기고, 그다음부터는 잘 그려지더라구요.

그래서 누른시점부터 저장을시켜주니까 문제없이 잘 작동해서 저렇게해놨는데, 개선할 수 있을까요?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

제가 구현한 구조와 달라서 제가 헷갈린 것 같습니다... 산군의 구조에서는 손가락을 누른 시점부터 저장하는게 맞을 것 같아요!

Comment thread app/src/main/java/woowacourse/paint/paintBoard/tools/PathEraser.kt
Comment thread app/src/main/java/woowacourse/paint/paintBoard/DrawingPaper.kt
@chws0508 chws0508 merged commit 0a25d44 into woowacourse:s9hn Oct 11, 2023
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.

2 participants