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

강원대_Android_이아림_6주차과제_STEP2 #74

Open
wants to merge 39 commits into
base: arieum
Choose a base branch
from

Conversation

arieum
Copy link

@arieum arieum commented Aug 2, 2024

💁‍♀️Foreground_Custom_Notification 실행영상

foreground.mp4

💁‍♀️Background_Notification 실행영상

background.mp4

@mkSpace
Copy link

mkSpace commented Aug 3, 2024

리베이스 해주시고 Repository Pattern CodeLab 링크 드리니 참고해서 변경해보세요

https://developer.android.com/codelabs/basic-android-kotlin-training-repository-pattern?hl=ko#0

@arieum
Copy link
Author

arieum commented Aug 3, 2024

저 뭘 잘못했길래... STEP1 멘토님 코멘트가 사라진거죠 .... 😢

image

rebase가 잘못된걸까요?? 제가 직접 닫지 않았습니다 !!

@arieum
Copy link
Author

arieum commented Aug 3, 2024

⚠️ 수정 중입니다 ⚠️

💁‍♀️In Logviewmodel, runBlocking을 없애고 viewmodelscope으로 바꾸었습니다

  • runBlocking은 자칫 잘못하면 ui스레드를 오랫동안 블로킹하여 anr를 발생시킬 수 있는 여지가 있다는 점을 알고, viewmodelscope 코루틴으로 바꿨습니다

💁‍♀️suspendCoroutine 관련 ···

💁‍♀️rebase에 시간을 쓸데없이 너무 많이 써서.. (울고시펐슴민다...🥲그렇게 시간을 많이 쓴 결과가 이 모양입니다ㅠ죄송합니다ㅠ이번에 처음으로 rebase 쓴것같습니다....) 공유해주신 링크 이제 보고 있습니다...! 읽고 낼 아침전까지 다 수정하겠습니다 ★

💁‍♀️In welcomeViewActivity, map화면으로 이동 하기전 delay(1s)하기 위해 **lifecyclescope.launch{}**를 적용했습니다

  • delay( ) 중단 함수를 추가하기 위해 일단 lifecyclescope를 선언하긴 했는데...
  • "1초지연하고 -> 지도화면으로 이동 " 이 액션을 위해서 액티비티가 파괴되야 취소되는 lifecyclescope 사용은 너무 과한걸까요?
  • 흠.. 차라리 아래 코드가 나았을까요??
Handler(Looper.getMainLooper()).postDelayed({moveMapView()}, 1000)

💁‍♀️지금 제 코드는 viewmodel에 비지니스 로직이 많이 섞여있고, repository와 manager 클래스 간에 혼용이 있어서 재사용성이 떨어져보입니다.. 멘토님이 공유해주신 링크를 참고하라한 것도 이때문이었을 것 같습니다. 어제 나름 열심히 수정했다고 생각했는데 오늘 다시 보니 논점을 잘못 짚은것 같아요..

arieum added 4 commits August 4, 2024 01:03
- simplify Appdatabase class by removing companion object
- update databaseModule to directly build database instance
- modify logrepo to use appdatabase instead of placedao
- remove explicit dispatcher.io usage
Copy link

@mkSpace mkSpace left a comment

Choose a reason for hiding this comment

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

아림님 6주 동안 고생 많으셨습니다.

최종적으로 작성하신 코드들 보니 확실히 많이 깔끔해졌다는게 보이네요.

다만 Hilt, Room, Coroutine을 사용한 프로젝트는 거의 처음이시다 보니 아직 어색한 부분은 다소 보이는 것 같습니다.

이 부분은 너무 당연한 부분이니 제가 이전에 링크 드렸던 코드랩이나 여러 다른 잘 만든 레포들 보고 학습하시면 금방 실무와 가까운 코드를 작성하실거라 믿습니다.

리뷰 남겨드리니 확인 부탁드려요!


지금 현재 AndroidManifest에 작성하신 대로 Notification을 수신 받긴 할 텐데 기능 요구사항 중에 foreground 상태에서 수신 시 커스터마이징을 하라는 요구사항이 있어서요. 이게 제대로 동작하는지는 잘 모르겠네요.

이 부분을 커스터마이징 하려면 보통 FirebaseMessagingService에서 Background로 수신 받고 Notification을 띄우는 형태가 일반적인데 이를 활용한 부분은 누락되어 있는것 같아서 여쭤봅니다

Comment on lines 85 to 86
.setTitle("알림권한이 필요합니다")
.setMessage("This app needs notification permission to send you updates and important information")
Copy link

Choose a reason for hiding this comment

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

이런 문자열은 strings.xml로 다 빼주세요

@mkSpace
Copy link

mkSpace commented Aug 4, 2024

⚠️ 수정 중입니다 ⚠️

💁‍♀️In Logviewmodel, runBlocking을 없애고 viewmodelscope으로 바꾸었습니다

  • runBlocking은 자칫 잘못하면 ui스레드를 오랫동안 블로킹하여 anr를 발생시킬 수 있는 여지가 있다는 점을 알고, viewmodelscope 코루틴으로 바꿨습니다

💁‍♀️suspendCoroutine 관련 ···

💁‍♀️rebase에 시간을 쓸데없이 너무 많이 써서.. (울고시펐슴민다...🥲그렇게 시간을 많이 쓴 결과가 이 모양입니다ㅠ죄송합니다ㅠ이번에 처음으로 rebase 쓴것같습니다....) 공유해주신 링크 이제 보고 있습니다...! 읽고 낼 아침전까지 다 수정하겠습니다 ★

💁‍♀️In welcomeViewActivity, map화면으로 이동 하기전 delay(1s)하기 위해 **lifecyclescope.launch{}**를 적용했습니다

  • delay( ) 중단 함수를 추가하기 위해 일단 lifecyclescope를 선언하긴 했는데...
  • "1초지연하고 -> 지도화면으로 이동 " 이 액션을 위해서 액티비티가 파괴되야 취소되는 lifecyclescope 사용은 너무 과한걸까요?
  • 흠.. 차라리 아래 코드가 나았을까요??
Handler(Looper.getMainLooper()).postDelayed({moveMapView()}, 1000)

💁‍♀️지금 제 코드는 viewmodel에 비지니스 로직이 많이 섞여있고, repository와 manager 클래스 간에 혼용이 있어서 재사용성이 떨어져보입니다.. 멘토님이 공유해주신 링크를 참고하라한 것도 이때문이었을 것 같습니다. 어제 나름 열심히 수정했다고 생각했는데 오늘 다시 보니 논점을 잘못 짚은것 같아요..

제가 질문 형태로 리뷰 드렸던 부분은 상당 부분 알고 사용하시는지 궁금해서 여쭤본 거였구요. 해당하는 코드가 정말 자신이 생각하는대로 동작하는지 제대로 따져봤음 해서 리뷰 드린거였어요.

코드를 작성하실 때 여러 블로그 등을 참조하실 텐데 그 코드들이 과연 제대로 동작하는가? 현재 상황에서 최선의 방법인가? 하는 의문을 항상 갖고 작성하신다면 좋은 코드를 작성하시는데 도움이 될거라 생각이 듭니다.

@arieum
Copy link
Author

arieum commented Aug 4, 2024

최종 수정본 이후 실행영상입니다!

Foreground일때도 firebasemessage를 수신할 수 있도록 수정했습니다!
(제가 과제를 잘못 이해했던것같아요.. foreground일때는 firebasemessage없이도 notification이 울리도록 하는 건 줄 알았습니다..🥲)

fcm.mp4

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