[feature/#835] 🚌 알림 화면 리팩토링 #837

l2hyunwoo · 2024-09-08T02:15:56Z

Issue

close [Refactor] 알림 화면 Compose로 리팩토링 #835

What is this issue?

알림 모듈들 domain, data 모듈로 빼버리기
화면 Compose로 마이그레이션
FirebaseMessagingService를 LifecycleAwareFirebaseMessagingService 붙이기

height · 2024-09-08T02:15:58Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

l2hyunwoo · 2024-09-08T14:34:17Z

core/common/src/main/java/org/sopt/official/common/navigator/DeepLinkType.kt

+internal val navigator by lazy {
+  EntryPointAccessors.fromApplication(appContext, NavigatorEntryPoint::class.java).navigatorProvider()
+}


유용한 팁

navigator를 일반 변수로 사용하기 위해서 EntryPoint를 활용하여 가져오고 있습니다. 힐트 모듈에 저장되어있는 값들은 EntryPoint 활용하면 그냥 꺼내서 쓸 수 있습니다.

concern point

SingletonModule에서 꺼내오기 때문에 applicationContext를 활용해야합니다. 이때 applicationContext를 별도의 주입과정필요 없이 가져오려고 하기 때문에 appContext 변수로 만들어서 가져옵니다.

애초에 applicationContext는 전역 싱글톤이기 때문에 이렇게 가져와도 큰 문제점은 없다고 판단됩니다.

APP 클래스의 context를 가져오지 않는 이유는 App객체 생성 비용을 줄이기 위함이고, 힐트를 사용하지 않는 이유는 탐색 및 주입 과정을 거치지 않기 위해서 인가요?

@s9hn

저 모듈에서 App 클래스 참조하면 순환참조 발생합니다.

저 거대한 로직을 클래스 형태로 분리해서 주입받을 시간보다 service locator 형태로 일단 돌아갈 수 있게 코드를 개편해서 정리하는게 더 빨리 문제를 해결할 수 있겠다는 판단입니다. 그런데 반드시 수정되어야 하는 로직이에요.

아하 모듈이 달랏군요 b

chattymin

어우 손 빠르시네요
고생하셨슴담

근데 언제 또 버전올리셨대요... 8.5라 실행안돼서 당황하다 업뎃 햇네요 ㅋㅋㅋ

data/notification/src/main/java/org/sopt/official/data/notification/di/NotificationModule.kt

.editorconfig

...on/src/main/java/org/sopt/official/feature/notification/detail/NotificationDetailActivity.kt

chattymin · 2024-09-08T20:10:31Z

...otification/src/main/java/org/sopt/official/feature/notification/all/NotificationActivity.kt

+      SoptTheme {
+        Scaffold(modifier = Modifier


저는 Activity에는 Composable을 이용한 화면을 놔두기 보다는 따로 추출하는 편인데 같이 쓰신 이유가 있을까요??
Activity에서는 각종 처리들을 하고, Screen과 같은 Composable을 통한 View로서 분리해두는게 가독성이 좋고 나중에 추적하기가 편하다 생각해서 저는 그렇게 하는 편입니당

재사용되는 컴포저블이 거의 없어서 그냥 한번에 컴포저블을 써내려간건데 생각해보니 가독성 이슈가 있군요. 한번 분리 해보도록 하겠습니다.

leeeyubin

속도가 엄청 빠르시군여.. 역시 최고입니다!! 수고하셨어요!!!!!

...otification/src/main/java/org/sopt/official/feature/notification/all/NotificationActivity.kt

...ication/src/main/java/org/sopt/official/feature/notification/all/NotificationPagingSource.kt

s9hn

2천줄 그래도 다 봤읍니다 .. 고생티비
개인적으로 ,, 저는 가독성 충이라 가능한 분리하긴해요 코드만 보고도 어떤 화면 구성인지 알 수 있었음해서.. 물론 재사용성도 고려하고
이건 모 스타일 차이인듯

s9hn · 2024-09-09T07:47:59Z

app/src/main/java/org/sopt/official/feature/notification/SchemeActivity.kt

-    private val args by serializableExtra(StartArgs("", ""))
+  @Inject
+  lateinit var dataStore: SoptDataStore
+  private val args by serializableExtra(Argument("", ""))


args 사용 부분을 봐야 무슨 상태값인지 이해가 가네요 좀 더 구체적인 네이밍도 좋겠어요 :)

navigation argument여서 이렇게 지은 것이긴 해요 그리고 이 args내의 변수가 어떤의미인지 나타내기때문에 args 변수 네이밍이 그렇게 나쁜 것 같지 않아요

이현우 요원님 조영호 선생님께서 찾으십니다

app/src/main/java/org/sopt/official/feature/notification/SchemeActivity.kt

s9hn · 2024-09-09T08:06:04Z

core/common/src/main/java/org/sopt/official/common/navigator/DeepLinkType.kt

+internal val navigator by lazy {
+  EntryPointAccessors.fromApplication(appContext, NavigatorEntryPoint::class.java).navigatorProvider()
+}


APP 클래스의 context를 가져오지 않는 이유는 App객체 생성 비용을 줄이기 위함이고, 힐트를 사용하지 않는 이유는 탐색 및 주입 과정을 거치지 않기 위해서 인가요?

...otification/src/main/java/org/sopt/official/data/notification/service/NotificationService.kt

...otification/src/main/java/org/sopt/official/feature/notification/all/NotificationActivity.kt

s9hn · 2024-09-09T08:16:27Z

...otification/src/main/java/org/sopt/official/feature/notification/all/NotificationActivity.kt

+              navigationIcon = {
+                IconButton(onClick = { onBackPressedDispatcher.onBackPressed() }) {
+                  Icon(
+                    Icons.AutoMirrored.Filled.ArrowBack,


이 친구만 named call argument 없서요

밑에도 많ㄴ..

(귀찮음 이슈) 빨리빨리 해버릇한게 다 들통나버린 느낌....제대로 붙이겠나이다

...otification/src/main/java/org/sopt/official/feature/notification/all/NotificationActivity.kt

...n/src/main/java/org/sopt/official/feature/notification/detail/NotificationDetailViewModel.kt

…eActivity.kt Co-authored-by: SeHun <[email protected]>

l2hyunwoo self-assigned this Sep 8, 2024

l2hyunwoo requested a review from a team as a code owner September 8, 2024 02:15

pull-request-size bot added the size/XXL label Sep 8, 2024

l2hyunwoo force-pushed the feature/#835 branch from 486cffe to 9ae3acc Compare September 8, 2024 02:50

l2hyunwoo commented Sep 8, 2024

View reviewed changes

chattymin approved these changes Sep 8, 2024

View reviewed changes

data/notification/src/main/java/org/sopt/official/data/notification/di/NotificationModule.kt Show resolved Hide resolved

.editorconfig Outdated Show resolved Hide resolved

...on/src/main/java/org/sopt/official/feature/notification/detail/NotificationDetailActivity.kt Show resolved Hide resolved

chattymin reviewed Sep 8, 2024

View reviewed changes

leeeyubin approved these changes Sep 9, 2024

View reviewed changes

s9hn approved these changes Sep 9, 2024

View reviewed changes

l2hyunwoo force-pushed the feature/#835 branch 3 times, most recently from b2af65a to 07a511e Compare September 14, 2024 03:53

l2hyunwoo added 17 commits September 18, 2024 21:42

[feat/#835] Add domain/notification module

a75632b

[feature/#835] Add data notification module

4379824

[feature/#835] Spotless Apply

74fcc33

[feature/#835] Add NotificationRepository impl in data module

f7769f8

[feature/#835] Replace notification module with new impl

dfa774d

[feature/#835] Remove useless usecases

b403023

[feature/#835] Remove useless files

9e9c827

[feature/#835] Apply LifecycleAwareFirebaseMessagingService

cd656db

[feature/#835] Spotless Apply

f02d709

[feautre/#835] Add feature notification module

16eb406

[feature/#835] Remove viewmodels

19db89c

[feature/#835] Spotless apply

d0d0a4c

[feature/#835] NotificationHistory -> Notification

2dc16c6

[feature/#835] Add Navigator Intent Provider

3712d83

[feature/#835] Add appContext props && remove useless code

b5ea5ba

[feature/#835] Refactor deeplink logic

3dd977b

[feature/#835] Spotless Apply

49a7540

l2hyunwoo and others added 14 commits September 18, 2024 21:42

[feature/#835] Fix build issues

094f858

[feature/#835] Reafactor code

dfd81a9

[feature/#835] Fix build issue

3142689

[feature/#835] Convert notification activity to compose

c99cfc1

[feature/#835] Add paging dependency

60c2cd6

[feature/#835] Apply paging3 + compose

0602694

[feature/#835] Spotless Apply

49d7728

[feature/#835] Remove useless usecases

ec43701

[feature/#835] Fix viewmodel naming

799ae5f

[feature/#835] Refactor NotificationDetailActivity

4d3c0c5

[feature/#835] Fix build issue

d3af694

[feature/#835] Refactor with compose

6fc26e2

Update app/src/main/java/org/sopt/official/feature/notification/Schem…

7f173a4

…eActivity.kt Co-authored-by: SeHun <[email protected]>

Update .editorconfig

a119b3d

l2hyunwoo force-pushed the feature/#835 branch from 07a511e to a119b3d Compare September 18, 2024 12:42

l2hyunwoo merged commit 81d1b52 into develop Sep 18, 2024
1 check passed

l2hyunwoo deleted the feature/#835 branch September 18, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature/#835] 🚌 알림 화면 리팩토링 #837

[feature/#835] 🚌 알림 화면 리팩토링 #837

l2hyunwoo commented Sep 8, 2024 •

edited

Loading

height bot commented Sep 8, 2024

l2hyunwoo Sep 8, 2024

s9hn Sep 9, 2024

l2hyunwoo Sep 9, 2024

s9hn Sep 9, 2024

chattymin left a comment •

edited

Loading

chattymin Sep 8, 2024

l2hyunwoo Sep 8, 2024

leeeyubin left a comment

s9hn left a comment •

edited

Loading

s9hn Sep 9, 2024 •

edited

Loading

l2hyunwoo Sep 9, 2024

s9hn Sep 9, 2024

s9hn Sep 9, 2024

s9hn Sep 9, 2024

s9hn Sep 9, 2024

l2hyunwoo Sep 9, 2024

[feature/#835] 🚌 알림 화면 리팩토링 #837

[feature/#835] 🚌 알림 화면 리팩토링 #837

Conversation

l2hyunwoo commented Sep 8, 2024 • edited Loading

Issue

What is this issue?

height bot commented Sep 8, 2024

Choose a reason for hiding this comment

유용한 팁

concern point

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chattymin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeeyubin left a comment

Choose a reason for hiding this comment

s9hn left a comment • edited Loading

Choose a reason for hiding this comment

s9hn Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l2hyunwoo commented Sep 8, 2024 •

edited

Loading

chattymin left a comment •

edited

Loading

s9hn left a comment •

edited

Loading

s9hn Sep 9, 2024 •

edited

Loading