Skip to content
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

Upload UI 상태 #147

Merged
merged 16 commits into from
Nov 22, 2023
Merged

Upload UI 상태 #147

merged 16 commits into from
Nov 22, 2023

Conversation

youlalala
Copy link
Member

@youlalala youlalala commented Nov 22, 2023

Issue

Overview

  • 업로드한 이미지, 오디오 상태 관리
    progress bar 는 이 두상태가 하나라도 loading 중이라면 loading 중임을 표시합니다.
  • 기존에 있었던 AutoCompleteTextView에서 position 변경을 감지하는 것이 아니라 text 변경을 감지하도록 바꿨습니다.
  • 파일이름가져오기 공식문서

Screenshot

Screen_recording_20231122_211042.mp4

To Reviewers

이미지랑 오디오는 url 값을 받아와야해서 따로 상태관리를 하도록 uploadedFileState 라는 데이터 클래스를 추가하게 되었습니당!
viewmodel 을 중점적으로 봐주시면 될 것 같네용

Comment on lines -9 to -12
@BindingAdapter("changeSelectedPosition")
fun AutoCompleteTextView.bindPosition(onChange: (Int) -> Unit) {
setOnItemClickListener { _, _, position, _ -> onChange(position) }
}
Copy link
Member Author

Choose a reason for hiding this comment

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

position 이 필요한 것 같지 않아서 선택한 텍스트로 판단하도록 바꼈습니당

@youlalala youlalala changed the title Android/feature/60 Upload UI 상태 정리 Nov 22, 2023
@youlalala youlalala self-assigned this Nov 22, 2023
Copy link

github-actions bot commented Nov 22, 2023

Test Results

4 tests   4 ✔️  1s ⏱️
1 suites  0 💤
1 files    0

Results for commit d577bd8.

♻️ This comment has been updated with latest results.

@youlalala youlalala added ✨ feat 기능 개발 🤖 android android labels Nov 22, 2023
@youlalala youlalala added this to the 🆙 upload milestone Nov 22, 2023
@youlalala youlalala changed the title Upload UI 상태 정리 Upload UI 상태 Nov 22, 2023
Copy link
Collaborator

@2taezeat 2taezeat left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~ 😀😀

Comment on lines +52 to +60
private fun getFileName(uri: Uri): String? {
val contentResolver = requireContext().contentResolver
contentResolver.query(uri, null, null, null, null)?.use { cursor ->
val nameIndex = cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME)
cursor.moveToFirst()
return cursor.getString(nameIndex)
}
return null
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 함수 하는 일이 무엇인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

파일이름을 가져오는 함수입니당 공식문서 참고했어용

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<string name="upload_file">파일 업로드</string>
Copy link
Member

Choose a reason for hiding this comment

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

core ui에서도 strings를 관리하고 있는데, core에서는 재사용될 스트링만 넣을 건지 방식을 통일해야 할 듯

Copy link
Member Author

Choose a reason for hiding this comment

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

core 는 재사용될 string 을 관리하고, 재사용되지 않는 string은 각 피쳐에서 관리하는 게 좋을 것 같아용! 나중에 관리가 힘들어질듯 ㅠㅠ

Copy link
Member

@HamBP HamBP left a comment

Choose a reason for hiding this comment

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

고생했어용~

Comment on lines 75 to 77
}.catch {
// TODO : 에러 처리
_imageState.value = imageState.value.copy(isLoading = false)
Copy link
Member

Choose a reason for hiding this comment

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

onComplete는 성공 실패 여부와 상관 없이 호출 돼서 loading = false 해주는 게 더 적절할 거 같아

Copy link
Member Author

Choose a reason for hiding this comment

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

넹 수정하겠어요

Comment on lines 51 to 70
fun uploadImage(imageFile: File) {
// todo : image 파일을 업로드 한다.
// todo : 반환 값을 uploadedImage에 저장한다.
fun uploadImage(imageUri: Uri) {
imageUri.path?.let { path ->
Copy link
Member

Choose a reason for hiding this comment

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

원래 파일로 변환해서 넘겨줬었는데, uri를 받아서 처리하는 방식으로 바꾼 이유가 있을까?

Copy link
Member Author

Choose a reason for hiding this comment

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

uri 에 관련된 다른 작업을 viewmodel에서 할 수 있지 않을까 해서 넘겨줬는데, 반대로 fragment에서 file 로 변환하는 이유가 있나여?

Copy link
Member

Choose a reason for hiding this comment

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

ViewModel은 주로 데이터를 다루는데 URI를 파일로 변환하는 건 안드로이드 플랫폼에 종속되어 있는 로직이야

import android.net.Uri

또한 LiveData 대신 StateFlow를 사용하는 이유 중 하나가 안드로이드가 아닌 코틀린 종속이기 때문인데, File 변환을 위해 안드로이드 종속을 추가하는 건 어색한 거 같아.

이론적으로 AAC ViewModel이랑 Hilt만 떼어 내면 KMM에서 iOS에서도 ViewModel을 재활용할 수 있어

Copy link
Member

Choose a reason for hiding this comment

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

물론 구조적으로는 그렇지만 나중에 KMM으로 이전할 가능성은...

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 지적입니당 ~ 수정했습니다!👏🏻

Copy link
Member

@HamBP HamBP left a comment

Choose a reason for hiding this comment

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

리뷰 달면 삭제가 안 되네... -> 아니네 아래에서 지울 수 있는 듯

Comment on lines 84 to 87
uploadFileUseCase.getAudioUrl(File(path)).onEach {
_audioState.value = audioState.value.copy(isLoading = false, url = it)
}.onStart {
_audioState.value = audioState.value.copy(isLoading = true)
Copy link
Member

Choose a reason for hiding this comment

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

onStart가 onEach보다 먼저 실행되니까 onStart를 앞에다가 두는 게 더 가독성이 좋을 것 같아!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋아

@youlalala youlalala merged commit bf4b743 into develop Nov 22, 2023
1 check passed
@youlalala youlalala linked an issue Nov 22, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 android android ✨ feat 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

음원 파일 업로드 (서버와 통신)
3 participants