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

Feature/#786 푸시 알림으로 채팅 화면에 들어가면 뒤로가기를 했을 때 홈 화면으로 이동 #829

Conversation

tmdgh1592
Copy link
Member

#️⃣ 연관된 이슈

close : #786

📝 작업 내용

  • 푸시 알림으로 앱에 진입했을 때, 뒤로가기를 하면 앱이 종료되지 않고 홈 화면으로 이동하게끔 하였습니다.
  • 기존에 NotificationChannel을 생성하는 코드가 보일러 플레이트 라고 생각하여 개선을 해보았습니다.
    • KerdyNotificationChannel 클래스를 만들어 채널 생성을 관리합니다.
    • 가능한 OCP를 준수하도록 변경하였습니다.
      • 채널이 추가되어도 KerdyNotificationChannel 의 상수만 추가하면 되므로 유지보수성 🆙

스크린샷 (선택)

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

예상 소요 시간 : 2시간
실제 소요 시간 : 1시간

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

@tmdgh1592 tmdgh1592 added Android 안드로이드 관련 이슈 Low Priority 리뷰 우선순위가 낮은 PR labels Nov 7, 2023
@tmdgh1592 tmdgh1592 added this to the 레벨 5 milestone Nov 7, 2023
@tmdgh1592 tmdgh1592 self-assigned this Nov 7, 2023
Copy link
Collaborator

@chws0508 chws0508 left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다!
훨씬 코드가 깔끔해진 것 같아요
apporve와 함께 궁금한 점 질문 남깁니다.

@@ -27,6 +27,7 @@
<activity
android:name=".presentation.ui.messageList.MessageListActivity"
android:launchMode="singleTask"
android:parentActivityName=".presentation.ui.main.MainActivity"
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 다른 알림들도 parentActivity를 지정해줄 수 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저와 토마스는 이 방식이 아닌, activity에서 뒤로가기에 대한 동작으로 parentActivity로 이동시키도록 구현해놓아서, 이 부분에 대한 통일이 필요할 것 같네요. 저는 현재 부나의 방식이 좋아보이네요

Copy link
Member Author

Choose a reason for hiding this comment

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

혹시 다른 알림들도 parentActivity를 지정해줄 수 있을까요?

넵, Toolbar의 < 버튼이나, 기기의 뒤로가기 버튼을 클릭했을 때 등의 분기를 생각하지 않아도 되어서 parentActivity를 지정하는 것을 권장드립니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

동의합니다.

Comment on lines +135 to +142
): PendingIntent? = TaskStackBuilder.create(this).run {
addNextIntentWithParentStack(intent)
getPendingIntent(
notificationId,
PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE,
)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

이런게 있었군요 ㄷㄷ

Copy link
Collaborator

Choose a reason for hiding this comment

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

PendingIntent.FLAG_IMMUTABLE를 사용한 이유가 있을까요? 어느 상황에서 써야하나요

Copy link
Member Author

Choose a reason for hiding this comment

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

FLAG_IMMUTABLE은 PendingIntent가 가지고 있는 Intent의 Bundle(extra)값을 수정할 필요가 없을 때 지정합니다~
API 31 이상부터는 필수로 지정해주어야 해서 전달하고 있습니다.

Copy link
Collaborator

@ki960213 ki960213 left a comment

Choose a reason for hiding this comment

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

가독성 있게 잘 작성해주셨네용. 👍
더 이상 리뷰할 게 없네요.

@@ -27,6 +27,7 @@
<activity
android:name=".presentation.ui.messageList.MessageListActivity"
android:launchMode="singleTask"
android:parentActivityName=".presentation.ui.main.MainActivity"
Copy link
Collaborator

Choose a reason for hiding this comment

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

동의합니다.

@tmdgh1592 tmdgh1592 merged commit 3a74e07 into android-main Nov 8, 2023
1 check passed
@tmdgh1592 tmdgh1592 deleted the Feature/#786-푸시_알림으로_채팅_화면에_들어가면_뒤로가기를_했을_때_홈_화면으로_이동 branch November 8, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 안드로이드 관련 이슈 Low Priority 리뷰 우선순위가 낮은 PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants