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

Move Toast invocations out of Camera/VideoEdit ViewModels. #39

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

donovanfm
Copy link
Contributor

@donovanfm donovanfm commented Mar 20, 2024

Closes #19

@Goooler
Copy link
Contributor

Goooler commented Mar 22, 2024

One more thing, the PR comment could be "Closes #19", this will close that issue automatically after this PR gets merged.

@yaraki
Copy link
Member

yaraki commented Mar 22, 2024

LGTM except what @Goooler pointed out.

Comment on lines +111 to +122
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
}
}
}

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

Choose a reason for hiding this comment

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

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

Copy link

@hoc081098 hoc081098 Mar 22, 2024

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()
  }
}

Copy link

@yongsuk44 yongsuk44 Mar 22, 2024

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.

Copy link

@hoc081098 hoc081098 Mar 23, 2024

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

Choose a reason for hiding this comment

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

@hoc081098 😀 I understand, thanks

Copy link
Contributor

@Goooler Goooler left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can you tell me why Application Context is used in ViewModel?
5 participants