-
Notifications
You must be signed in to change notification settings - Fork 77
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
Move Toast invocations out of Camera/VideoEdit ViewModels. #39
base: main
Are you sure you want to change the base?
Conversation
651ca00
to
6cc3d37
Compare
6cc3d37
to
e124a3a
Compare
app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreenViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreenViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/camera/Camera.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreenViewModel.kt
Outdated
Show resolved
Hide resolved
One more thing, the PR comment could be "Closes #19", this will close that issue automatically after this PR gets merged. |
app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreenViewModel.kt
Outdated
Show resolved
Hide resolved
LGTM except what @Goooler pointed out. |
LaunchedEffect(lifecycleOwner, context) { | ||
viewModel.videoSaveState.collect { state -> | ||
when (state) { | ||
VideoSaveState.VIDEO_SAVE_SUCCESS -> | ||
Toast.makeText(context, "Edited video saved", Toast.LENGTH_LONG).show() | ||
VideoSaveState.VIDEO_SAVE_FAIL -> | ||
Toast.makeText(context, "Error applying edits on video", Toast.LENGTH_LONG) | ||
.show() | ||
VideoSaveState.PENDING -> Unit | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add viewModel.markMessageShown()
to avoid showing toast twice, for example: onStop and onStart again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoc081098
I think it's better to change to the existing collectAsStateWithLifecycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yongsuk44 nice, but LifecycleResumeEffect
is more suitable for flow collection 🙏
val scope = rememberCoroutineScope()
LifecycleResumeEffect(scope, viewModel) {
// ON_RESUME code is executed here
val job = viewModel.videoSaveState
.onEach{ /*handler*/}
.launchIn(scope)
onPauseOrDispose {
// do any needed clean up here
job.cancel()
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoc081098
Thanks for the feedback, I have question.
Considering the link I provided, collectAsStateWithLifecycle is designed to simplify the task of activating Flow collection when the app is in the foreground, making it easier for developers to manage state. So, why use LifecycleResumeEffect?
Why are you hoisting the state in the ViewModel and then bringing state change events back to the Composable? It seems to go against the Unidirectional Data Flow (UDF) design pattern.
I thought it was fetching state change events, I must have seen it wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yongsuk44
collectAsStateWithLifecycle()
= produceState() + repeatOnLifecycle() + flow.collect{}
.
👉 we only collect the flow 👍, we don't need a returned Compose State<T>
, don't need the recomposition of the @Composable
.
✅ LifecycleResumeEffect
is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoc081098 😀 I understand, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to rebase this branch and fix the style conflicts.
Closes #19