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.

Sign up for GitHub

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

Jump to bottom

#257 [feat] 성향결과 이미지 다운로드 구현 #314

Merged
merged 25 commits into from
Sep 11, 2023
Merged

#257 [feat] 성향결과 이미지 다운로드 구현 #314

merged 25 commits into from
Sep 11, 2023

Conversation

2zerozu
Copy link
Contributor

@2zerozu 2zerozu commented Sep 10, 2023 •
edited
Loading

관련 이슈

2023-09-10.8.19.37.mov

작업한 내용

  • 이미지 다운로드 로직 구현

PR 포인트

@2zerozu 2zerozu changed the title Feature/#257 feat result image download #257 [feat] 성향결과 이미지 다운로드 구현 Sep 10, 2023
@2zerozu 2zerozu requested review from KWY0218 and murjune and removed request for KWY0218 September 10, 2023 11:22
@2zerozu 2zerozu self-assigned this Sep 10, 2023
@2zerozu 2zerozu added 영주🐼 영주가 작업함! feat 새로운 기능 추가 Pull Request🔥 풀리퀘 날림! labels Sep 10, 2023
@2zerozu 2zerozu added this to the Hous 3차 릴리즈 milestone Sep 10, 2023
KWY0218
KWY0218 reviewed Sep 10, 2023
View reviewed changes
Copy link
Member

@KWY0218 KWY0218 left a comment

Choose a reason for hiding this comment

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

작업하시느라 수고하셨습니다 ~ !
코맨트 확인해주세요!

.../main/java/hous/release/android/presentation/personality/result/PersonalityResultActivity.kt Outdated
Comment on lines 96 to 98
} else {
showToast(this, "권한을 설정해주세요.")
}
Copy link
Member

Choose a reason for hiding this comment

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

string resource

...main/java/hous/release/android/presentation/personality/result/PersonalityResultViewModel.kt Outdated
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.launch
import timber.log.Timber
import javax.inject.Inject

@HiltViewModel
class PersonalityResultViewModel @Inject constructor(
private val application: Application,
Copy link
Member

Choose a reason for hiding this comment

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

viewmodel 내부엔 context를 지양하고 있습니다.
application 또한 context 를 상속 받고 있기 때문에
viewmodel 과 application 을 분리해야 된다고 생각합니다..!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

viewmodel 내부엔 context를 지양하고 있습니다. application 또한 context 를 상속 받고 있기 때문에 viewmodel 과 application 을 분리해야 된다고 생각합니다..!

context 안받으려고 application 받았는데..
액티비티에서 코루틴을 사용하지 않으려고 application 받아서 구현했거든요
좀 더 고민해보겠습니다

Copy link
Member

Choose a reason for hiding this comment

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

viewmodel 내부엔 context를 지양하고 있습니다.
application 또한 context 를 상속 받고 있기 때문에
viewmodel 과 application 을 분리해야 된다고 생각합니다..!

좀 더 추가 설명을 달자면 context와 viewModel의 생명주기는 다르다는 점 때문입니다. 따라서, context를 잘 못 사용하게 된다면 메모리 누수가 발생할 수 있습니다.
예시로, configure Change가 발생한다면 가정해볼게요.
그렇다면, context가 파괴되고, 재 생성될 겁니다. 그런데 viewModel은 configure Change에도 살아남죠.
만약 viewmodel이 configure Change 이전에 넘겨준 context일 경우 메모리 누수와 더불어 Error가 발생할 수 있습니다

murjune
murjune reviewed Sep 10, 2023
View reviewed changes
Copy link
Member

@murjune murjune left a comment •
edited
Loading

Choose a reason for hiding this comment

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

고생하셨습니다!! 코멘트 확인해주세용

@2zerozu
추가적으로 이미지를 저장하는 로직은 data layer 에서 처리해야한다고 생각이 드네요.
레포지토리를 하나 파서 처리해도 괜찮을 거 같습니다!

...main/java/hous/release/android/presentation/personality/result/PersonalityResultViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/hous/release/android/util/ImageDownloader.kt Outdated Show resolved Hide resolved
...main/java/hous/release/android/presentation/personality/result/PersonalityResultViewModel.kt Outdated Show resolved Hide resolved
...main/java/hous/release/android/presentation/personality/result/PersonalityResultViewModel.kt Outdated Show resolved Hide resolved
...main/java/hous/release/android/presentation/personality/result/PersonalityResultViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/hous/release/android/util/ImageDownloader.kt Outdated Show resolved Hide resolved
app/src/main/java/hous/release/android/util/ImageDownloader.kt Outdated Show resolved Hide resolved
murjune
murjune approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@murjune murjune left a comment

Choose a reason for hiding this comment

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

아 어푸로브를 안줬네 ^____^

KWY0218
KWY0218 approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@KWY0218 KWY0218 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 ~ ! 👍

domain/src/main/java/hous/release/domain/entity/HomyType.kt Outdated
@@ -1,5 +1,5 @@
package hous.release.domain.entity

enum class HomyType {
YELLOW, RED, BLUE, PURPLE, GREEN, GRAY
YELLOW, RED, BLUE, PURPLE, GREEN, GRAY;
Copy link
Member

Choose a reason for hiding this comment

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

; ㅋㅋㅋㅋㅋ 자바 세계관 충돌..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 젠장 ㅋㅋㅋ 수정하고 머지할게

@2zerozu 2zerozu merged commit a40c45c into develop Sep 11, 2023
1 check passed
@2zerozu 2zerozu deleted the feature/#257-feat-result-image-download branch September 11, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murjune murjune murjune approved these changes

@KWY0218 KWY0218 KWY0218 approved these changes

Assignees

@2zerozu 2zerozu

Labels
feat 새로운 기능 추가 Pull Request🔥 풀리퀘 날림! 영주🐼 영주가 작업함!
Projects
None yet
Milestone
Hous 3차 릴리즈
Development

Successfully merging this pull request may close these issues.

[feat] 성향결과 이미지 다운로드 구현
3 participants
@2zerozu @KWY0218 @murjune