Address review feedback

Change-Id: If336af8fa5c58d6401dfd7a561a1a340ff388175
2 years ago · 37d4b65325
parent cd03bd16e7
commit 37d4b65325
6 changed files with 51 additions and 37 deletions
--- a/app/src/main/java/com/google/samples/apps/nowinandroid/navigation/NiaNavHost.kt
+++ b/app/src/main/java/com/google/samples/apps/nowinandroid/navigation/NiaNavHost.kt
@ -54,9 +54,7 @@ fun NiaNavHost(
        searchScreen(
            onBackClick = navController::popBackStack,
            onInterestsClick = { appState.navigateToTopLevelDestination(INTERESTS) },
-            onTopicClick = {
-                navController.navigateToTopic(it)
-            },
+            onTopicClick = navController::navigateToTopic,
        )
        interestsGraph(
            onTopicClick = { topicId ->
--- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/DefaultSearchContentsRepository.kt
+++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/DefaultSearchContentsRepository.kt
@ -27,7 +27,11 @@ import com.google.samples.apps.nowinandroid.core.network.NiaDispatchers.IO
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.distinctUntilChanged
+import kotlinx.coroutines.flow.flatMapLatest
 import kotlinx.coroutines.flow.flattenConcat
+import kotlinx.coroutines.flow.map
+import kotlinx.coroutines.flow.mapLatest
 import kotlinx.coroutines.withContext
 import javax.inject.Inject

@ -54,16 +58,21 @@ class DefaultSearchContentsRepository @Inject constructor(
        val newsResourceIds = newsResourceFtsDao.searchAllNewsResources("*$searchQuery*")
        val topicIds = topicFtsDao.searchAllTopics("*$searchQuery*")

-        return combine(newsResourceIds, topicIds) { newsFlow, topicsFlow ->
-            combine(
-                newsResourceDao.getNewsResources(filterNewsIds = newsFlow.toSet()),
-                topicDao.getTopicEntities(topicsFlow.toSet()),
-            ) { newsResources, topics ->
-                SearchResult(
-                    topics = topics.map { it.asExternalModel() },
-                    newsResources = newsResources.map { it.asExternalModel() },
-                )
+        val newsResourcesFlow = newsResourceIds
+            .mapLatest { it.toSet() }
+            .distinctUntilChanged()
+            .flatMapLatest {
+                newsResourceDao.getNewsResources(filterNewsIds = it)
            }
-        }.flattenConcat()
+        val topicsFlow = topicIds
+            .mapLatest { it.toSet() }
+            .distinctUntilChanged()
+            .flatMapLatest(topicDao::getTopicEntities)
+        return combine(newsResourcesFlow, topicsFlow) { newsResources, topics ->
+            SearchResult(
+                topics = topics.map { it.asExternalModel() },
+                newsResources = newsResources.map { it.asExternalModel() },
+            )
+        }
    }
 }
--- a/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchResultUiState.kt
+++ b/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchResultUiState.kt
@ -29,6 +29,8 @@ sealed interface SearchResultUiState {
     */
    object EmptyQuery : SearchResultUiState

+    object LoadFailed: SearchResultUiState
+
    data class Success(
        val topics: List<FollowableTopic> = emptyList(),
        val newsResources: List<UserNewsResource> = emptyList(),
--- a/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchScreen.kt
+++ b/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchScreen.kt
@ -43,9 +43,7 @@ import androidx.compose.material3.TextField
 import androidx.compose.material3.TextFieldDefaults
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
-import androidx.compose.runtime.MutableState
 import androidx.compose.runtime.getValue
-import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
@ -91,6 +89,7 @@ internal fun SearchRoute(
    forYouViewModel: ForYouViewModel = hiltViewModel(),
 ) {
    val uiState by searchViewModel.searchResultUiState.collectAsStateWithLifecycle()
+    val searchQuery by searchViewModel.searchQuery.collectAsStateWithLifecycle()
    SearchScreen(
        modifier = modifier,
        onBackClick = onBackClick,
@ -99,6 +98,7 @@ internal fun SearchRoute(
        onSearchQueryChanged = searchViewModel::onSearchQueryChanged,
        onTopicClick = onTopicClick,
        onNewsResourcesCheckedChanged = forYouViewModel::updateNewsResourceSaved,
+        searchQuery = searchQuery,
        uiState = uiState,
    )
 }
@ -112,9 +112,9 @@ internal fun SearchScreen(
    onNewsResourcesCheckedChanged: (String, Boolean) -> Unit = { _, _ -> },
    onSearchQueryChanged: (String) -> Unit = {},
    onTopicClick: (String) -> Unit = {},
+    searchQuery: String = "",
    uiState: SearchResultUiState = SearchResultUiState.Loading,
 ) {
-    val searchQuery = remember { mutableStateOf("") }
    TrackScreenViewEvent(screenName = "Search")
    Column(modifier = modifier) {
        Spacer(Modifier.windowInsetsTopHeight(WindowInsets.safeDrawing))
@ -124,7 +124,8 @@ internal fun SearchScreen(
            searchQuery = searchQuery,
        )
        when (uiState) {
-            SearchResultUiState.Loading -> Unit
+            SearchResultUiState.Loading,
+            SearchResultUiState.LoadFailed,
            SearchResultUiState.EmptyQuery -> Unit
            is SearchResultUiState.Success -> {
                if (uiState.isEmpty()) {
@ -150,12 +151,11 @@ internal fun SearchScreen(
@Composable
 fun EmptySearchResultBody(
    onInterestsClick: () -> Unit = {},
-    searchQuery: MutableState<String>,
+    searchQuery: String,
 ) {
    Column(horizontalAlignment = Alignment.CenterHorizontally) {
-        val queryValue = searchQuery.value
-        val message = stringResource(id = searchR.string.search_result_not_found, queryValue)
-        val start = message.indexOf(queryValue)
+        val message = stringResource(id = searchR.string.search_result_not_found, searchQuery)
+        val start = message.indexOf(searchQuery)
        Text(
            text = AnnotatedString(
                text = message,
@ -163,7 +163,7 @@ fun EmptySearchResultBody(
                    AnnotatedString.Range(
                        SpanStyle(fontWeight = FontWeight.Bold),
                        start = start,
-                        end = start + queryValue.length,
+                        end = start + searchQuery.length,
                    ),
                ),
            ),
@ -261,7 +261,7 @@ private fun SearchToolbar(
    modifier: Modifier = Modifier,
    onBackClick: () -> Unit = {},
    onSearchQueryChanged: (String) -> Unit = {},
-    searchQuery: MutableState<String> = mutableStateOf(""),
+    searchQuery: String = "",
 ) {
    Row(
        verticalAlignment = Alignment.CenterVertically,
@ -286,7 +286,7 @@ private fun SearchToolbar(
@Composable
 private fun SearchTextField(
    onSearchQueryChanged: (String) -> Unit,
-    searchQuery: MutableState<String>,
+    searchQuery: String,
 ) {
    val focusRequester = remember { FocusRequester() }
    TextField(
@ -306,7 +306,6 @@ private fun SearchTextField(
        },
        trailingIcon = {
            IconButton(onClick = {
-                searchQuery.value = ""
                onSearchQueryChanged("")
            }) {
                Icon(
@ -319,7 +318,6 @@ private fun SearchTextField(
            }
        },
        onValueChange = {
-            searchQuery.value = it
            onSearchQueryChanged(it)
        },
        modifier = Modifier
@ -327,7 +325,7 @@ private fun SearchTextField(
            .padding(16.dp)
            .focusRequester(focusRequester),
        shape = RoundedCornerShape(32.dp),
-        value = searchQuery.value,
+        value = searchQuery,
    )
    LaunchedEffect(Unit) {
        focusRequester.requestFocus()
@ -346,8 +344,7 @@ private fun SearchToolbarPreview() {
@Composable
 private fun EmptySearchResultColumnPreview() {
    NiaTheme {
-        val searchQuery = remember { mutableStateOf("C++") }
-        EmptySearchResultBody(searchQuery = searchQuery)
+        EmptySearchResultBody(searchQuery = "C++")
    }
 }

--- a/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchViewModel.kt
+++ b/feature/search/src/main/java/com/google/samples/apps/nowinandroid/feature/search/SearchViewModel.kt
@ -16,11 +16,12 @@

 package com.google.samples.apps.nowinandroid.feature.search

+import androidx.lifecycle.SavedStateHandle
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
 import com.google.samples.apps.nowinandroid.core.domain.GetSearchContentsUseCase
+import com.google.samples.apps.nowinandroid.feature.search.SearchResultUiState.LoadFailed
 import dagger.hilt.android.lifecycle.HiltViewModel
-import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.SharingStarted
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.flatMapLatest
@ -33,20 +34,25 @@ import javax.inject.Inject
 class SearchViewModel @Inject constructor(
    // TODO: Add GetSearchContentsCountUseCase to check if the fts tables are populated
    getSearchContentsUseCase: GetSearchContentsUseCase,
+    private val savedStateHandle: SavedStateHandle,
 ) : ViewModel() {

-    val searchQuery = MutableStateFlow("")
+    val searchQuery = savedStateHandle.getStateFlow("searchQuery", "")

    val searchResultUiState: StateFlow<SearchResultUiState> =
        searchQuery.flatMapLatest { query ->
            if (query.length < 2) {
                flowOf(SearchResultUiState.EmptyQuery)
            } else {
-                getSearchContentsUseCase(query).map {
-                    SearchResultUiState.Success(
-                        topics = it.topics,
-                        newsResources = it.newsResources,
-                    )
+                try {
+                    getSearchContentsUseCase(query).map {
+                        SearchResultUiState.Success(
+                            topics = it.topics,
+                            newsResources = it.newsResources,
+                        )
+                    }
+                } catch (exception: Exception) {
+                    flowOf(LoadFailed)
                }
            }
        }.stateIn(
@ -56,6 +62,6 @@ class SearchViewModel @Inject constructor(
        )

    fun onSearchQueryChanged(query: String) {
-        searchQuery.value = query
+        savedStateHandle["searchQuery"] = query
    }
 }
--- a/feature/search/src/test/java/com/google/samples/apps/nowinandroid/feature/search/SearchViewModelTest.kt
+++ b/feature/search/src/test/java/com/google/samples/apps/nowinandroid/feature/search/SearchViewModelTest.kt
@ -16,6 +16,7 @@

 package com.google.samples.apps.nowinandroid.feature.search

+import androidx.lifecycle.SavedStateHandle
 import com.google.samples.apps.nowinandroid.core.domain.GetSearchContentsUseCase
 import com.google.samples.apps.nowinandroid.core.testing.data.newsResourcesTestData
 import com.google.samples.apps.nowinandroid.core.testing.data.topicsTestData
@ -54,6 +55,7 @@ class SearchViewModelTest {
    fun setup() {
        viewModel = SearchViewModel(
            getSearchContentsUseCase = getSearchContentsUseCase,
+            savedStateHandle = SavedStateHandle()
        )
    }