PDF 화면 구성 및 PDF 렌더링 #27

Guri999 · 2024-12-31T02:39:18Z

#️⃣연관된 이슈

#26 #11 #20

📝작업 내용

기능추가

PDF 파일을 렌더링 합니다
PDF 파일의 페이지가 많은 경우가 있습니다. 이때 렌더링 시간이 오래걸리기 때문에 LazyColumn의 아이템이 생성되는 순간 렌더링을 하게 하여 렌더링시간을 단축했습니다.
그리고 확장 줌이 가능하기 때문에 해상도가 깨지지 않게 고해상도 이미지로 렌더링하였습니다

화면 터치시 페이지 이동이 가능한 TopBar 가 출력됩니다
화면을 만지면 화면 상단에 페이지 이동이 가능한 Topbar가 보이게 됩니다. 이때 상태바는 투명색에서 Topbar와 같은 색을 가지게 됩니다.

리팩토링

Domain 모듈을 제거했습니다
기존 CleanArchitecture기반의 구성은 프로젝트 규모에 비해 오버엔지니어링이라는 생각이 들어 관리하기 용이하게 RepositoryPattern으로 변경 하였습니다.

파일 탐색 백그라운드 쓰레드로 변경
기존에 main스레드에서 수행되던 파일 탐색 작업을 백그라운드 스레드로 변경하였습니다.

f-lab-dean

현재는 UI에서 데이터 소스에 접근하고 모든 상태를 관리하는 구조로 보입니다.
앞으로 아키텍처가 MVVM 등으로 변경되는지, 아니면 이러한 구조를 선택한 이유가 있는지 궁금합니다.

하나의 PR에는 하나의 작업만이 포함되는 것이 좋습니다.
현재의 PR에서 기능추가와 리팩토링은 연관이 없어 보이므로 PR을 분리하는 것이 좋고,
연관이 있다 하더라도 변화량을 최대한 줄여 리뷰가 원활히 진행되도록 분리하는 것이 좋습니다.

f-lab-dean · 2024-12-31T08:27:40Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

-    PdfScreen(
-        popBackStack = popBackStack
-    )
+    val uri = Uri.fromFile(File(path))


리컴포지션시에 Uri를 읽어오는 작업이 반복 될 것 같습니다.
uri를 remember와 같은 것으로 처리하지 않아도 무방한가요?

remember로 처리하는게 맞는것 같습니다

f-lab-dean · 2024-12-31T08:30:52Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

    }
 }

 @Preview
 @Composable
 private fun Preview() {
    SeeDocsTheme {
-        PdfScreen()
+//        PdfScreen()


프리뷰에 내용이 없다면 삭제해도 될 것 같습니다.

삭제하겠습니다

f-lab-dean · 2024-12-31T08:36:46Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

@@ -73,22 +84,46 @@ private fun PdfImage(
    var bitmap by remember {
        mutableStateOf<Bitmap?>(null)
    }
+    var isLoading by remember { mutableStateOf(true) }


의견)
PdfRenderer를 내부적으로 사용하는 Pdf to Bitmap 클래스를 만들어서 로직을 캡슐화하고, 렌더링 상태(ex. 로딩, 렌더링 완료)를 StateFlow로 구독할 수 있게 하는 방법은 어떠신가요?

f-lab-dean · 2024-12-31T08:37:45Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

@@ -89,8 +89,8 @@ private fun PdfImage(
    LaunchedEffect(Unit) {
        isLoading = true
        val page = renderer.openPage(pageIndex)
-        Bitmap.createBitmap(page.width, page.height, Bitmap.Config.ARGB_8888).also {
-            page.render(it, null, null, PdfRenderer.Page.RENDER_MODE_FOR_PRINT)
+        Bitmap.createBitmap(page.width * 3, page.height * 3, Bitmap.Config.ARGB_8888).also {


기기의 메모리 상태 등에 의해 문제가 발생할 위험은 없나요?

디바이스 하나에 2~3페이지 정도만 랜더링이 되어서 괜찮을 것 같다 판단했습니다
크기를 1배로 랜더링 하면 확대할때 이미지 화질이 많이 낮아서 기존 이미지보다 높은 화질로 랜더링 하긴 해야됐는데, 이때 처음부터 3배로 할지 아니면 확대할때 새로 랜더링을 할지 고민을 했습니다
그런데 어차피 읽을때 대부분 확대를 해야될테니 랜더링하는 과정이 많아지면 오히려 성능적으로 좋지 않을것 같다고 생각들었습니다

확대할때 새로 렌더링을 하는 것은 성능적인 부분에서 리스크가 있겠지만,
메모리 상황을 고려하지 않고 3배로 확대하는 것 또한 리스크가 있는 부분일 것 같습니다.

수정하실 필요는 없지만, 이런 부분을 개선할 수 있는 방법을 고민해봐도 좋을 것 같아요.
3배로 늘린 크기의 최대치를 지정해놓는다던가 하는 방법이 있을 듯 합니다.

f-lab-dean · 2024-12-31T08:38:04Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

@@ -89,8 +89,8 @@ private fun PdfImage(
    LaunchedEffect(Unit) {
        isLoading = true
        val page = renderer.openPage(pageIndex)
-        Bitmap.createBitmap(page.width, page.height, Bitmap.Config.ARGB_8888).also {
-            page.render(it, null, null, PdfRenderer.Page.RENDER_MODE_FOR_PRINT)
+        Bitmap.createBitmap(page.width * 3, page.height * 3, Bitmap.Config.ARGB_8888).also {


매직넘버는 상수로 만들어 사용하는 것이 좋을 것 같습니다.

수정하겠습니다

f-lab-dean · 2024-12-31T08:46:31Z

core/model/src/main/java/kr/co/model/FileInfo.kt

+                when(extension) {
+                    "pdf" -> PDF
+                    "jpg", "jpeg", "png", "gif" -> IMAGE
+                    else -> IMAGE


이외의 확장자를 IMAGE로 했을 때 문제가 되는 부분은 없을까요?

f-lab-dean · 2024-12-31T08:49:27Z

.idea/deploymentTargetSelector.xml

필요하지 않은 파일 확인해주세요.

f-lab-dean · 2024-12-31T09:24:34Z

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

+        keyboardOptions = keyboardOptions,
+    ) {
+        Box(
+            modifier = Modifier


OutlinedTextField를 사용하지 않은 이유는 contentPadding 때문일까요?

f-lab-dean · 2024-12-31T09:26:21Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

+
+    var isTopBarVisible by remember { mutableStateOf(false) }
+
+    var tobBarJob by remember { mutableStateOf<Job?>(null) }


또는 탑바의 Visibility를 관리하는 rememberTopBarState 같은 상태나 혹은 타이머 등의 구현도 가능할 듯 한데,
현재의 방식을 선택하신 이유가 궁금합니다.

화면을 터치했을때 보여주는 작업인데, 누르고 3초뒤엔 안보이지만 사용자가 계속해서 화면을 터치하고 있다면 작업을 취소하고 계속해서 보여주기 위해 이렇게 구현했습니다

어떤 기능인지는 이해를 했구요.
기능의 동작이나 구현 자체도 문제가 아니라 더 좋은 구조는 없을까에 대한 의논을 펼치고자 하는것이 의도입니다.

onTopBarVisibleChange 는 onPdfBodyPressed 와 같은 내용이 더 맞지 않을까 합니다. PdfScreen 입장에서는 LazyColumn을 tap한 것이 isTopBarVisible를 변경하게 하는 것인지 모르기 때문입니다. tap했을 때 isTopBarVisible를 변경하는 것은 상위에서 결정하는 동작으로 보입니다.

만약 PdfScreen이 isTopBarVisible의 구체적인 동작을 알아야 한다면, isTopBarVisible의 상태 관리를 상위에서 할 필요가 없어보입니다.

TopBar의 상태 처리를 하나로 캡슐화하는 것을 고려해보면 좋겠습니다. TopBar라는 하나의 View에 대한 상태, 처리로직이 산발적으로 위치한다면, 협업의 관점에서 의도치 않은 결과가 생길 수 있습니다. 예를들어 다른 코루틴 잡을 tobBarJob에 할당한다던지, 다른 콜백, 로직에서 isTopBarVisible를 변경할 때 tobBarJob의 동작을 cancel하지 않는다던가 등의 상황이 있을 수 있습니다. 코드가 복잡해질수록 구체적인 사항을 놓칠 수 있기 때문입니다.

f-lab-dean · 2024-12-31T09:29:32Z

core/database/src/main/java/kr/co/database/SeeDocsDatabase.kt

+
+abstract class SeeDocsDatabase : RoomDatabase() {
+
+    abstract fun recentFileDao(): RecentFileDao


현재의 PR에서 SeeDocsDatabase가 사용되는 곳은 없는데, 이 PR에 포함된 이유가 있을까요?

잘못 포함된 커밋입니다,, revert해서 도려낼까요?

가급적 PR에 포함되지 않아야할 내용은 포함되지 않도록 하는것이 좋습니다.
다음 PR부터 잘 챙겨주세요.
개발 계획을 좀 더 명확히 하고 진행하면 이런일이 줄어들 것 같습니다.

f-lab-dean · 2025-01-07T02:59:26Z

feature/pdf/src/main/java/kr/co/pdf/PdfScreen.kt

-    var isTopBarVisible by remember { mutableStateOf(false) }
-
-    var tobBarJob by remember { mutableStateOf<Job?>(null) }
+    val tabBarState = rememberTopBarState()


tabBarState -> topBarState로 변경되어야 할 것 같습니다.

f-lab-dean · 2025-01-07T03:04:23Z

core/ui/src/main/java/kr/co/ui/util/TopBarState.kt

+
+    private var currentScope: Job? = null
+
+    fun onBodyPress() {


TopBarState에 onBodyPress는 조금 어색한 것 같습니다.
PdfScreen의 Body 부분이 클릭되었을 때 TopBarState의 상태를 변경해야 하므로, TopBarState의 입장에서의 함수명은 show 와 같은 느낌의 방향은 어떠신가요?

예를 들어 저라면 아래와 같은 형태로 작성할 것 같습니다.

fun show(hideInterval: Long = DEFAULT_HIDE_INTERVAL) { ... }

f-lab-dean · 2025-01-07T03:04:31Z

core/ui/src/main/java/kr/co/ui/util/TopBarState.kt

+    var topBarVisible by mutableStateOf(false)
+        private set
+
+    private var currentScope: Job? = null


currentJob으로 변경해야 할 것 같습니다.

f-lab-dean · 2025-01-07T03:06:22Z

core/ui/src/main/java/kr/co/ui/util/TopBarState.kt

+    fun onBodyPress() {
+        currentScope?.cancel()
+
+        currentScope = scope.launch {


CoroutineScope(Dispatchers.IO) 와 같은 잘못된 Scope가 전달되었을 때 문제는 없을까요?

f-lab-dean · 2025-01-07T03:10:57Z

feature/pdf/src/main/java/kr/co/util/PdfToBitmap.kt

+        val page = renderer.openPage(pageIndex)
+
+        val bitmap =
+            Bitmap.createBitmap(


의도된 줄내림 일까요?

f-lab-dean · 2025-01-07T03:11:24Z

feature/pdf/src/main/java/kr/co/util/PdfToBitmap.kt

+
+    suspend fun renderPage(pageIndex: Int) {
+        mutex.withLock {
+            if (renderingPages.contains(pageIndex)) return


if문 다음에 이어지는 줄이 있다면 {}를 생략하지 않는것이 가독성에 도움이 됩니다.
return문이 있기 때문에 이 상황에서는 좀 더 낫지만, 일관적인 컨벤션을 위해 괄호를 사용하는 것을 추천드립니다.

f-lab-dean · 2025-01-07T03:12:19Z

feature/pdf/src/main/java/kr/co/util/PdfToBitmap.kt

+                page.close()
+            }
+
+


new line이 두 개인데, 의도된 것일까요?

f-lab-dean · 2025-01-07T03:14:54Z

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

@@ -11,7 +11,7 @@ import androidx.annotation.RequiresApi
 import androidx.core.app.ActivityCompat
 import androidx.core.content.ContextCompat

-abstract class PermissionActivity : ComponentActivity() {
+internal class PermissionManager {


이 리팩토링은 이 브랜치에 어울리지 않는 커밋인 것 같습니다.

f-lab-dean

프로젝트 진행을 위해 머지 승인을 드리겠습니다.
다음을 확인해주세요.

모든 코멘트에 답변을 달아 의견을 말씀해주세요.
코멘트 내용을 코드에 반영했다면 반영한 커밋을 남겨주세요.
포함될 필요가 없는 파일이 있는지 확인해주세요.

f-lab-dean · 2025-01-07T03:17:45Z

.idea/deploymentTargetSelector.xml

.idea 폴더의 파일이 포함되지 않도록 gitignore를 수정해주세요.

This reverts commit 003c38d.

This reverts commit 91e5fa5.

sonarqubecloud · 2025-01-07T11:38:18Z

Quality Gate passed

Issues
3 New issues
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

Guri999 added 10 commits December 31, 2024 00:13

Feat: PDF 렌더링

bdff90f

Feat: PDF Zoom & drag

e95c491

Feat: PDF 해상도 증가

cf10836

Feat: PDF Topbar

0785726

Feat: PDF TopBar Page calculation

2d70b89

Feat: Domain 모듈 제거

4b66946

Feat: Domain 모듈 제거

58596a1

Feat: File Model 생성, 파일 찾기 로직 분리

c4aa77c

Feat: Room 생성

e65341e

Refactor: 파일 탐색 백그라운드 스레드로 변경

809743a

Guri999 self-assigned this Dec 31, 2024

f-lab-dean reviewed Dec 31, 2024

View reviewed changes

Guri999 added 9 commits January 3, 2025 14:51

Refactor: Uri remember로 처리

0de159f

Refactor: 매직넘버 상수로 관리

21a6a33

Refactor: page,onPageChange -> page, onPageChange

308e067

Refactor: 빈 프리뷰 제거

830538f

Refactor: TopBar 관리 캡슐화

b7e491a

Refactor: Permission 관리 변경

1a1ed20

Chore: onTopBarVisibleChange -> onPdfBodyPressed 이름 변경

641f6de

Refactor: PdfToBitmap class 생성하여 pdf 렌더링 관리

6020dd8

Chore: if문 괄호

3bd4b0d

f-lab-dean reviewed Jan 7, 2025

View reviewed changes

f-lab-dean approved these changes Jan 7, 2025

View reviewed changes

f-lab-dean reviewed Jan 7, 2025

View reviewed changes

.idea/deploymentTargetSelector.xml Outdated

Copy link

f-lab-dean Jan 7, 2025

Choose a reason for hiding this comment

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

.idea 폴더의 파일이 포함되지 않도록 gitignore를 수정해주세요.

Guri999 added 5 commits January 7, 2025 20:10

Feat: Koin 설정

91e5fa5

Refactor: PDF화면 MVI refactor

003c38d

Revert "Refactor: PDF화면 MVI refactor"

a7f951d

This reverts commit 003c38d.

Revert "Feat: Koin 설정"

39ab182

This reverts commit 91e5fa5.

Refactor: TopBarState currentScope -> currentJob

ea25198

Guri999 added 6 commits January 7, 2025 20:30

Refactor: TopBarState onBodyPress -> show

514b612

Refactor: TopBarState 생성자 제한

1b8822a

Chore: bitmap 줄내림 수정

04c1ab7

Chore: if문 중괄호 표시

6345800

Chore: line 제거

9a39999

Chore: ignore .idea 추가

7afa071

Guri999 merged commit 757e8dd into main Jan 7, 2025
2 checks passed

Guri999 deleted the feature/pdf branch January 7, 2025 11:38

This was referenced Jan 7, 2025

Feature/pdf mvi #28

Closed

PDF MVI 리팩토링 #29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDF 화면 구성 및 PDF 렌더링 #27

PDF 화면 구성 및 PDF 렌더링 #27

Guri999 commented Dec 31, 2024 •

edited

Loading

f-lab-dean left a comment •

edited

Loading

f-lab-dean Dec 31, 2024

Guri999 Jan 3, 2025

f-lab-dean Dec 31, 2024

Guri999 Jan 3, 2025

f-lab-dean Dec 31, 2024

f-lab-dean Dec 31, 2024

Guri999 Jan 3, 2025

f-lab-dean Jan 3, 2025 •

edited

Loading

f-lab-dean Dec 31, 2024

Guri999 Jan 3, 2025

f-lab-dean Dec 31, 2024

f-lab-dean Dec 31, 2024

f-lab-dean Dec 31, 2024

f-lab-dean Dec 31, 2024 •

edited

Loading

Guri999 Jan 3, 2025

f-lab-dean Jan 3, 2025

f-lab-dean Dec 31, 2024

Guri999 Jan 3, 2025

f-lab-dean Jan 3, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025

f-lab-dean Jan 7, 2025

f-lab-dean left a comment

f-lab-dean Jan 7, 2025

sonarqubecloud bot commented Jan 7, 2025


		var isTopBarVisible by remember { mutableStateOf(false) }

		var tobBarJob by remember { mutableStateOf<Job?>(null) }


		abstract class SeeDocsDatabase : RoomDatabase() {

		abstract fun recentFileDao(): RecentFileDao

PDF 화면 구성 및 PDF 렌더링 #27

PDF 화면 구성 및 PDF 렌더링 #27

Conversation

Guri999 commented Dec 31, 2024 • edited Loading

#️⃣연관된 이슈

📝작업 내용

기능추가

리팩토링

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

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 Jan 3, 2025 • 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

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

f-lab-dean Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-dean Jan 7, 2025 • 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

f-lab-dean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 7, 2025

Quality Gate passed

Guri999 commented Dec 31, 2024 •

edited

Loading

f-lab-dean left a comment •

edited

Loading

f-lab-dean Jan 3, 2025 •

edited

Loading

f-lab-dean Dec 31, 2024 •

edited

Loading

f-lab-dean Jan 3, 2025 •

edited

Loading

f-lab-dean Jan 7, 2025 •

edited

Loading