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

Feature: Use type safe view states #73

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

Feature: Use type safe view states #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2251484
CHANGE to ignore kotlin generated caches
sebaslogen Aug 22, 2024
2042b01
CHANGE to use type-safe view state to avoid illegal states
sebaslogen Aug 22, 2024
9851a7e
Fix unit tests
sebaslogen Aug 22, 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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
*.iml
.gradle
.kotlin
/local.properties
/.idea/caches
/.idea/libraries
Expand Down
7 changes: 4 additions & 3 deletions data/user/src/main/kotlin/nl/q42/template/data/user/local/UserLocalDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
package nl.q42.template.data.user.local

import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.update
import nl.q42.template.data.user.local.model.UserEntity
import javax.inject.Inject

internal class UserLocalDataSource @Inject constructor() {

private val userFlow = MutableStateFlow<UserEntity?>(null) // this is dummy code, replace it with your own local storage implementation.
private val userFlow = MutableSharedFlow<UserEntity?>() // this is dummy code, replace it with your own local storage implementation.

fun setUser(userEntity: UserEntity) {
suspend fun setUser(userEntity: UserEntity) {

// usually you store in DataStore or DB here...

userFlow.update { userEntity } // this is dummy code, replace it with your own local storage implementation.
userFlow.emit(userEntity) // this is dummy code, replace it with your own local storage implementation.
}

fun getUserFlow(): Flow<UserEntity?> = userFlow
Expand Down
17 changes: 7 additions & 10 deletions feature/home/src/main/kotlin/nl/q42/template/home/main/presentation/HomeViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
import nl.q42.template.actionresult.data.handleAction
import nl.q42.template.domain.user.usecase.FetchUserUseCase
Expand All @@ -29,7 +28,7 @@ class HomeViewModel @Inject constructor(
private val navigator: RouteNavigator,
) : ViewModel(), RouteNavigator by navigator {

private val _uiState = MutableStateFlow<HomeViewState>(HomeViewState())
private val _uiState = MutableStateFlow<HomeViewState>(HomeViewState.Loading)
val uiState: StateFlow<HomeViewState> = _uiState.asStateFlow()

init {
Expand Down Expand Up @@ -58,23 +57,21 @@ class HomeViewModel @Inject constructor(
fun fetchUser() {
viewModelScope.launch {

_uiState.update { it.copy(showError = false, isLoading = true) }
_uiState.value = HomeViewState.Loading

handleAction(
action = fetchUserUseCase(),
onError = { _uiState.update { it.copy(showError = true, isLoading = false) } },
onSuccess = { _uiState.update { it.copy(isLoading = false) } },
onError = { _uiState.value = HomeViewState.Error },
onSuccess = {},
)
}
}

private fun startObservingUserChanges() {
getUserFlowUseCase().filterNotNull().onEach { user ->
_uiState.update {
it.copy(
userEmailTitle = ViewStateString.Res(R.string.emailTitle, user.email.value)
)
}
_uiState.value = HomeViewState.Content(
userEmailTitle = ViewStateString.Res(R.string.emailTitle, user.email.value)
)
}.launchIn(viewModelScope)
}
}
10 changes: 5 additions & 5 deletions feature/home/src/main/kotlin/nl/q42/template/home/main/presentation/HomeViewState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package nl.q42.template.home.main.presentation

import nl.q42.template.ui.presentation.ViewStateString

data class HomeViewState(
val userEmailTitle: ViewStateString? = null,
val isLoading: Boolean = false,
val showError: Boolean = false,
)
sealed interface HomeViewState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe sealed ViewStates are inconvenient in usage. I tried it a few times, but always move away from them eventually, when not doing SDUI. Some of the reasons:

  1. They presume you never load while showing content. (pull to refresh, for example, or partial loading).
  2. They presume you never error (like a snackbar or any non fullscreen error) when showing old or partial content or loading.
  3. It's inconvenient when you get content from different sources (multiple observables from the domain layer, callbacks from something, function calls from the view). You need to be able to create or copy the ViewState.Content (so support both: copy and create) in all locations where you want to adjust something in the ViewState's content.

Unless you have a fix for this, I believe with the old ViewState you are more flexible and have to write less code to change values in the content.

Copy link
Collaborator Author

@sebaslogen sebaslogen Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback, I hadn't though on the perspective of SDUI vs non-SDUI.

My ideas through the reasons with your new perspective of non-SDUI apps:

  1. This we solved for Hema and PostNL (SDUI), but I think the same solution applies for non-SDUI apps: the app defines clear ViewStates for Full-screen-loading vs content+loading, it would look like this:
sealed interface HomeViewState {
    data class Content(val isLoadingWithPTR: Boolean, val userEmailTitle: ViewStateString) : HomeViewState
    data object FullScreenLoader : HomeViewState
    data object Error : HomeViewState
}

The advantage here is that dev has to make an explicit decision about which type of loader (s)he wants to show, making the case of showing a PTR loader when there is no content impossible.

  1. The same concept applies for full screen errors and transient errors on both Hema and PostNL:
sealed interface HomeViewState {
    data class Content(val transientError: MyErrorType?, val userEmailTitle: ViewStateString) : HomeViewState
    data object Loader : HomeViewState
    data object FullScreenError : HomeViewState
}

In this scenario, normally full-screen-errors are just a simple boolean, and transient-errors (like snackbars) can contain more complex logic like a message plus an action of a more complex type. So this distinction is an advantage.

  1. For this one I wanna make sure I got point correctly, what I understand is:
    1. for example: the ViewState has some content with 3 items that are not favorite
    2. the user favorites element 1 on the UI layer and a call to the ViewModel is triggered to change the data model
    3. the inconvenience you mention is that in the VM the dev should write something like:
fun onFavoriteClicked(index: Int) {
  when(viewState) {
    is HomeViewState.Content -> {
      viewState.items[index].isFavorite = true
    }
    HomeViewState.Loading, HomeViewState.Error -> {} // no-op or throw IllegalStateException()
  }
}

instead of just

fun onFavoriteClicked(index: Int) {
  viewState.items?.get(index)?.isFavorite = true
}

Is this understanding correct? Please correct me if I misunderstood this point. Otherwise, this is my proposed solution:
Since we can only modify content once we already have content, we can start those functions with a check, either a soft one or a hard one:

fun onFavoriteClicked(index: Int) {
  if (viewState !is HomeViewState.Content ) return // or throw IllegalStateException()
  viewState.items[index].isFavorite = true
}

This has the advantage of making the code simple and readable but still able to catch bugs faster.

Let me know what you think of these solutions and if you think we can reach a more convenient and also safe solution.

data class Content(val userEmailTitle: ViewStateString) : HomeViewState
data object Loading : HomeViewState
data object Error : HomeViewState
}
23 changes: 13 additions & 10 deletions feature/home/src/main/kotlin/nl/q42/template/home/main/ui/HomeContent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ internal fun HomeContent(
horizontalAlignment = CenterHorizontally,
) {

/**
* This is dummy. Use the strings file IRL.
*/
viewState.userEmailTitle?.get()?.let { Text(text = it) }

if (viewState.isLoading) CircularProgressIndicator()
if (viewState.showError) Text(text = "Error")
when(viewState) {
is HomeViewState.Content -> {
/**
* This is dummy. Use the strings file IRL.
*/
Text(text = viewState.userEmailTitle.get())
}
HomeViewState.Loading -> CircularProgressIndicator()
HomeViewState.Error -> Text(text = "Error")
}

Button(onClick = onLoadClicked) {
Text("Refresh")
Expand All @@ -55,22 +58,22 @@ internal fun HomeContent(
@Composable
private fun HomeContentErrorPreview() {
PreviewAppTheme {
HomeContent(HomeViewState(showError = true), {}, {}, {})
HomeContent(HomeViewState.Error, {}, {}, {})
}
}

@PreviewLightDark
@Composable
private fun HomeContentLoadingPreview() {
PreviewAppTheme {
HomeContent(HomeViewState(isLoading = true), {}, {}, {})
HomeContent(HomeViewState.Loading, {}, {}, {})
}
}

@PreviewLightDark
@Composable
private fun HomeContentEmptyPreview() {
PreviewAppTheme {
HomeContent(HomeViewState(userEmailTitle = "[email protected]".toViewStateString()), {}, {}, {})
HomeContent(HomeViewState.Content(userEmailTitle = "[email protected]".toViewStateString()), {}, {}, {})
}
}
4 changes: 2 additions & 2 deletions feature/home/src/test/kotlin/nl/q42/template/home/main/presentation/HomeViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class HomeViewModelTest {
)

viewModel.uiState.test {
assertTrue(awaitItem().isLoading)
assertTrue(awaitItem() == HomeViewState.Loading)
}
}

Expand All @@ -65,7 +65,7 @@ class HomeViewModelTest {
)

viewModel.uiState.test {
assertTrue(awaitItem().showError)
assertTrue(awaitItem() == HomeViewState.Error)
}
}
}