-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] FCM 푸쉬알림 / 푸쉬알림 구현 #229
Conversation
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.
구현하느라 넘 고생하셨습니다!! 👍👍👍
override fun onActivitySaveInstanceState(activity: android.app.Activity, outState: Bundle) {} | ||
|
||
override fun onActivityDestroyed(activity: android.app.Activity) {} | ||
}) |
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.
한 클래스는 최대한 하나의 기능만 담당해야 한다는 '단일 책임의 원칙'에 따라, ActivityLifecycleCallbacks 관련 코드는 별도의 클래스로 분리하는 건 어떨까요?!
관련 자료를 검색해봤을 때도 별도의 클래스를 만들어서 사용하는 경우가 많았던 거 같아요 :)
|
||
override fun onMessageReceived(remoteMessage: RemoteMessage) { | ||
super.onMessageReceived(remoteMessage) | ||
if (remoteMessage.data.isNotEmpty() && !WineyApplication.isAppInForeground) { |
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.
외부 클래스에서 isAppInForeground 라는 변수를 참조할 때도 WineyApplication 클래스에서 직접 가져오는 것보다,
별도의 클래스를 따로 만들어서 앱의 포그라운드/백그라운드 상태를 구분하는 데 사용한다면
각 클래스가 자신이 맡은 책임에 좀 더 집중한다는 느낌이 들어서 좋을 거 같습니다!
} | ||
|
||
private fun sendNotification(remoteMessage: RemoteMessage) { | ||
val uniId: Int = (System.currentTimeMillis() / 7).toInt() |
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.
혹시 uniId 가 어떤 뜻인지 알 수 있을까요?? 코드 맥락 상으로는 식별자 역할을 하는 임의의 숫자 같은 느낌인데, 변수명을 조금 더 구체적으로 적어도 좋을 거 같습니다 :)
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.
uniId라는 변수명은 unique identifier의 줄임말로, 고유 식별자를 의미하는 네이밍으로 쓰였는데 지금보니 모호해 보이는 부분이 있네요 ! 조금 더 구체적인 네이밍 고려해보겠습니다 !
val intent = Intent(this, SplashActivity::class.java) | ||
intent.putExtra(KEY_NOTI_TYPE, remoteMessage.data[KEY_NOTI_TYPE]) | ||
intent.putExtra(KEY_FEED_ID, remoteMessage.data[KEY_FEED_ID]) | ||
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP) |
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.
apply 같은 스코프 함수를 이용하면 intent 라는 변수명의 중복을 줄일 수 있을 거 같아요!
uniId, | ||
intent, | ||
PendingIntent.FLAG_ONE_SHOT or PendingIntent.FLAG_IMMUTABLE | ||
) |
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.
이 글을 통해 Intent vs. PendingIntent 차이점을 알 수 있었네요! 참고하면 좋을 거 같아요~!
@@ -332,6 +333,9 @@ class WineyFeedFragment : | |||
} | |||
|
|||
is UiState.Failure -> { | |||
val intent = Intent(requireActivity(), LoginActivity::class.java) | |||
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK or Intent.FLAG_ACTIVITY_NEW_TASK) | |||
startActivity(intent) | |||
snackBar(binding.root) { state.msg } |
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.
원래 로직이 토큰 만료되면 MainViewModel에서 로그아웃 메소드 실행하고
메인 액티비티에서 logoutState 관찰해서 로그인 화면으로 넘어가는 걸로 알고 있는데
여기서도 로그인 화면으로 바로 전환하게 만든 이유가 있을까요??
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.
http응답이 401일때만 로그아웃메소드가 실행되어서 404등 다른 실패응답에는 로그아웃 처리가 제대로 되지 않아서 지금과 같이 구현했습니다만 지금처럼 일괄처리하는 방식보다는 404만 추가대응하는 방식이 나아보여서 다시 수정했습니다.
is UiState.Success -> { | ||
when (state.data) { | ||
true -> { | ||
binding.ivMypageAgree.transitionToEnd() |
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.
오홍?! 버튼 토글할 때도 모션 레이아웃을 적용할 수 있군요! 👍
val newData = data?.copy(fcmIsAllowed = true) | ||
dataStoreRepository.saveUserInfo(newData) | ||
} | ||
} |
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.
lifecycleScope.launch {
val data = dataStoreRepository.getUserInfo().first()
val newData = data?.copy(fcmIsAllowed = true)
dataStoreRepository.saveUserInfo(newData)
}
중복되는 위의 내용을 함수화 시키면 좋을 거 같습니다~!
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.
isAllowed가 true든 false든 patchUserInfo 함수를 실행시켜야 한다면, when 블록 바깥에서 한번만 호출하면 될 거 같아요!
@@ -292,6 +356,18 @@ class MyPageFragment : BindingFragment<FragmentMyPageBinding>(R.layout.fragment_ | |||
} | |||
} | |||
|
|||
private fun updateSwitchState(data: User) { |
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.
'무엇을' switch 하는 건지 바로 알 수 있도록 함수명에 힌트를 주면 좋을 거 같아요~!
<StateSet> | ||
|
||
</StateSet> | ||
</MotionScene> |
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.
우왕! 모션 레이아웃 코드 처음 보네요! 👍👍
근데 xml 파일 이름이 다소 긴 거 같아서 mypage_noti_agree_motion_scene.xml
이 정도의 이름도 괜찮을 듯 합니다!
아니면 특별히 지금처럼 이름을 지은 이유가 있을까요??
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.
수정하겠습니다 !!
<path | ||
android:pathData="M12,12m8,-0a8,8 0,1 1,-16 -0a8,8 0,1 1,16 -0" | ||
android:fillColor="#ffffff"/> | ||
</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.
ic_mypage_switchon.xml, ic_mypage_switchoff.xml
-> 혹시 이 파일들은 어디서 사용되나요??
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.
사용하지 않아서 삭제했습니다 !
import android.app.Application | ||
import android.os.Bundle | ||
|
||
class ActivityLifecycleHandler(private val application: Application) : |
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.
생성자의 인자로 application을 넘겨줘야 하는 이유가 궁금합니다!
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.
불필요한 인자네요 수정하겠습니다 !
} | ||
|
||
private fun generateUniqueIdentifier(): Int { | ||
return (System.currentTimeMillis() / 7).toInt() |
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.
7로 나누는 이유가 있을까요?!
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.
특별한 이유는 없습니다 ! 유니크한 고유 식별자를 만들기위해서 사용한 방식입니다.
return (System.currentTimeMillis() / 7).toInt() | ||
} | ||
|
||
private fun createAndShowNotification(remoteMessage: RemoteMessage, uniqueIdentifier: Int, pendingIntent: PendingIntent) { |
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.
음 개인적으로 함수명에 and를 사용하는 것은 지양하는 게 좋을 거 같다는 생각이 듭니다!
가독성을 너무 해치지 않는 선에서, 최대한 한 함수는 하나의 기능만 담당하는 게 좋을 거 같아서요!
여기서도 create, show 라는 각 동작에 대해 별도의 함수로 분리하는 게 어떨까요?!
그러면 나중에 notification builder 생성 코드를 수정할 일이 있을 때 좀 더 편할 거 같습니다 :)
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.
넵 수정하겠습니다
NotificationType.LIKE_NOTIFICATION -> navigateToDetail(feedId?.toInt()) | ||
NotificationType.COMMENT_NOTIFICATION -> navigateToDetail(feedId?.toInt()) | ||
else -> navigateToLevelupHelp() | ||
} |
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.
어라랏 여기서 notiType에 따라 다른 함수가 실행되는 게 아니라, 몇몇은 동일한 함수를 실행시키네요?! 그러면 when문으로 구분하는 이유가 없어지는 거 같은데,, 이 부분에 대한 의견 궁금합니다!
val newData = data?.copy(fcmIsAllowed = true) | ||
dataStoreRepository.saveUserInfo(newData) | ||
} | ||
} |
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.
isAllowed가 true든 false든 patchUserInfo 함수를 실행시켜야 한다면, when 블록 바깥에서 한번만 호출하면 될 거 같아요!
LIKE_NOTIFICATION("LIKENOTI"), | ||
COMMENT_NOTIFICATION("COMMENTNOTI"), | ||
HOW_TO_LEVEL_UP("HOWTOLEVELUP") | ||
} |
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.
러넥트에서 enum class 자체를 인텐트의 부가 데이터로 넘기는 코드를 작성했는데, 아래 사진처럼 enum class가 Serializable?
타입으로 인식되더라구요! 그래서 getCompatibleSerializableExtra 같은 확장함수 이용해서 이넘 클래스 자체를 부가 데이터로 주고 받을 수 있게 했습니다!
enum class CourseDetailRootScreen {
COURSE_STORAGE_SCRAP,
COURSE_DISCOVER,
COURSE_DISCOVER_SEARCH,
MY_PAGE_UPLOAD_COURSE
}
private fun initIntentExtraData() {
intent.getCompatibleSerializableExtra<CourseDetailRootScreen>(EXTRA_ROOT_SCREEN)?.let {
rootScreen = it
}
publicCourseId = intent.getIntExtra(EXTRA_PUBLIC_COURSE_ID, 0)
}
이렇게 하면 굳이 이넘 클래스가 key라는 문자열 속성을 가질 필요가 없어서 좋은 거 같아요!
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.
참고로 함수를 더블 클릭하고, Ctrl + P 누르면 함수의 인자 타입에 대해 알 수 있습니다 :)
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.
참고하겠습니다 !! 근데 지금같은 경우에는 fcm알림으로 날아오는 데이터에 notitype이 포함되어있고 그걸 구분하기위해서 해당 키값을 이넘클래스에 포함시켜주었는데, 위 방식으로 처리할 수 있을까요?
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.
아아 서버에서 저 문자열들로 노티 타입을 구분하는군요!! 그러면 지금처럼 구현하는 게 좋을 거 같습니다!
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.
리뷰 늦어져서 미안합니다 고생했어요 😭
override fun onNewToken(token: String) { | ||
super.onNewToken(token) | ||
|
||
CoroutineScope(Dispatchers.IO).launch { dataStoreRepository.saveDeviceToken(token) } |
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.
Dispatchers.IO 으로 지정되어야하는 이유가 있을까요 ? FCM 자체가 백그라운드 스레드에서 호출된다고 알고 있어서 별도 스코프를 지정하지 않아도 될까하는데 궁금합니다 ! 자세히 공부해본 적은 없어서 아니라면 알려주세요 😮
All methods are invoked on a background thread, and may be called when the app is in the background or not open
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.
맞습니다! 별도의 백그라운드 스레드에서 실행되기때문에 스코프 지정이 필요없습니다. 그런데 Dispatcher.default는 최대 12개의 스레드, 즉 12개의 작업만 한번에 할 수 있는 반면, Dispatcher.io는 최대 64개의 스레드를 사용한다고 합니다. 따라서 한 가지의 무거운 작업을 할때는 Dispatcher.default, 대기시간이 있는 가벼운 입출력 작업을 할때는 Dispatcher.io가 성능향상에 도움이 된다고 합니다.
@@ -43,4 +46,14 @@ class AuthDataSource @Inject constructor( | |||
requestPatchNicknameDto: RequestPatchNicknameDto | |||
): BaseResponse<Unit> = | |||
authService.patchNickname(requestPatchNicknameDto) | |||
|
|||
suspend fun patchAllowedNotification( |
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.
patchAllowedNotification 보다는 patchNotificationAgreement와 같은 네이밍해줘도 좋을 것 같아요 !
@@ -0,0 +1,72 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<MotionScene |
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.
😲 👍
📝 Work Description
📸 Screenshot
📣 To Reviewers
앱이 백그라운드일때만 알림이 오도록 구현했습니다.
코드에 대해서 궁금하신점 언제든지 물어봐주세요 !!