-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#839 행사 카카오 공유 기능 #888
The head ref may contain hidden characters: "Feature/#839-\uD589\uC0AC_\uCE74\uCE74\uC624_\uACF5\uC720_\uAE30\uB2A5"
Changes from all commits
8cdebdd
82dc30c
e503eb2
d299cac
f98ae52
d2501fc
958d765
ce9e337
debf877
d826e56
48f7798
60ee764
1dfbb56
982cb75
c2d40c1
5d573ca
ce91571
1cc4141
37b0cad
9d40008
d67fc94
4671335
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ import com.emmsale.presentation.base.RefreshableViewModel | |
import com.emmsale.presentation.common.livedata.NotNullLiveData | ||
import com.emmsale.presentation.common.livedata.NotNullMutableLiveData | ||
import com.emmsale.presentation.common.livedata.SingleLiveEvent | ||
import com.emmsale.presentation.ui.eventDetail.eventSharer.EventSharer | ||
import com.emmsale.presentation.ui.eventDetail.eventSharer.EventTemplateMaker | ||
import com.emmsale.presentation.ui.eventDetail.uiState.EventDetailScreenUiState | ||
import com.emmsale.presentation.ui.eventDetail.uiState.EventDetailUiEvent | ||
import dagger.hilt.android.lifecycle.HiltViewModel | ||
|
@@ -24,8 +26,10 @@ class EventDetailViewModel @Inject constructor( | |
stateHandle: SavedStateHandle, | ||
private val eventRepository: EventRepository, | ||
private val recruitmentRepository: RecruitmentRepository, | ||
private val eventTemplateMaker: EventTemplateMaker, | ||
private val eventSharer: EventSharer, | ||
) : RefreshableViewModel() { | ||
val eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID | ||
var eventId = stateHandle[EVENT_ID_KEY] ?: DEFAULT_EVENT_ID | ||
|
||
private val _event = NotNullMutableLiveData(Event()) | ||
val event: NotNullLiveData<Event> = _event | ||
|
@@ -51,17 +55,12 @@ class EventDetailViewModel @Inject constructor( | |
private val _uiEvent = SingleLiveEvent<EventDetailUiEvent>() | ||
val uiEvent: LiveData<EventDetailUiEvent> = _uiEvent | ||
|
||
init { | ||
fetchEvent() | ||
fetchIsScrapped() | ||
} | ||
|
||
private fun fetchEvent(): Job = fetchData( | ||
fun fetchEvent(): Job = fetchData( | ||
fetchData = { eventRepository.getEventDetail(eventId) }, | ||
onSuccess = { _event.value = it }, | ||
) | ||
|
||
private fun fetchIsScrapped(): Job = viewModelScope.launch { | ||
fun fetchIsScrapped(): Job = viewModelScope.launch { | ||
when (val result = eventRepository.isScraped(eventId)) { | ||
is Success -> _isScraped.value = result.data | ||
else -> {} | ||
|
@@ -117,6 +116,15 @@ class EventDetailViewModel @Inject constructor( | |
onFinish = { _canStartToWriteRecruitment.value = true }, | ||
) | ||
|
||
fun shareEvent() { | ||
val eventShareTemplate = eventTemplateMaker.create( | ||
eventId = eventId, | ||
eventName = event.value.name, | ||
posterUrl = event.value.posterImageUrl, | ||
) | ||
eventSharer.shareEvent(eventShareTemplate) | ||
} | ||
|
||
Comment on lines
+119
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 구성변경에 대응할 필요가 없고 비즈니스 로직이 포함되지 않은 로직을 viewModel에 정의할 필요가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 갤러리도 저장소 역할을 하기 때문에 관점에 따라 Activity에서 처리할 수도 있고 ViewModel에서 처리할 수도 있을 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 토마스 말씀처럼 Activity는 UiController이지 BusinessController가 아닙니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 그렇군요. 토스트 메세지 띄우기, 스낵바 보여주기, 다른 화면 이동 같은 UI 관련 기능만 Activity에서 처리하고 다른 앱에 데이터 공유 기능은 UI 관련 기능으로 볼 수 없으니 Activity에서 처리하지 않는 게 좋겠군요. |
||
companion object { | ||
const val EVENT_ID_KEY = "EVENT_ID_KEY" | ||
private const val DEFAULT_EVENT_ID = -1L | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package com.emmsale.presentation.ui.eventDetail.eventSharer | ||
|
||
import android.content.Context | ||
import com.emmsale.R | ||
import com.emmsale.presentation.common.extension.showToast | ||
import com.kakao.sdk.common.util.KakaoCustomTabsClient | ||
import com.kakao.sdk.share.ShareClient | ||
import com.kakao.sdk.share.WebSharerClient | ||
import com.kakao.sdk.template.model.FeedTemplate | ||
import dagger.hilt.android.qualifiers.ApplicationContext | ||
import javax.inject.Inject | ||
|
||
class EventSharer @Inject constructor( | ||
@ApplicationContext private val context: Context, | ||
) { | ||
Comment on lines
+13
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. context가 필요한 객체를 viewModel에서 의존하고 있는 것 같아요...! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 위 클래스 객체는 Activity의 context가 아닌, ApplicationContext를 의존하기 때문에 구성변경시 메모리릭이 발생하지는 않습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. context를 사용한다는 것은 안드로이드 관련 기능과 강하게 결합된다고 생각합니다. 물론 AAC ViewModel을 사용하고 있긴 하지만 ViewModel은 안드로이드 의존성을 최대한 줄이는 게 적절한 책임할당이라고 생각합니다. 메모리릭이 발생하지 않는다는 이유로 ApplicationContext를 사용해도 괜찮다면 뷰모델의 주 생성자 매개변수로 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 만약 토마스 말씀처럼 context를 주입받아서 ViewModel을 사용하는 방법도 있을 겁니다. 저는 테스트하기 좋은 구조가 자연스럽게 유지보수하기에도 좋은 구조로 이어진다고 생각합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 또한, context를 직접적으로 사용하지 않기 때문에 안드로이드 의존성도 직접적으로는 없다고 볼 수 있습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 저장소가 "데이터를 가져올 수 있고 영속화 할 수 있는 객체"라고 추상화해서 생각했습니다. Repository도 context에 의존하진 않고 LocalDataSource, RemoteDataSource를 추상화한 인터페이스에 의존하고 있습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 음.. 뷰모델은 UI 레이어입니다. 레이어에 따라 안드로이드 의존성을 줄여야 하는 것이라면 UI 레이어이기 때문에 크게 문제가 되지 않는다고 이해해도 될까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아 죄송합니다. 뷰모델과 Repository는 도메인 로직을 처리하기 때문에 라고 정정하겠습니다 😓 |
||
private val isNotKakaoTalkInstalled | ||
get() = !ShareClient.instance.isKakaoTalkSharingAvailable(context) | ||
|
||
fun shareEvent(eventTemplate: FeedTemplate) { | ||
if (isNotKakaoTalkInstalled) { | ||
handleKakaoNotInstalledError(eventTemplate) | ||
return | ||
} | ||
|
||
share(eventTemplate) | ||
} | ||
|
||
private fun handleKakaoNotInstalledError(eventTemplate: FeedTemplate) { | ||
val sharerUrl = WebSharerClient.instance.makeDefaultUrl(eventTemplate) | ||
|
||
runCatching { | ||
KakaoCustomTabsClient.openWithDefault(context, sharerUrl) | ||
}.onFailure { | ||
context.showToast(R.string.eventdetail_no_exist_browser) | ||
} | ||
|
||
runCatching { | ||
KakaoCustomTabsClient.open(context, sharerUrl) | ||
}.onFailure { | ||
context.showToast(R.string.eventdetail_no_exist_browser) | ||
} | ||
} | ||
|
||
private fun share(eventTemplate: FeedTemplate) { | ||
ShareClient.instance.shareDefault(context, eventTemplate) { sharingResult, error -> | ||
if (error != null) { | ||
context.showToast(R.string.eventdetail_kakao_share_fail) | ||
} else if (sharingResult != null) { | ||
context.startActivity(sharingResult.intent) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package com.emmsale.presentation.ui.eventDetail.eventSharer | ||
|
||
import android.content.Context | ||
import com.emmsale.R | ||
import com.emmsale.presentation.ui.eventDetail.EventDetailViewModel | ||
import com.kakao.sdk.template.model.Button | ||
import com.kakao.sdk.template.model.Content | ||
import com.kakao.sdk.template.model.FeedTemplate | ||
import com.kakao.sdk.template.model.Link | ||
import dagger.hilt.android.qualifiers.ApplicationContext | ||
import javax.inject.Inject | ||
|
||
class EventTemplateMaker @Inject constructor( | ||
@ApplicationContext private val context: Context, | ||
) { | ||
fun create( | ||
eventId: Long, | ||
eventName: String, | ||
posterUrl: String?, | ||
): FeedTemplate = FeedTemplate( | ||
content = Content( | ||
title = eventName, | ||
description = context.getString(R.string.eventdetail_share_template_description), | ||
imageUrl = posterUrl ?: "", | ||
link = Link(androidExecutionParams = mapOf(EventDetailViewModel.EVENT_ID_KEY to eventId.toString())), | ||
), | ||
buttons = listOf( | ||
Button( | ||
context.getString(R.string.eventdetail_share_button_title), | ||
Link(androidExecutionParams = mapOf(EventDetailViewModel.EVENT_ID_KEY to eventId.toString())), | ||
), | ||
), | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<vector xmlns:android="http://schemas.android.com/apk/res/android" | ||
android:width="16dp" | ||
android:height="20dp" | ||
android:viewportWidth="16" | ||
android:viewportHeight="20"> | ||
<path | ||
android:fillColor="#D9D9D9" | ||
android:pathData="M7,13H9V4H11L8,0L5,4H7V13ZM15,7H12V9H14V18H2V9H4V7H1C0.735,7 0.48,7.105 0.293,7.293C0.105,7.48 0,7.735 0,8V19C0,19.265 0.105,19.52 0.293,19.707C0.48,19.895 0.735,20 1,20H15C15.265,20 15.52,19.895 15.707,19.707C15.895,19.52 16,19.265 16,19V8C16,7.735 15.895,7.48 15.707,7.293C15.52,7.105 15.265,7 15,7Z" /> | ||
</vector> |
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.
뷰모델이 eventId를 가지고 있어서 그럴까요? 점점 side effect가 늘어나는 구현이 되고 있네요ㅠㅠ
fetchEvent(eventId: Long)
,fetchIsScrapped(eventId: Long)
이렇게 하고 Activity에서 eventId를 가지고 있는 게 나을까요? 궁금해서 질문드립니다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.
side effect가 늘어나도 상태 관리에 대한 문제가 발생하는 상황이 아니라고 생각이 드는데 혹시 어떤 것이 우려되시나요?
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.
예를 들어
fetchEvent()
호출 후 eventId 값을 변경하고fetchIsScrapped()
를 호출하면 일관성이 깨지게 됩니다.사실 저희는 이렇게 코드를 작성하지 않을 거라 예상되지만 side effect 발생 가능성을 열어둔 구조라고 생각합니다.
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.
말씀하신 부분에 대해서 공감합니다.
다만, 토마스 말씀처럼 그렇게 코드를 작성하지도 않을 것이고 모든 코드를 순수 함수로 만들기에는 어려움이 있다고 생각합니다.
예를 들어서, 제안해주신
fetchEvent(eventId: Long)
,fetchIsScrapped(eventId: Long)
도 마찬가지로 함수 외부의 상태를 변경하기 때문에 결국 side effect가 발생하는 것은 동일합니다.그만큼 함수형으로 모든 코드를 작성하는 것이 어렵다고 생각하며 이후에 문제를 느낀다면 그 때 개선해봐도 좋을 것 같습니다 👍
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.
사실 이전 구조와 차이점은 뷰모델 객체가 생성된 이후에 eventId가 변경될 수 있다는 것입니다.
eventId의 setter가 public 이지만 "카카오 공유 메세지 클릭해서 들어올 때 처리하는 것 빼고는 eventId를 수정하면 안돼"라는 추가 지식을 알아야만 의도치 않은 결과를 막을 수 있다는 점 때문에 약간 아쉬운 구조라고 생각했는데, 그래도 매번 eventId를 인자로 넘겨야 하는 것보다는 낫다고 생각이 들어서 지금도 괜찮은 것 같네요 :)