-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] 새로운 Intent 구조 적용 #400
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
base: develop
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @PeraSite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Intent에 하드코딩된 문자열 키를 사용하여 데이터를 전달하는 기존 방식을 개선하기 위해 새로운 Intent 구조를 도입합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Pull Request Overview
This PR refactors the Intent handling mechanism from hardcoded string keys to a type-safe structure using Parcelable data classes and a generic ActivityCompanion base class. The refactoring is demonstrated with IntroActivity, replacing the string-based "launch_path" extra with a structured IntentOptions approach.
Key Changes:
- Introduced
ActivityCompanionbase class for type-safe Intent creation and parameter extraction - Applied new Intent structure to
IntroActivitywithIntentOptionsdata class - Updated all
IntroActivitylaunch points (widget, local notifications, remote notifications) to use the new API
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ActivityCompanion.kt |
New base class providing type-safe Intent creation and options extraction |
IntroActivity.kt |
Adopted new Intent structure with IntentOptions data class and simplified launch path handling |
MealWidget.kt |
Updated to use IntroActivity.start() with type-safe IntentOptions |
NotificationReceiver.kt |
Migrated local notification Intent creation to new structure |
EatSsuFirebaseMessagingService.kt |
Migrated remote notification Intent creation to new structure |
launchApp.kt |
Removed deprecated widget launch utility function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/src/main/java/com/eatssu/android/presentation/base/ActivityCompanion.kt
Outdated
Show resolved
Hide resolved
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.
Code Review
안녕하세요! Intent 구조를 개선하기 위한 리팩토링 PR 잘 보았습니다. 하드코딩된 문자열 키를 사용하는 대신 Parcelable 데이터 클래스와 ActivityCompanion 패턴을 도입하여 타입 안정성을 크게 향상시킨 점이 매우 인상적입니다. 이 새로운 구조는 코드의 가독성을 높이고 잠재적인 런타임 오류를 줄이는 데 큰 도움이 될 것입니다.
PR 설명에서 질문주신 것처럼, 이 구조를 다른 모든 Activity에도 점진적으로 적용하는 것을 적극 추천합니다. 일관성 있는 구조는 프로젝트 전체의 유지보수성을 향상시킬 것입니다.
한 가지 개선 제안을 ActivityCompanion.kt 파일에 남겼습니다. 확인 부탁드립니다.
app/src/main/java/com/eatssu/android/presentation/base/ActivityCompanion.kt
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/src/main/java/com/eatssu/android/presentation/mypage/userinfo/UserInfoActivity.kt
Show resolved
Hide resolved
...rc/main/java/com/eatssu/android/presentation/cafeteria/review/modify/ModifyReviewActivity.kt
Show resolved
Hide resolved
HI-JIN2
left a comment
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 확장함수를 만들어 사용하는 것이 보편적인 안드로이드 엔지니어가 행하는 방법인가를 조금 고민해보아야할 것 같아유...
게다가 컴포즈 네비게이션으로 점차 넘어가게 되면 intent의 사용비율이 되게 많이 줄어들거라서 한 프로젝트 안에서 엄청 반복되는 코드가 아닌데 이걸 확장함수로 만들어야하나 라는 생각도 조금은 들구요
위의 코멘트와 별개로, 제가 보기에도 이 코드는 마음에 들지 않아서 상수화는 적극 도입하면 좋을 것 같습니다!
val intent = Intent(this, IntroActivity::class.java).apply {
putExtra("launch_path", "remote_notification")
진님께서 우려스럽다고 말씀해주신 부분에 대해서 저도 충분히 고민해보았는데요, 캄포즈로 모든 액티비티를 다 마이그레이션하는 것이 말씀해주신 것처럼 최고의 방법임은 틀림없으나 상당히 오랜 시간이 걸려야 마이그레이션이 진행 될 것 같아요. 그 전까지 새로운 뷰를 만들면서 Intent extra를 새롭게 설계해야할 수도 있으니 미리미리 진행해두면 좋을 것 같습니다! |
Summary
하드코딩된 문자열을 Key로 사용하고, 값이 잘 들어왔기를 바래야하는 Intent 구조를 개선합니다.
getParcelableExtra와 putExtra(String, Parcelable)를 사용해 직렬화에서 오는 오버헤드는 있겠습니다만 코드 퀄리티가 크게 개선됩니다.Describe your changes
Intent Extra가 필요한 Activity 내부에 Extra 값을 표현하는 Parcelable data class와 ActivityCompanion를 상속받는 companion object를 선언합니다.
이후 Activity 내부에서 Intent 값을 참조할 때는 다음과 같은 방식을 사용합니다.
새 Intent를 만들어서 startActivity할 때는 다음과 같은 방식을 사용합니다.
기존 Intent.apply {}로 적용하던 flag 처리도 한번에 할 수 있습니다.
Activity 내부에서는 여전히 Intent 값이 없을 경우에 대응해야 하지만, 외부에서는 무조건 IntentOptions을 만들어야하기 떄문에 훨씬 깔끔하다고 생각합니다!
Issue
To reviewers