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

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

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e124a3a
Move Toast invocations out of Camera/VideoEdit ViewModels.
donovanfm Mar 20, 2024
445550b
Run spotlessApply
donovanfm Mar 20, 2024
2bc9213
Clean up video/photo save state logic.
donovanfm Mar 21, 2024
ed065cc
Changing some vars to vals.
donovanfm Mar 26, 2024
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
13 changes: 13 additions & 0 deletions app/src/main/java/com/google/android/samples/socialite/ui/camera/Camera.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.google.android.samples.socialite.ui.camera
import android.Manifest
import android.annotation.SuppressLint
import android.view.Surface
import android.widget.Toast
import androidx.camera.core.CameraSelector
import androidx.camera.core.Preview
import androidx.camera.view.RotationProvider
Expand Down Expand Up @@ -91,6 +92,18 @@ fun Camera(
val lifecycleOwner = LocalLifecycleOwner.current
val context = LocalContext.current

LaunchedEffect(lifecycleOwner, context) {
viewModel.imageCaptureState.collect { state ->
when (state) {
ImageCaptureState.IMAGE_CAPTURE_SUCCESS ->
Toast.makeText(context, "Photo saved.", Toast.LENGTH_SHORT).show()
ImageCaptureState.IMAGE_CAPTURE_FAIL ->
Toast.makeText(context, "Photo capture failed.", Toast.LENGTH_SHORT).show()
ImageCaptureState.PENDING -> Unit
}
}
}

var isLayoutUnfolded by remember { mutableStateOf<Boolean?>(null) }

LaunchedEffect(lifecycleOwner, context) {
Expand Down
19 changes: 14 additions & 5 deletions app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import android.content.Context
import android.os.Build
import android.provider.MediaStore
import android.view.Display
import android.widget.Toast
import androidx.annotation.RequiresPermission
import androidx.camera.core.AspectRatio
import androidx.camera.core.Camera
Expand Down Expand Up @@ -58,6 +57,7 @@ import java.text.SimpleDateFormat
import java.util.Locale
import javax.inject.Inject
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.launch

@HiltViewModel
Expand All @@ -72,6 +72,8 @@ class CameraViewModel @Inject constructor(

val chatId: Long? = savedStateHandle.get("chatId")
var viewFinderState = MutableStateFlow(ViewFinderState())
private var _imageCaptureState = MutableStateFlow(ImageCaptureState.PENDING)
donovanfm marked this conversation as resolved.
Show resolved Hide resolved
val imageCaptureState: StateFlow<ImageCaptureState> = _imageCaptureState

val aspectRatioStrategy =
AspectRatioStrategy(AspectRatio.RATIO_16_9, AspectRatioStrategy.FALLBACK_RULE_NONE)
Expand Down Expand Up @@ -189,19 +191,20 @@ class CameraViewModel @Inject constructor(
ContextCompat.getMainExecutor(context),
object : ImageCapture.OnImageSavedCallback {
override fun onError(exc: ImageCaptureException) {
val msg = "Photo capture failed."
Toast.makeText(context, msg, Toast.LENGTH_SHORT).show()
_imageCaptureState.value = ImageCaptureState.IMAGE_CAPTURE_FAIL
}

override fun onImageSaved(output: ImageCapture.OutputFileResults) {
var state: ImageCaptureState
donovanfm marked this conversation as resolved.
Show resolved Hide resolved
val savedUri = output.savedUri
if (savedUri != null) {
state = ImageCaptureState.IMAGE_CAPTURE_SUCCESS
sendPhotoMessage(savedUri.toString())
onMediaCaptured(Media(savedUri, MediaType.PHOTO))
} else {
val msg = "Photo capture failed."
Toast.makeText(context, msg, Toast.LENGTH_SHORT).show()
state = ImageCaptureState.IMAGE_CAPTURE_FAIL
}
_imageCaptureState.value = state
}
},
)
Expand Down Expand Up @@ -310,6 +313,12 @@ data class ViewFinderState(
val lensFacing: Int = CameraSelector.LENS_FACING_BACK,
)

enum class ImageCaptureState {
PENDING,
IMAGE_CAPTURE_SUCCESS,
IMAGE_CAPTURE_FAIL,
}

donovanfm marked this conversation as resolved.
Show resolved Hide resolved
/**
* Defines the current state of the camera.
*/
Expand Down
17 changes: 17 additions & 0 deletions app/src/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.google.android.samples.socialite.ui.videoedit
import android.media.MediaMetadataRetriever
import android.net.Uri
import android.util.Log
import android.widget.Toast
import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
Expand Down Expand Up @@ -59,6 +60,7 @@ import androidx.compose.material3.TextFieldDefaults
import androidx.compose.material3.TopAppBar
import androidx.compose.material3.TopAppBarDefaults
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
Expand All @@ -71,6 +73,7 @@ import androidx.compose.ui.graphics.asImageBitmap
import androidx.compose.ui.graphics.vector.ImageVector
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.platform.LocalInspectionMode
import androidx.compose.ui.platform.LocalLifecycleOwner
import androidx.compose.ui.res.colorResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.tooling.preview.Preview
Expand All @@ -92,6 +95,7 @@ fun VideoEditScreen(
onCloseButtonClicked: () -> Unit,
navController: NavController,
) {
val lifecycleOwner = LocalLifecycleOwner.current
val context = LocalContext.current

val viewModel: VideoEditScreenViewModel = hiltViewModel()
Expand All @@ -104,6 +108,19 @@ fun VideoEditScreen(

val isProcessing = viewModel.isProcessing.collectAsState()

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

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

Copy link

Choose a reason for hiding this comment

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

https://developer.android.com/topic/architecture/ui-layer/events#consuming-trigger-updates

Copy link

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

Copy link

Choose a reason for hiding this comment

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

@hoc081098 😀 I understand, thanks


var removeAudioEnabled by rememberSaveable { mutableStateOf(false) }
var overlayText by rememberSaveable { mutableStateOf("") }
var redOverlayTextEnabled by rememberSaveable { mutableStateOf(false) }
Expand Down
17 changes: 11 additions & 6 deletions ...c/main/java/com/google/android/samples/socialite/ui/videoedit/VideoEditScreenViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import android.text.SpannableString
import android.text.SpannableStringBuilder
import android.text.style.ForegroundColorSpan
import android.text.style.RelativeSizeSpan
import android.widget.Toast
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.media3.common.MediaItem
Expand All @@ -42,7 +41,6 @@ import com.google.android.samples.socialite.repository.ChatRepository
import com.google.android.samples.socialite.ui.camera.CameraViewModel
import com.google.common.collect.ImmutableList
import dagger.hilt.android.lifecycle.HiltViewModel
import dagger.hilt.android.qualifiers.ApplicationContext
import java.io.File
import java.io.IOException
import java.text.SimpleDateFormat
Expand All @@ -54,7 +52,6 @@ import kotlinx.coroutines.launch

@HiltViewModel
class VideoEditScreenViewModel @Inject constructor(
@ApplicationContext private val application: Context,
private val repository: ChatRepository,
) : ViewModel() {

Expand All @@ -67,14 +64,17 @@ class VideoEditScreenViewModel @Inject constructor(
private val _isProcessing = MutableStateFlow(false)
val isProcessing: StateFlow<Boolean> = _isProcessing

private var _videoSaveState = MutableStateFlow(VideoSaveState.PENDING)
donovanfm marked this conversation as resolved.
Show resolved Hide resolved
val videoSaveState: StateFlow<VideoSaveState> = _videoSaveState

fun setChatId(chatId: Long) {
this.chatId.value = chatId
}

private val transformerListener: Transformer.Listener =
@UnstableApi object : Transformer.Listener {
override fun onCompleted(composition: Composition, exportResult: ExportResult) {
Toast.makeText(application, "Edited video saved", Toast.LENGTH_LONG).show()
_videoSaveState.value = VideoSaveState.VIDEO_SAVE_SUCCESS

sendVideo()

Expand All @@ -88,8 +88,7 @@ class VideoEditScreenViewModel @Inject constructor(
exportException: ExportException,
) {
exportException.printStackTrace()
Toast.makeText(application, "Error applying edits on video", Toast.LENGTH_LONG)
.show()
_videoSaveState.value = VideoSaveState.VIDEO_SAVE_FAIL
_isProcessing.value = false
}
}
Expand Down Expand Up @@ -183,3 +182,9 @@ class VideoEditScreenViewModel @Inject constructor(
}
}
}

enum class VideoSaveState {
PENDING,
VIDEO_SAVE_SUCCESS,
VIDEO_SAVE_FAIL,
}
donovanfm marked this conversation as resolved.
Show resolved Hide resolved