Skip to content

Migrate ResultsScreen to use Nav3 #95

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,18 @@ import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.unit.IntOffset
import androidx.core.net.toUri
import androidx.lifecycle.viewmodel.navigation3.rememberViewModelStoreNavEntryDecorator
import androidx.navigation3.runtime.entry
import androidx.navigation3.runtime.entryProvider
import androidx.navigation3.runtime.rememberSavedStateNavEntryDecorator
import androidx.navigation3.ui.NavDisplay
import com.android.developers.androidify.camera.CameraPreviewScreen
import com.android.developers.androidify.creation.CreationScreen
import com.android.developers.androidify.customize.CustomizeAndExportScreen
import com.android.developers.androidify.home.AboutScreen
import com.android.developers.androidify.home.HomeScreen
import com.android.developers.androidify.results.ResultsScreen
import com.android.developers.androidify.theme.transitions.ColorSplashTransitionScreen
import com.google.android.gms.oss.licenses.OssLicensesMenuActivity

Expand Down Expand Up @@ -110,6 +113,51 @@ fun MainNavigation() {
onAboutPressed = {
backStack.add(About)
},
onImageCreated = { resultImageUri, prompt, originalImageUri ->
backStack.removeAll{ it is ImageResult}
backStack.add(
ImageResult(
result = resultImageUri.toString(),
prompt = prompt,
originalImageUri = originalImageUri?.toString()
)
)
}
Comment on lines +116 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The current implementation of onImageCreated adds the ImageResult screen on top of the Create screen in the back stack. This leads to an incorrect navigation flow when pressing the back button from the results screen, as it would create a stack of Create screens ([..., Create, Create]).

To fix this, you should replace the Create screen with the ImageResult screen in the back stack. This ensures that navigating back from the results screen correctly returns the user to a single, updated creation screen.

                    onImageCreated = { resultImageUri, prompt, originalImageUri ->
                        backStack.removeAll { it is ImageResult }
                        // Replace the current entry (Create) with ImageResult to ensure a correct back stack
                        if (backStack.isNotEmpty()) {
                            backStack[backStack.lastIndex] = ImageResult(
                                result = resultImageUri.toString(),
                                prompt = prompt,
                                originalImageUri = originalImageUri?.toString()
                            )
                        } else {
                            backStack.add(
                                ImageResult(
                                    result = resultImageUri.toString(),
                                    prompt = prompt,
                                    originalImageUri = originalImageUri?.toString()
                                )
                            )
                        }
                    }

)
}
entry<ImageResult> { resultKey ->
ResultsScreen(
resultImageUri = resultKey.result.toUri(),
originalImageUri = resultKey.originalImageUri?.toUri(),
onNextPress = { resultImageUri, originalImageUri ->
backStack.removeAll{ it is ImageResult}
backStack.add(
ShareResult(
resultUri = resultImageUri.toString(),
originalImageUri = originalImageUri?.toString()
)
)
},
promptText = resultKey.prompt,
onAboutPress = {
backStack.add(About)
},
onBackPress = {
backStack.removeLastOrNull()
backStack.add(Create(fileName = resultKey.originalImageUri, prompt = resultKey.prompt))
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand, we shouldn't need to do this as the Create() entry should already be on the backstack after removing the last one?

}
)
}
entry<ShareResult> { shareKey ->
CustomizeAndExportScreen(
resultImageUri = shareKey.resultUri.toUri(),
originalImageUri = shareKey.originalImageUri?.toUri(),
onBackPress = {
backStack.removeLastOrNull()
},
onInfoPress = {
backStack.add(About)
}
)
}
entry<About> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,9 @@ object Camera : NavigationRoute

@Serializable
object About : NavigationRoute

@Serializable
data class ImageResult(val originalImageUri: String? = null, val prompt: String? = null, val result: String) : NavigationRoute

@Serializable
data class ShareResult(val resultUri: String, val originalImageUri: String?) : NavigationRoute
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ fun CreationScreen(
onCameraPressed: () -> Unit = {},
onBackPressed: () -> Unit,
onAboutPressed: () -> Unit,
onImageCreated: (resultImageUri: Uri, prompt: String?, originalImageUri: Uri?) -> Unit,
) {
val uiState by creationViewModel.uiState.collectAsStateWithLifecycle()
BackHandler(
Expand Down Expand Up @@ -213,44 +214,16 @@ fun CreationScreen(
}

ScreenState.RESULT -> {
val prompt = uiState.descriptionText.text.toString()
val key = if (uiState.descriptionText.text.isBlank()) {
uiState.imageUri.toString()
} else {
prompt
}
ResultsScreen(
uiState.resultBitmap!!,
onImageCreated(
uiState.resultBitmapUri!!,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of !! is strongly discouraged. Either resultBitmapUri cannot be null or it can and the null case should be handled.

uiState.descriptionText.text.toString(),
if (uiState.selectedPromptOption == PromptType.PHOTO) {
uiState.imageUri
} else {
null
},
promptText = prompt,
viewModel = hiltViewModel(key = key),
onAboutPress = onAboutPressed,
onBackPress = onBackPressed,
onNextPress = creationViewModel::customizeExportClicked,
}
Comment on lines 216 to +224
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should likely be able to remove this state now as the Result screen state is handled by Nav3

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we should only have here, Edit and Loading.

)
}

ScreenState.CUSTOMIZE -> {
val prompt = uiState.descriptionText.text.toString()
val key = if (uiState.descriptionText.text.isBlank()) {
uiState.imageUri.toString()
} else {
prompt
}
uiState.resultBitmap?.let { bitmap ->
CustomizeAndExportScreen(
resultImage = bitmap,
originalImageUri = uiState.imageUri,
onBackPress = onBackPressed,
onInfoPress = onAboutPressed,
viewModel = hiltViewModel<CustomizeExportViewModel>(key = key),
)
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package com.android.developers.androidify.creation

import android.content.Context
import android.graphics.Bitmap
import android.net.Uri
import android.util.Log
import androidx.compose.foundation.text.input.TextFieldState
Expand Down Expand Up @@ -153,7 +152,7 @@ class CreationViewModel @Inject constructor(
)
}
_uiState.update {
it.copy(resultBitmap = bitmap, screenState = ScreenState.RESULT)
it.copy(resultBitmapUri = imageGenerationRepository.saveImage(bitmap), screenState = ScreenState.RESULT)
}
} catch (e: Exception) {
handleImageGenerationError(e)
Expand Down Expand Up @@ -220,25 +219,13 @@ class CreationViewModel @Inject constructor(

ScreenState.RESULT -> {
_uiState.update {
it.copy(screenState = ScreenState.EDIT, resultBitmap = null)
it.copy(screenState = ScreenState.EDIT, resultBitmapUri = null)
}
}

ScreenState.EDIT -> {
// do nothing, back press handled outside
}

ScreenState.CUSTOMIZE -> {
_uiState.update {
it.copy(screenState = ScreenState.RESULT)
}
}
}
}

fun customizeExportClicked() {
_uiState.update {
it.copy(screenState = ScreenState.CUSTOMIZE)
}
}
}
Expand All @@ -252,14 +239,13 @@ data class CreationState(
val generatedPrompt: String? = null,
val promptGenerationInProgress: Boolean = false,
val screenState: ScreenState = ScreenState.EDIT,
val resultBitmap: Bitmap? = null,
val resultBitmapUri: Uri? = null,
)

enum class ScreenState {
EDIT,
LOADING,
RESULT,
CUSTOMIZE,
}

data class BotColor(
Expand Down Expand Up @@ -300,4 +286,4 @@ private fun getBotColors(): List<BotColor> {
enum class PromptType(val displayName: String) {
PHOTO("Photo"),
TEXT("Prompt"),
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class CreationViewModelTest {
viewModel.onSelectedPromptOptionChanged(PromptType.PHOTO)
viewModel.startClicked()
assertEquals(ScreenState.RESULT, viewModel.uiState.value.screenState)
assertNotNull(viewModel.uiState.value.resultBitmap)
assertNotNull(viewModel.uiState.value.resultBitmapUri)
}

@Test
Expand Down Expand Up @@ -178,7 +178,7 @@ class CreationViewModelTest {
}
viewModel.startClicked()
assertEquals(ScreenState.RESULT, viewModel.uiState.value.screenState)
assertNotNull(viewModel.uiState.value.resultBitmap)
assertNotNull(viewModel.uiState.value.resultBitmapUri)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ class ResultsScreenTest {
val composeTestRule = createAndroidComposeRule<ComponentActivity>()

// Create a dummy bitmap for testing
private val testBitmap: Bitmap = Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888)
val dummyUri = android.net.Uri.parse("dummy://image")

@Test
fun resultsScreenContents_displaysActionButtons() {
val shareButtonText = composeTestRule.activity.getString(R.string.customize_and_share)
// Note: Download button is identified by icon, harder to test reliably without tags/desc

val initialState = ResultState(resultImageBitmap = testBitmap, promptText = "test")
val initialState = ResultState(resultImageUri = dummyUri, promptText = "test")
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down Expand Up @@ -78,7 +78,7 @@ class ResultsScreenTest {
val frontCardDesc = composeTestRule.activity.getString(R.string.resultant_android_bot)

// Ensure promptText is non-null when bitmap is present
val initialState = ResultState(resultImageBitmap = testBitmap, promptText = "test")
val initialState = ResultState(resultImageUri = dummyUri, promptText = "test")
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down Expand Up @@ -108,7 +108,7 @@ class ResultsScreenTest {
val backCardDesc = composeTestRule.activity.getString(R.string.original_image)
val dummyUri = android.net.Uri.parse("dummy://image")

val initialState = ResultState(resultImageBitmap = testBitmap, originalImageUrl = dummyUri)
val initialState = ResultState(resultImageUri = dummyUri, originalImageUrl = dummyUri)
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down Expand Up @@ -143,7 +143,7 @@ class ResultsScreenTest {
val promptText = "test prompt"
val promptPrefix = composeTestRule.activity.getString(R.string.my_bot_is_wearing)

val initialState = ResultState(resultImageBitmap = testBitmap, promptText = promptText) // No original image URI
val initialState = ResultState(resultImageUri = dummyUri, promptText = promptText) // No original image URI
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down Expand Up @@ -175,7 +175,7 @@ class ResultsScreenTest {
val frontCardDesc = composeTestRule.activity.getString(R.string.resultant_android_bot)
val dummyUri = android.net.Uri.parse("dummy://image")

val initialState = ResultState(resultImageBitmap = testBitmap, originalImageUrl = dummyUri)
val initialState = ResultState(resultImageUri = dummyUri, originalImageUrl = dummyUri)
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down Expand Up @@ -210,7 +210,7 @@ class ResultsScreenTest {
var shareClicked = false

// Ensure promptText is non-null when bitmap is present
val initialState = ResultState(resultImageBitmap = testBitmap, promptText = "test")
val initialState = ResultState(resultImageUri = dummyUri, promptText = "test")
val state = mutableStateOf(initialState)

composeTestRule.setContent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.draw.dropShadow
import androidx.compose.ui.graphics.ImageBitmap
import androidx.compose.ui.graphics.asAndroidBitmap
import androidx.compose.ui.graphics.shadow.Shadow
import androidx.compose.ui.graphics.vector.ImageVector
import androidx.compose.ui.layout.LookaheadScope
Expand All @@ -72,6 +71,7 @@ import androidx.compose.ui.res.stringResource
import androidx.compose.ui.res.vectorResource
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import androidx.core.net.toUri
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.navigation3.ui.LocalNavAnimatedContentScope
Expand All @@ -97,15 +97,15 @@ import com.android.developers.androidify.theme.R as ThemeR
@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun CustomizeAndExportScreen(
resultImage: Bitmap,
resultImageUri: Uri,
originalImageUri: Uri?,
onBackPress: () -> Unit,
onInfoPress: () -> Unit,
isMediumWindowSize: Boolean = isAtLeastMedium(),
viewModel: CustomizeExportViewModel = hiltViewModel<CustomizeExportViewModel>(),
) {
LaunchedEffect(resultImage, originalImageUri) {
viewModel.setArguments(resultImage, originalImageUri)
LaunchedEffect(resultImageUri, originalImageUri) {
viewModel.setArguments(resultImageUri, originalImageUri)
}
val state = viewModel.state.collectAsStateWithLifecycle()
val context = LocalContext.current
Expand Down Expand Up @@ -426,9 +426,9 @@ fun CustomizeExportPreview() {
AnimatedContent(true) { targetState ->
targetState
CompositionLocalProvider(LocalNavAnimatedContentScope provides this@AnimatedContent) {
val bitmap = ImageBitmap.imageResource(R.drawable.placeholderbot)
val imageUri = ("android.resource://com.android.developers.androidify.results/" + R.drawable.placeholderbot).toUri()
val state = CustomizeExportState(
exportImageCanvas = ExportImageCanvas(imageBitmap = bitmap.asAndroidBitmap()),
exportImageCanvas = ExportImageCanvas(imageUri = imageUri),
)
CustomizeExportContents(
state = state,
Expand All @@ -453,10 +453,10 @@ fun CustomizeExportPreviewLarge() {
AnimatedContent(true) { targetState ->
targetState
CompositionLocalProvider(LocalNavAnimatedContentScope provides this@AnimatedContent) {
val bitmap = ImageBitmap.imageResource(R.drawable.placeholderbot)
val imageUri = ("android.resource://com.android.developers.androidify.results/" + R.drawable.placeholderbot).toUri()
val state = CustomizeExportState(
exportImageCanvas = ExportImageCanvas(
imageBitmap = bitmap.asAndroidBitmap(),
imageUri = imageUri,
aspectRatioOption = SizeOption.Square,
),
selectedTool = CustomizeTool.Background,
Expand Down
Loading
Loading