Feature/#839 행사 카카오 공유 기능 #888

tmdgh1592 · 2023-12-19T04:13:09Z

#️⃣ 연관된 이슈

close : #839

📝 작업 내용

카카오톡 공유를 통해 행사 공유하기 기능을 추가하였습니다.
카카오톡 공유 기능이 없는 이전 버전에서는 링크를 클릭하면 행사 목록 화면으로 이동합니다.
공유하기의 경우 eventId를 액티비티에서 파싱해서 지정해주어야 합니다.
이러한 이유로 다음과 같이 외부에서 eventId를 지정해준 후에 직접 행사 정보를 fetch하도록 변경하였습니다.

EventDetailViewModel.kt

// before
val eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID

init {
    fetchEvent()
    fetchIsScrapped()
}

// after
var eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID

init이 제거되었습니다.

EventDetailActivity.kt

fun onCreate() {
    fetchEvent()
    ...
}

private fun fetchEvent() {
        val eventId = intent.data?.getQueryParameter(EVENT_ID_KEY)?.toLong()
        if (eventId != null) viewModel.eventId = eventId

        viewModel.fetchEvent()
        viewModel.fetchIsScrapped()
    }

스크린샷 (선택)

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

예상 소요 시간 : 3시간
실제 소요 시간 : 3시간

💬 리뷰어 요구사항 (선택)

행사 공유 문구는 제가 임의로 정의했습니다.
더 좋은 문구가 있으시다면 추천해주세요~

…행사_카카오_공유_기능

ki960213

카카오 sdk를 잘 사용해주셨네요 👍
빌드 설정에 대해서도 배운 게 많아요.

ki960213 · 2023-12-19T06:41:33Z

...23-emmsale/app/src/main/java/com/emmsale/presentation/ui/eventDetail/EventDetailViewModel.kt

 ) : RefreshableViewModel() {
-    val eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID
+    var eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID


뷰모델이 eventId를 가지고 있어서 그럴까요? 점점 side effect가 늘어나는 구현이 되고 있네요ㅠㅠ
fetchEvent(eventId: Long), fetchIsScrapped(eventId: Long) 이렇게 하고 Activity에서 eventId를 가지고 있는 게 나을까요? 궁금해서 질문드립니다

side effect가 늘어나도 상태 관리에 대한 문제가 발생하는 상황이 아니라고 생각이 드는데 혹시 어떤 것이 우려되시나요?

예를 들어 fetchEvent() 호출 후 eventId 값을 변경하고 fetchIsScrapped()를 호출하면 일관성이 깨지게 됩니다.
사실 저희는 이렇게 코드를 작성하지 않을 거라 예상되지만 side effect 발생 가능성을 열어둔 구조라고 생각합니다.

말씀하신 부분에 대해서 공감합니다.
다만, 토마스 말씀처럼 그렇게 코드를 작성하지도 않을 것이고 모든 코드를 순수 함수로 만들기에는 어려움이 있다고 생각합니다.

예를 들어서, 제안해주신 fetchEvent(eventId: Long), fetchIsScrapped(eventId: Long) 도 마찬가지로 함수 외부의 상태를 변경하기 때문에 결국 side effect가 발생하는 것은 동일합니다.
그만큼 함수형으로 모든 코드를 작성하는 것이 어렵다고 생각하며 이후에 문제를 느낀다면 그 때 개선해봐도 좋을 것 같습니다 👍

사실 이전 구조와 차이점은 뷰모델 객체가 생성된 이후에 eventId가 변경될 수 있다는 것입니다.
eventId의 setter가 public 이지만 "카카오 공유 메세지 클릭해서 들어올 때 처리하는 것 빼고는 eventId를 수정하면 안돼"라는 추가 지식을 알아야만 의도치 않은 결과를 막을 수 있다는 점 때문에 약간 아쉬운 구조라고 생각했는데, 그래도 매번 eventId를 인자로 넘겨야 하는 것보다는 낫다고 생각이 들어서 지금도 괜찮은 것 같네요 :)

ki960213 · 2023-12-19T06:41:40Z

...23-emmsale/app/src/main/java/com/emmsale/presentation/ui/eventDetail/EventDetailViewModel.kt

+    fun shareEvent() {
+        val eventShareTemplate = eventTemplateMaker.create(
+            eventId = eventId,
+            eventName = event.value.name,
+            posterUrl = event.value.posterImageUrl,
+        )
+        eventSharer.shareEvent(eventShareTemplate)
+    }
+


구성변경에 대응할 필요가 없고 비즈니스 로직이 포함되지 않은 로직을 viewModel에 정의할 필요가 있을까요?

구성변경과 상관없이 카카오톡 공유하기를 비즈니스 로직이라고 생각했습니다.

카카오톡 앱이 있어야만 수행할 수 있는 로직이 비즈니스 로직인가요? 궁금해서 질문드립니다..!
뷰모델이 카카오톡 앱에 공유할 데이터를 가지고 있고 UI 컨트롤러가 카카오톡 앱에 그 데이터를 공유하는 게 올바른 책임 할당이라고 생각합니다. 😊

어떻게 생각하느냐에 따라 다르겠지만 저는 비즈니스 로직이라고 생각합니다.
흔히 안드로이드에서 네트워크, 저장소 등을 사용하는 로직들도 비즈니스 로직으로 분류하듯이,
카카오 api를 사용해서 네트워크 연결을 하는 로직 또한 비즈니스 로직으로 분류된다고 생각합니다.

그렇다면 갤러리 앱에서 사진을 불러오는 기능도 뷰모델에서 하는 게 좋을까요? 아니면 지도 앱에 현재 화면에 보여지는 행사의 장소를 검색하는 기능도 뷰모델에서 하는 게 좋을까요? 😕
위 기능 모두 Context를 @ApplicationContext로 주입받는 클래스를 만들면 ViewModel은 Context에 의존하지는 않게 됩니다

갤러리도 저장소 역할을 하기 때문에 관점에 따라 Activity에서 처리할 수도 있고 ViewModel에서 처리할 수도 있을 것 같습니다.
다만, 카카오톡 공유하기와 다르게 갤러리는 확실하게 Activity의 콜백을 필요로하기 때문에 뷰모델에서 처리하기 까다로울 듯 하네요 😅

토마스 말씀처럼 Activity는 UiController이지 BusinessController가 아닙니다.
따라서 Ui와 관련된 로직 위주로 컨트롤하는 역할을 수행하죠.
행사 정보를 공유한다 라는 기능은 UI보다는 비즈니스 로직에 가깝다고 느껴지네요.

저희 앱에는 갤러리에 사진을 추가하는 기능이 없지만 만약 생긴다면 사진을 저장한다라는 기능도 비즈니스 로직이므로 뷰모델에서 처리하는 게 나을까요?

사진을 저장한다 라는 로직도 비즈니스 로직이기 때문에, 어떠한 개발자들은 토마스가 말씀하신 것처럼 하기도 합니다.
위에 댓글에도 언급했지만 행사 정보를 공유한다. 와 사진을 저장한다 라는 코드를 비즈니스 로직으로 분리했을 때 증가하는 코드와 복잡성은 후자일 때 더 커지죠..
행사 정보 공유 로직은 그런 단점이 따로 없기 때문에 비즈니스 로직으로 ViewModel에 위치시켜도 문제가 되지 않는다는 것입니다.
또한, ActivityController가 반드시 필요한 로직인가 아닌가에 대해서도 따져봐야 하구요. (갤러리)

그렇군요. 토스트 메세지 띄우기, 스낵바 보여주기, 다른 화면 이동 같은 UI 관련 기능만 Activity에서 처리하고 다른 앱에 데이터 공유 기능은 UI 관련 기능으로 볼 수 없으니 Activity에서 처리하지 않는 게 좋겠군요.
덕분에 Activity와 ViewModel의 역할에 대해 배웠네요. :)

ki960213 · 2023-12-19T06:41:44Z

...emmsale/app/src/main/java/com/emmsale/presentation/ui/eventDetail/eventSharer/EventSharer.kt

+class EventSharer @Inject constructor(
+    @ApplicationContext private val context: Context,
+) {


context가 필요한 객체를 viewModel에서 의존하고 있는 것 같아요...!

위 클래스 객체는 Activity의 context가 아닌, ApplicationContext를 의존하기 때문에 구성변경시 메모리릭이 발생하지는 않습니다.
그 외에 토마스가 생각하시기에 context가 필요한 객체를 ViewModel에서 의존하면 어떠한 문제가 발생하나요??

context를 사용한다는 것은 안드로이드 관련 기능과 강하게 결합된다고 생각합니다. 물론 AAC ViewModel을 사용하고 있긴 하지만 ViewModel은 안드로이드 의존성을 최대한 줄이는 게 적절한 책임할당이라고 생각합니다. 메모리릭이 발생하지 않는다는 이유로 ApplicationContext를 사용해도 괜찮다면 뷰모델의 주 생성자 매개변수로 @ApplicationContext val context: Context를 선언하는 것은 괜찮을까용?

만약 토마스 말씀처럼 context를 주입받아서 ViewModel을 사용하는 방법도 있을 겁니다.
하지만 위 코드에서는 그럴 필요와 이유가 전혀 없기 때문에 객체를 전달받는 구조로 작성하였습니다.

저는 테스트하기 좋은 구조가 자연스럽게 유지보수하기에도 좋은 구조로 이어진다고 생각합니다.
context 자체를 주입받고 내부에서 Sharer과 TemplateMaker를 생성한다면 테스트하기 어려워질 듯 합니다.
우리가 DI를 사용하는 이유도 마찬가지죠.

또한, context를 직접적으로 사용하지 않기 때문에 안드로이드 의존성도 직접적으로는 없다고 볼 수 있습니다.
Repository에서 Room을 사용한다고 해서, Repository를 사용하는 ViewModel이 (AAC를 제외하고) 안드로이드 의존성에 강하게 결합되어 있다고 하지는 않는 것과 비슷하다고 생각해주셔도 좋을 것 같습니다~

저는 저장소가 "데이터를 가져올 수 있고 영속화 할 수 있는 객체"라고 추상화해서 생각했습니다. Repository도 context에 의존하진 않고 LocalDataSource, RemoteDataSource를 추상화한 인터페이스에 의존하고 있습니다.
사실 지금 Repository에서 ApiResponse 객체를 받고 있다는 것이 데이터를 어떻게 가져올 지에 대한 구현을 노출하고 있다는 것을 의미하긴 하지만 저는 이건 타협할만한 가치가 있다고 생각합니다. ApiResponse를 사용하면 뷰모델에서 Repository 메서드 호출마다 직접 네트워크 에러를 캐치하지 않아도 되기 때문입니다. 하지만 카카오톡 공유 기능은 UI 컨트롤러에서 직접 처리하는 게 더 코드가 줄기 때문에 타협하지 않는 게 낫다고 생각합니다.
정리하자면 뷰모델과 Repository는 도메인 레이어이기 때문에 최대한 안드로이드 의존성을 줄이되, 구현 시간, 코드량, 실수 확률을 크게 줄일 수 있다면 타협하는 것이 좋다고 생각합니다.

음.. 뷰모델은 UI 레이어입니다.
그리고 코드가 말도 안 되게 늘어나는게 아니라면 역할에 따라 위치하는게 옳다고 생각합니다.

레이어에 따라 안드로이드 의존성을 줄여야 하는 것이라면 UI 레이어이기 때문에 크게 문제가 되지 않는다고 이해해도 될까요?

아 죄송합니다. 뷰모델과 Repository는 도메인 로직을 처리하기 때문에 라고 정정하겠습니다 😓

...-emmsale/app/src/main/java/com/emmsale/presentation/ui/feedDetail/uiState/CommentsUiState.kt

android/2023-emmsale/app/src/main/res/drawable/ic_eventdetail_share.xml

tmdgh1592 · 2023-12-19T09:16:14Z

@ki960213
제보해주신 공유된 메시지 전체 부분을 클릭하면 팅기는 버그 수정 완료하였습니다 : )
이 외에 리뷰 반영과 의견을 남겨놓았으니 확인부탁드립니다.

ki960213

고생하셨습니다 😄

# Conflicts: # android/2023-emmsale/app/proguard-rules.pro # android/2023-emmsale/app/src/main/java/com/emmsale/presentation/ui/feedDetail/uiState/CommentsUiState.kt # android/2023-emmsale/app/src/main/res/layout/activity_child_comments.xml # android/2023-emmsale/app/src/main/res/layout/activity_feed_detail.xml

tmdgh1592 added 13 commits December 19, 2023 09:50

feat: 행사 공유 아이콘 리소스 추가

8cdebdd

feat: 행사 상세 화면 툴바에 공유하기 아이콘 버튼 추가

82dc30c

chore: 카카오 공유 모듈 의존성 추가

e503eb2

chore: 카카오 공유하기 난독화 설정

d299cac

chore: 카카오 앱키, 스키마, 호스트 리소스화

f98ae52

feat: 카카오 scheme, host를 manifest에 등록

d2501fc

feat: 카카오 Sdk 초기화하는 기능 구현

958d765

feat: 행사 공유 템플릿을 생성하는 클래스 구현

ce9e337

feat: 행사 공유를 공유하는 클래스 구현

debf877

refactor: 행사 공유 로직을 ViewModel에서 처리하도록 변경

d826e56

feat: 행사 정보 카카오톡 공유하기 기능 구현

48f7798

chore: 카카오톡 공유하기 모듈 버전 업그레이드 (v2.12.0)

60ee764

chore: 카카오톡 공유하기 난독화 추가

1dfbb56

tmdgh1592 added Android 안드로이드 관련 이슈 기능 추가 새로운 기능 추가 및 기존 기능 변경 labels Dec 19, 2023

tmdgh1592 requested review from chws0508 and ki960213 December 19, 2023 04:13

tmdgh1592 self-assigned this Dec 19, 2023

refactor: 코드 포맷팅

982cb75

tmdgh1592 force-pushed the android-main branch from b255e70 to e8b18a9 Compare December 19, 2023 05:48

Merge remote-tracking branch 'upstream/android-main' into Feature/#839-…

c2d40c1

…행사_카카오_공유_기능

tmdgh1592 force-pushed the Feature/#839-행사_카카오_공유_기능 branch from d839e7b to c2d40c1 Compare December 19, 2023 05:48

ki960213 reviewed Dec 19, 2023

View reviewed changes

tmdgh1592 added 4 commits December 19, 2023 17:59

feat: 공유하기 아이콘 색상에 투명도를 없애고 #D9D9D9로 변경

5d573ca

fix: 카카오톡으로 공유된 행사 메시지 전체 부분을 클릭하면 크래시 발생하는 버그 수정

ce91571

refactor: 라인 포맷팅

1cc4141

refactor: 라인 포맷팅

37b0cad

ki960213 approved these changes Dec 27, 2023

View reviewed changes

ki960213 added 2 commits January 28, 2024 23:16

fix(RefreshableViewModel): 새로고침 시 네트워크 에러에 의해 실패하면 네트워크 에러 이벤트가 발생되도록 변경

d67fc94

feat(EventDetailActivity): 행사 상세 화면에서 탭이 좌우 스크롤 되도록 변경

4671335

ki960213 merged commit d1d3dcc into android-main Jan 28, 2024
1 check passed

ki960213 deleted the Feature/#839-행사_카카오_공유_기능 branch January 28, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/#839 행사 카카오 공유 기능 #888

Feature/#839 행사 카카오 공유 기능 #888

tmdgh1592 commented Dec 19, 2023 •

edited

Loading

ki960213 left a comment •

edited

Loading

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 27, 2023

ki960213 Dec 27, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023

tmdgh1592 Dec 19, 2023

ki960213 Dec 19, 2023

tmdgh1592 Dec 19, 2023 •

edited

Loading

ki960213 Dec 19, 2023

tmdgh1592 commented Dec 19, 2023

ki960213 left a comment

Feature/#839 행사 카카오 공유 기능 #888

Feature/#839 행사 카카오 공유 기능 #888

Conversation

tmdgh1592 commented Dec 19, 2023 • edited Loading

#️⃣ 연관된 이슈

📝 작업 내용

스크린샷 (선택)

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

💬 리뷰어 요구사항 (선택)

ki960213 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 commented Dec 19, 2023

ki960213 left a comment

Choose a reason for hiding this comment

tmdgh1592 commented Dec 19, 2023 •

edited

Loading

ki960213 left a comment •

edited

Loading

tmdgh1592 Dec 19, 2023 •

edited

Loading