-
Notifications
You must be signed in to change notification settings - Fork 1
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: AlarmManager 작업들 Local DB에 저장 #762
base: develop
Are you sure you want to change the base?
Conversation
This reverts commit 05a009a.
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.
오디 안드들 진짜 멋져요👍 너무 야무져요 🫒
테이블도 만들고 쿼리도 만들고.. 그럼 얼마 안 남은 김에 안드3 백엔드7로 가시죠🤓
코드에서 멋이 좔좔 흘러서 approve 남기고 갑니다!ㅎㅎ
@Insert(onConflict = OnConflictStrategy.REPLACE) | ||
suspend fun save(etaReservationEntity: EtaReservationEntity): Long | ||
|
||
@Query("DELETE FROM eta_reservation WHERE meetingId = :meetingId") |
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.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 이 사람 또 안드 코드 훔쳐보네... 💓 approve 받았다 히히
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 isOpen: Boolean, | ||
) { | ||
@PrimaryKey(autoGenerate = true) | ||
var id: Long = 0 |
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.
@PrimaryKey를 meetingId로 할 수도 있는데 id로 해준 이유 궁급하니다!
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.
하나의 meeting에 "eta 폴링을 시작시키는 알림", "eta 폴링을 종료시키는 알림" 두개가 존재합니다! 즉, meetingId
가 두개씩 중복되기 때문에 PrimaryKey로는 적합하지 않다고 생각했어요. meetingId
, reserveMiliis
이 두개의 필드를 복합키로 설정할지도 생각했었는데요. id
를 따로 설정해서, AlarmManage�r
의 requestCode
(Alarm을 구분하는 id 같은 값)를 Entity의 id
로 설정하면 좋을 것 같아 이렇게 구현했습니다!
abstract fun etaReservationDao(): EtaReservationDao | ||
|
||
companion object { | ||
val MIGRATION_3_4 = |
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 Room.databaseBuilder(context, OdyDatabase::class.java, "ody_db") .addMigrations(MIGRATION_3_4) .addTypeConverter(MateEtaListTypeConverter(moshi)) .build()
MIGRATION_3_4 처럼 "ody_db"도 상수화 어떤가요
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.
그리고 MIGRATION_3_TO_4 변수명 어떤가요
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.
좋아요! 수정했습니다 Database 만들어주는 부분도 OdyDatabase.create()로 호출할 수 있도록 함수 분리했습니다. 76d0098
|
||
class EtaDashboardCloseBroadcastReceiver : BroadcastReceiver() { | ||
@Inject | ||
lateinit var matesEtaRepository: MatesEtaRepository |
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.
@Inject 프로퍼티에는 private은 못 붙나요?
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.
넵 Hilt로 필드 주입하려면 public이어야 합니다!!
로그아웃 후 재로그인 할 때요, |
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.
수고하셨습니다 올리브!!
reservationId: Long, | ||
): PendingIntent { | ||
val intentClass = | ||
if (isOpen) EtaDashboardOpenBroadcastReceiver::class else EtaDashboardCloseBroadcastReceiver::class |
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.
Eta ~ Receiver 의 이름이 길었군요..
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.
BroadcastReceiver 자체가 너무 길어서 그런 것 같아요. 아니면 Dashboard를 지워도 좋을 것 같은데 일단 다른 코드의 통일성을 위해 냅뒀습니다.
if (!isReservationPending) { | ||
etaReservationDao.deleteAll() | ||
} | ||
val serviceIntent = Intent(context, EtaDashboardService::class.java) |
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.
Activity에서 getIntent 쓰는것처럼
EtaDashboardService.getIntent(context) 식으로 만들어보는건 어떤가요
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.
이미 EtaDashboardService
에 getIntent
가 있긴 한데요. action과 meetingId를 넘겨줘야 하는 getIntent
라서 저도 이부분 고민하긴 했습니다.. action과 meetingId를 nullable로 두는 방식으로 수정했습니다! adb0380
|
||
class DefaultMatesEtaRepository | ||
@Inject | ||
constructor( | ||
@ApplicationContext private val context: Context, |
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 context를 repository에서 그대로 들고가는거 괜찮을까요..
EtaDashboardService를 주입받고, EtaDashboardService에 stopService같은 메서드를 구현(메서드 내부에서는 stopSelf를 호출)해서 사용하는 방법은 혹시 가능할까요?
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.
저는 이 repository 구현체 자체가 안드로이드 의존성이 있고(AlarmManager
, Room
), local 패키지에 있기 때문에 applicationContext
를 가지고 있는 것도 괜찮은 것 같아요. 이 repository가 activity 생명주기에 따라 생성/소멸되는 것이 아닌 singleton 인스턴스로 애플리케이션 전역에서 살아있기 때문에 메모리 누수도 발생할 것 같지는 않습니다.
EtaDashboardService를 주입받고, EtaDashboardService에 stopService같은 메서드를 구현(메서드 내부에서는 stopSelf를 호출)해서 사용하는 방법은 혹시 가능할까요?
EtaDashboardService
인스턴스는 안드로이드 프레임워크에서 생성해주고 있고, 항상 Intent
를 통해서만 EtaDashboardService
를 접근할 수 있습니다. EtaDashboardService
인스턴스 자체를 직접적으로 접근할 수 있는 방법이 없기 때문에 현재 구조에서 개선할 수 있는 방법은 떠오르지 않네요. 🥹🥹 일단 이대로 머지하겠습니다!
🚩 연관 이슈
close #760
📝 작업 내용
🏞️ 스크린샷 (선택)
🗣️ 리뷰 요구사항 (선택)
구현한 내용 관련 시나리오입니다~! 변경사항이 초큼 많은데 이해 안 되는 부분 있으면 말해주세용
아래 사항들 모두 테스트했는데 문제 없었습니다!
현재 코드의 한계점