Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions data/diary/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ android {

dependencies {
implementation(projects.data.presigned)
implementation(libs.mlkit.text.recognition)
implementation(libs.kotlinx.coroutines.play.services)
Comment on lines +28 to +29
Copy link
Member

Choose a reason for hiding this comment

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

오호 레포지토리를 새로 만드니까 얘네 의존성도 data > diary로 이동하는군요

Copy link
Member

Choose a reason for hiding this comment

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

음...이 부분도 고민은 한번 해봤으면 해요. data:diary 모듈은 다이어리 데이터의 영속성을 관리하는 역할에 집중해야 한다고 생각해요. 지금은 '이미지에서 텍스트를 추출하는 기능' 이고 과연 Diary라는 도메인의 기능인가? 라는 의문이 생기네요. 장기적으로 보면 core:ml 혹은 data:recognition같은 모듈로 분리하는 과정이 필요할 수도 있을 것 같아요ㅎㅎ 그냥 고민만 해보셔

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package com.hilingual.data.diary.di
import com.hilingual.data.diary.localstorage.DiaryTempRepository
import com.hilingual.data.diary.localstorage.DiaryTempRepositoryImpl
import com.hilingual.data.diary.repository.DiaryRepository
import com.hilingual.data.diary.repository.TextRecognitionRepository
import com.hilingual.data.diary.repositoryimpl.DiaryRepositoryImpl
import com.hilingual.data.diary.repositoryimpl.TextRecognitionRepositoryImpl
import dagger.Binds
import dagger.Module
import dagger.hilt.InstallIn
Expand All @@ -35,4 +37,8 @@ internal abstract class RepositoryModule {
@Binds
@Singleton
abstract fun bindDiaryTempRepository(impl: DiaryTempRepositoryImpl): DiaryTempRepository

@Binds
@Singleton
abstract fun bindTextRecognitionRepository(impl: TextRecognitionRepositoryImpl): TextRecognitionRepository
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

모듈을 따로 분리하지 않고 다이어리 모듈 안에 구현하신 이유가 궁금합니다!
저는 이 부분에 대해서 이미지 -> 텍스트 추출 기능이 data:diary 모듈에 딱 맞는 기능은 아닌 것 같아서 모듈 분리를 해야 하나 싶다가도 별도의 모듈로 분리했을 때 너무 과하게 모듈이 많아지는 것에 대해 우려가 되어 고민이 많이 되더라구요!
최근에 하이링구얼의 모듈 구조에 대해 계속해서 고민을 하던 중이라 나현이의 의견도 궁금하네요ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋은 리뷰 감사합니다 ㅎㅎ 텍스트 인식이 diary 도메인의 핵심 책임은 아니라는 데 동의합니다. 다만 현재는 일기 작성에서만 사용되고 Repository로 이미 추상화되어 있어 실제 재사용 필요성이 생길 때 core:mlkit 같은 모듈로 분리해도
늦지 않을 것 같아서 일단 diary 모듈에 두었습니다. 향후 다른 기능에서도 필요해지면 말씀하신 방향으로 리팩토링 하는 것도 너무 좋을 것 같아요 😃

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.hilingual.data.diary.repository

import android.net.Uri

interface TextRecognitionRepository {
suspend fun extractTextFromImage(uri: Uri): Result<String>
Copy link
Member

Choose a reason for hiding this comment

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

메소드명 굉장히 직관적이고 좋은 것 같습니다👍🏻👍🏻

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.hilingual.data.diary.repositoryimpl

import android.content.Context
import android.net.Uri
import com.google.mlkit.vision.common.InputImage
import com.google.mlkit.vision.text.TextRecognition
import com.google.mlkit.vision.text.latin.TextRecognizerOptions
import com.hilingual.data.diary.repository.TextRecognitionRepository
import dagger.hilt.android.qualifiers.ApplicationContext
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.tasks.await
import kotlinx.coroutines.withContext
import javax.inject.Inject

class TextRecognitionRepositoryImpl @Inject constructor(
@ApplicationContext private val context: Context
) : TextRecognitionRepository {

private val recognizer by lazy {
TextRecognition.getClient(TextRecognizerOptions.DEFAULT_OPTIONS)
}
Copy link
Member

Choose a reason for hiding this comment

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

크게 중요한건 아니지만 만약에 Impl을 단위 테스트 하고자 하면 ML kit 의존성을 모킹하기가 까다로울 수 있어요. Hilt모듈에서 @Provides로 제공하고 생성사 주입으로 변경하면 테스트 용이성이 좀 더 올라갑니다ㅎㅎ
생각해볼만한 주제여서 남겨요, 필수는 아님!👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

그리고 내부 코드를 까보면 TextRecongizerCloseable 인터페이스를 구현하고 있어요. 싱글톤 스코프로 주입받는 레포지토리 내부에서 lazy로 생성되면 앱 프로세스가 종료될 때까지 메모리를 차지할거에요. 일기를 여러번 작성한다면 살려놔도 좋을 것 같지만...여기도 고민되네요. 만약에 사용 빈도가 낮다고 판단되면 close()를 명시적으로 호출해서 자원을 해제하는 방법도 있습니다. 한번 고려해보셔요


override suspend fun extractTextFromImage(uri: Uri): Result<String> =
withContext(Dispatchers.IO) {
runCatching {
val image = InputImage.fromFilePath(context, uri)
recognizer.process(image).await().text
}
}
}
4 changes: 0 additions & 4 deletions presentation/diarywrite/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,5 @@ dependencies {
implementation(projects.data.calendar)
implementation(projects.data.diary)

// ML Kit
implementation(libs.mlkit.text.recognition)
implementation(libs.kotlinx.coroutines.play.services)

implementation(libs.lottie)
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,20 @@
*/
package com.hilingual.presentation.diarywrite

import android.content.Context
import android.net.Uri
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.navigation.toRoute
import com.google.mlkit.vision.common.InputImage
import com.google.mlkit.vision.text.TextRecognition
import com.google.mlkit.vision.text.latin.TextRecognizerOptions
import com.hilingual.core.common.extension.onLogFailure
import com.hilingual.core.common.util.UiState
import com.hilingual.core.navigation.DiaryWriteMode
import com.hilingual.data.calendar.repository.CalendarRepository
import com.hilingual.data.diary.localstorage.DiaryTempRepository
import com.hilingual.data.diary.repository.DiaryRepository
import com.hilingual.data.diary.repository.TextRecognitionRepository
import com.hilingual.presentation.diarywrite.navigation.DiaryWrite
import dagger.hilt.android.lifecycle.HiltViewModel
import dagger.hilt.android.qualifiers.ApplicationContext
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
Expand All @@ -42,7 +38,6 @@ import kotlinx.coroutines.flow.asSharedFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
import kotlinx.coroutines.tasks.await
import kotlinx.coroutines.withContext
import timber.log.Timber
import java.io.File
Expand All @@ -52,10 +47,10 @@ import javax.inject.Inject
@HiltViewModel
internal class DiaryWriteViewModel @Inject constructor(
savedStateHandle: SavedStateHandle,
@ApplicationContext private val context: Context,
private val calendarRepository: CalendarRepository,
private val diaryRepository: DiaryRepository,
private val diaryTempRepository: DiaryTempRepository
private val diaryTempRepository: DiaryTempRepository,
private val textRecognitionRepository: TextRecognitionRepository
) : ViewModel() {
private val route: DiaryWrite = savedStateHandle.toRoute<DiaryWrite>()

Expand Down Expand Up @@ -186,22 +181,17 @@ internal class DiaryWriteViewModel @Inject constructor(
fun extractTextFromImage(uri: Uri, tempFileToDelete: File? = null) {
viewModelScope.launch {
try {
runCatching {
withContext(Dispatchers.IO) {
val image = InputImage.fromFilePath(context, uri)
val recognizer =
TextRecognition.getClient(TextRecognizerOptions.DEFAULT_OPTIONS)
recognizer.process(image).await().text
textRecognitionRepository.extractTextFromImage(uri)
.onSuccess { extractedText ->
_uiState.update {
it.copy(
diaryText = extractedText.take(MAX_DIARY_TEXT_LENGTH)
)
}
}
}.onSuccess { extractedText ->
_uiState.update {
it.copy(
diaryText = extractedText.take(MAX_DIARY_TEXT_LENGTH)
)
.onFailure { throwable ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 저희 텍스트 추출에 실패했을 때 처리가 따로 안 되어 있었네요!? 이것도 다같이 고민해보면 좋을 것 같아요

Timber.tag("DiaryWriteViewModel").e(throwable, "Text recognition failed")
Copy link
Member

Choose a reason for hiding this comment

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

혹시 태그가 꼭 필요하지 않다면 onLogFailure 사용해주세요~

}
}.onFailure { throwable ->
Timber.tag("DiaryWriteViewModel").e(throwable, "Text recognition failed")
}
} finally {
withContext(Dispatchers.IO) {
if (tempFileToDelete?.exists() == true) {
Expand Down
Loading