Feature/explore #22

Guri999 · 2024-12-08T09:08:58Z

Explore 화면
LocalStorage 접근

f-lab-dean

PR 생성 시 효과적이고 의미있는 리뷰를 위해 다음과 같은 부분을 고려해주시면 좋겠습니다.

PR의 제목은 이 PR에서 어떤 작업이 있었는지, 무엇을 중점으로 보아야 하는지를 요약합니다. 따라서 리뷰어는 어떤 작업이 있을지 예상이 가능합니다. Feature/explore와 같은 제목은 리뷰어에게 그 어떤 단서도 주어지지 않습니다.
PR의 내용은 작업물의 내용에 대한 상세를 작성해주시기 바랍니다. 이러한 내용을 토대로 작업물의 이해도를 높이고, 어떤 부분을 고려하면 좋을지 리뷰어에게 단서를 제공합니다. 리뷰를 하는 사람은 현재 PR이 어떤 작업인지 컨텍스트가 전혀 없다는 것을 고려하여 가능한 간결하고 정확한 데이터를 제공해주어야 합니다.
하나의 PR은 하나의 작업만을 포함하는게 좋습니다. 현재의 PR은 작업 내용이 다양하고, 커밋이 많습니다. 이것은 리뷰어에게 혼란을 주기 쉽습니다. 일반적으로 효율적인 리뷰를 위한 변화량은 400라인을 권장합니다.
일관적인 PR 생성을 위해 템플릿 작성을 권장합니다.

f-lab-dean · 2024-12-08T13:59:51Z

app/src/main/java/kr/co/seedocs/MainActivity.kt


+        if (checkStoragePermission(this)) {


onResume에서 storage permission을 체크하여 setContent를 호출하는 것은 바람직하지 않은 구조같습니다.
권한에 따라 UI의 상태가 변경되어야 한다면, 그에 따른 UI 상태를 Composable에 넘겨주어 리컴포지션할 수 있도록 구조를 변경하는게 좋을 것 같습니다.

추상 클래스로 변경했습니다

f-lab-dean · 2024-12-08T14:01:20Z

app/src/main/java/kr/co/seedocs/PermissionUtil.kt

+
+internal fun checkStoragePermission(
+    activity: ComponentActivity,
+): Boolean =


Util함수로 구현하는 것 보다, 외부 스토리지 권한 로직을 캡슐화한 클래스 또는 인터페이스로 제공하는 것이 좋을 것 같습니다.

추상 클래스로 변경 했습니다

f-lab-dean · 2024-12-08T14:01:42Z

app/src/main/java/kr/co/seedocs/PermissionUtil.kt

+    activity.startActivityForResult(intent, MANAGE_EXTERNAL_STORAGE_PERMISSION_REQUEST)
+}
+
+fun requestStoragePermissionApi19(activity: ComponentActivity) {


이 함수가 public인 이유가 무엇일까요?

단순 실수였습니다 바로 위 PR에서 수정했습니다

f-lab-dean · 2024-12-08T14:02:13Z

core/ui/src/main/java/kr/co/ui/widget/FileBox.kt

+                .clearAndSetSemantics { },
+            imageVector = SeeDocsIcon.PDF,
+            contentDescription = null,
+            tint = Color.Unspecified


tint는 Color.Unspecified로 적용되는 것이 맞나요?

네 혹시 뭔가 이상한점이 있나요?

구현의 의도, 실수를 확인하기 위한 질문입니다.
보통 tint 파라미터를 넘기는 경우는 특정한 컬러로 표현하기 위함인데, Color.Unspecified 를 지정하는 이유가 궁금해서요.

f-lab-dean · 2024-12-08T14:02:20Z

core/ui/src/main/java/kr/co/ui/widget/FileBox.kt

+                color = Theme.colors.text
+            )
+            Text(
+                text = "2023.01.01",


하드코딩된 이유가 궁금합니다.

외부로 부터 데이터를 받을 부분이라 UI를 보기위한 더미 데이터입니다.

기능 구현 이후 수정하였습니다.

나중에 구현이 되어야할 부분이라면 TODO 코멘트를 남겨주시는 것이 좋을 것 같습니다.

f-lab-dean · 2024-12-08T14:04:56Z

feature/explore/src/main/java/kr/co/explore/ExploreScreen.kt

+private fun readPDFOrDirectory(
+    path: String,
+): List<Item> =
+    File(path).listFiles()?.filter { !it.isHidden && (it.isDirectory || it.extension == "pdf") }?.map {


이 함수를 UI 쓰레드에서 호출해도 괜찮을까요?

백그라운드 스레드에서 호출하는게 맞을 것 같네요

네, TODO 등의 코멘트를 달고 다른 PR에서 수정하시면 될 것 같습니다.

f-lab-dean · 2024-12-08T14:05:18Z

feature/explore/src/main/java/kr/co/explore/ExploreScreen.kt

+        )
+    }?: emptyList()
+
+private data class Item(


UI 모델이라면 분리된 파일 또는 패키지에 위치하는 것은 어떤가요?

여러 화면에서 사용될 모델 이라 생각해서 임시로 안에 적은 뒤
다른 화면 로컬 파일 설정등을 하며 데이터 윤곽이 잡히면 수정할 계획이었습니다.

f-lab-dean · 2024-12-08T14:05:44Z

feature/explore/src/main/java/kr/co/explore/ExploreViewModel.kt

+
+import androidx.lifecycle.ViewModel
+import dagger.hilt.android.lifecycle.HiltViewModel
+import javax.inject.Inject


미완성된 또는 미사용되는 코드는 포함하지 않아도 될 것 같습니다.

f-lab-dean · 2024-12-08T14:07:25Z

feature/main/src/main/java/kr/co/main/MainBottomBar.kt

+                    ),
+            ) {
+                tabs.forEach { tab ->
+                    Item(


BottomTabItem과 같이 명확한 이름을 주는 것은 어떤가요?

수정하겠습니다

f-lab-dean · 2024-12-08T14:08:39Z

.idea/gradle.xml

@@ -37,6 +37,7 @@
            <option value="$PROJECT_DIR$/feature/bookmark" />
            <option value="$PROJECT_DIR$/feature/explore" />
            <option value="$PROJECT_DIR$/feature/main" />
+            <option value="$PROJECT_DIR$/feature/pdf" />


이 파일은 git에 올릴 필요가 있는 파일인가요?

필요없는걸 같이 PR 한것 같습니다

f-lab-dean · 2024-12-08T14:13:28Z

Quality Gate failed

Failed conditions 1 Security Hotspot 9.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@Guri999 SonarQube 결과가 실패인 이유를 확인하고 수정해주세요.

… feature/explore # Conflicts: # core/ui/src/main/java/kr/co/ui/widget/FileBox.kt # feature/main/src/main/java/kr/co/main/navigation/MainNavHost.kt

sonarqubecloud · 2024-12-24T14:07:07Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

f-lab-dean

LGTM + 코멘트를 확인해주세요.
꼭 반영해야할 필요는 없습니다만, 한번 고민해보시면 좋을 것 같아요.

f-lab-dean · 2024-12-24T14:10:48Z

app/src/main/java/kr/co/seedocs/PermissionActivity.kt

+import androidx.core.app.ActivityCompat
+import androidx.core.content.ContextCompat
+
+abstract class PermissionActivity : ComponentActivity() {


추상화된 함수를 제공하지 않는다면 open class가 더 어울릴 것 같습니다.

f-lab-dean · 2024-12-24T14:11:03Z

app/src/main/java/kr/co/seedocs/PermissionActivity.kt

+
+abstract class PermissionActivity : ComponentActivity() {
+
+    internal fun ComponentActivity.checkStoragePermission(): Boolean =


함수들이 이미 ComponentActivity 내에 있으므로 확장함수로 구현하지 않아도 될 것 같아요.

f-lab-dean · 2024-12-24T14:11:40Z

app/src/main/java/kr/co/seedocs/PermissionActivity.kt

+import androidx.core.app.ActivityCompat
+import androidx.core.content.ContextCompat
+
+abstract class PermissionActivity : ComponentActivity() {


권한이 Storage permission으로 한정되어 있으므로 이름을 명확하게 하는것이 좋을 것 같아요.

f-lab-dean · 2024-12-24T14:12:13Z

app/src/main/java/kr/co/seedocs/PermissionActivity.kt

+import androidx.core.app.ActivityCompat
+import androidx.core.content.ContextCompat
+
+abstract class PermissionActivity : ComponentActivity() {


어차피 캡슐화를 할 것이라면, 스토리지 권한만을 위한 로직의 책임을 가진 class로 만들고, 사용하는 곳에서 활용하도록 하는것도 좋을 것 같아요.

Guri999 added 3 commits December 3, 2024 20:48

Feat: 파일 탐색 권한 요청

0d8ae9e

Feat: 모든 파일 접근 권한 설정

55f9402

Feat: Explore 화면 파일 탐색 및 표시

c565a59

Guri999 added the ✨ feat 기능 개발 label Dec 8, 2024

Guri999 self-assigned this Dec 8, 2024

f-lab-dean suggested changes Dec 8, 2024

View reviewed changes

Guri999 added 10 commits December 16, 2024 13:06

Fix: 화면 구현을 onResume -> onCreate로 변경

72feff4

Fix: TODO 표기

465c1e0

Fix: SonarQube Error "READ_EXTERNAL_STORAGE" permission is necessary

c7cbefd

Fix: Bottom Navigation Item 이름 변경

24a247c

Fix: TODO 코멘트

8face98

Fix: contentPadding으로 변경

c808aa9

Fix: 하드코딩 텍스트 스트링 리소스로 변경

c895fb4

Merge branch 'main' of https://github.com/f-lab-edu/see-docs-app into…

ff157e5

… feature/explore # Conflicts: # core/ui/src/main/java/kr/co/ui/widget/FileBox.kt # feature/main/src/main/java/kr/co/main/navigation/MainNavHost.kt

Refactor: Change permission logic to an extension function

0f5bc9b

Refactor: Change permissionUtil to abstract class

5bb46d4

f-lab-dean approved these changes Dec 24, 2024

View reviewed changes

Guri999 merged commit 1f48fd7 into main Dec 26, 2024
2 checks passed

Guri999 deleted the feature/explore branch December 26, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/explore #22

Feature/explore #22

Guri999 commented Dec 8, 2024

f-lab-dean left a comment •

edited

Loading

f-lab-dean Dec 8, 2024

Guri999 Dec 24, 2024

f-lab-dean Dec 8, 2024 •

edited

Loading

Guri999 Dec 24, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 8, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 8, 2024

f-lab-dean Dec 9, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 8, 2024

f-lab-dean Dec 9, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 8, 2024

f-lab-dean Dec 9, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 23, 2024

f-lab-dean Dec 8, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 23, 2024

f-lab-dean Dec 8, 2024

Guri999 Dec 24, 2024

f-lab-dean commented Dec 8, 2024

Quality Gate failed

sonarqubecloud bot commented Dec 24, 2024

f-lab-dean left a comment

f-lab-dean Dec 24, 2024

f-lab-dean Dec 24, 2024

f-lab-dean Dec 24, 2024

f-lab-dean Dec 24, 2024


		abstract class PermissionActivity : ComponentActivity() {

		internal fun ComponentActivity.checkStoragePermission(): Boolean =

Feature/explore #22

Feature/explore #22

Conversation

Guri999 commented Dec 8, 2024

f-lab-dean 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

f-lab-dean Dec 8, 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

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

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

f-lab-dean commented Dec 8, 2024

Quality Gate failed

sonarqubecloud bot commented Dec 24, 2024

Quality Gate passed

f-lab-dean left a 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

f-lab-dean left a comment •

edited

Loading

f-lab-dean Dec 8, 2024 •

edited

Loading