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

[#5] 지역코드/시군구코드 로컬 저장 #6

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Conversation

ksw4015
Copy link
Collaborator

@ksw4015 ksw4015 commented Oct 2, 2024

변경사항

  • HiltWorker를 사용하기 위한 의존성 추가와 Application class의 수정이 있습니다.
  • AreaCode와 SigunguCode를 요청하고 Room을 통해 저장하는 AreaWorker Class가 추가되었습니다.
  • SplashViewModel에서 AreaWorker를 실행합니다.

멘토님께

  • SplashViewModel의 Log는 삭제예정입니다.
  • Worker Class와 WorkManager를 사용해보는 것이 이번이 처음이라 중점적으로 봐주시면 좋을것 같습니다.

@ksw4015 ksw4015 self-assigned this Oct 2, 2024
Copy link

@f-lab-Toru f-lab-Toru left a comment

Choose a reason for hiding this comment

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

코멘트 추가 완료

@@ -37,6 +40,16 @@ object DataModule {
.build()
}

@Provides
@Singleton
fun provideAreaCodeDatabase(application: Application): AreaCodeDatabase {

Choose a reason for hiding this comment

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

fun provideAreaCodeDatabase(@ApplicationContext context: Context): AreaCodeDatabase {
    ...
}

로 해도 좋을 것 같아요. predefined binding 에 대한 qualifier 입니다.

Copy link
Collaborator Author

@ksw4015 ksw4015 Oct 5, 2024

Choose a reason for hiding this comment

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

@ApplicationContext의 경우 생성자 주입으로 context를 제공하는 것이 아니라서 그런지 추가하면 Missing Binds 에러가 발생하네요

@PrimaryKey(autoGenerate = true)
val id: Int? = null,
val code: String,
val name: String

Choose a reason for hiding this comment

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

trailing comma 를 넣어 주면 좋을 거 같아요

val id: Int? = null,
val areaCode: String,
val code: String,
val name: String

Choose a reason for hiding this comment

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

마찬가지로 trailing comma 를 넣어 주면 좋을 것 같습니다.

return Result.success()
}

override suspend fun getForegroundInfo(): ForegroundInfo {

Choose a reason for hiding this comment

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

ForegroundInfo 가 뭔가 해서 찾아봤는데, Workmanager 가 12 이전 버전에서 사용될 때 ForegroundService 를 이용하기 위한 거 같네요. 혹시 앱을 닫아도 계속적으로 실행되어야 할 만큼 doWork() 내부의 구현이 오래 걸리는 걸까요? 이건 그냥 궁금해서 여쭤 보는 거예요 :) 그리고 getForegroundInfo() 와 관련된 메서드로 setExpedited() 가 있는데, 이것의 예시도 참고해 보세요. 말 그대로 Workmanager가 신속하게 동작하도록 하는 메서드입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

참고했던 예제에서 override를 하고있어서 필수로 override해야하는 함수인줄 알았습니다. 지금 보니 getForegroundInfo는 일반 open 함수라 ForegroundService까지 이용할 필요가없으면 굳이 override하지 않아도 될 것같네요! 지우는게 나을것 같습니다.

viewModelScope.launch {
val areaCodeItems = areaCodeDatabase.areaCodeDao.getAllAreaCodeEntities()
if(areaCodeItems.isEmpty()) {
val workRequest = OneTimeWorkRequestBuilder<AreaCodeWorker>().build()

Choose a reason for hiding this comment

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

위에 리뷰를 달았는데, workRequest 를 만들 때 따로 설정해줘야 하는 게 없다면 from 이라는 정적메소드를 사용해도 될 것 같습니다.

val myWorkRequest = OneTimeWorkRequest.from(MyWork::class.java)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정했습니다!

@@ -29,6 +29,7 @@ class SplashActivity : ComponentActivity() {

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
viewModel.initAreaCode(applicationContext)

Choose a reason for hiding this comment

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

스플래시화면에서 viewModel.initAreaCode()setContentView{} 이후에 불려도 되지 않을까 하는 생각이 들긴 해요. 아마 큰 차이는 없을 거 같지만요. 이건 의견을 좀 달아 주시면 좋을 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setContent 이후에 있어도 좋을것 같습니다

@ksw4015 ksw4015 merged commit 1cea513 into main Oct 7, 2024
1 check failed
@ksw4015 ksw4015 deleted the feature/5 branch October 7, 2024 10:31
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