-
Notifications
You must be signed in to change notification settings - Fork 582
Migrate from LiveData to Kotlin Flow #518
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
Migrate from LiveData to Kotlin Flow #518
Conversation
# Conflicts: # app/src/main/assets/nostr_relays.csv
# Conflicts: # app/src/main/assets/nostr_relays.csv
This commit removes the `androidx.lifecycle:lifecycle-livedata-ktx` library from the project's dependencies. The `[libraries]` and `[bundles]` sections in `gradle/libs.versions.toml` have been updated to reflect this removal, as the dependency is no longer in use.
…` to StateFlow
This commit refactors `LocationChannelManager` and `GeohashBookmarksStore` to use `StateFlow` instead of `LiveData` for managing and exposing their state. This change aligns with modern Android development practices and improves testability.
**Key Changes:**
- **`LocationChannelManager`**:
- All `MutableLiveData` properties (`permissionState`, `availableChannels`, `selectedChannel`, etc.) have been replaced with `MutableStateFlow`.
- Consumers now access these properties as `StateFlow`.
- State updates have been changed from `postValue()` to direct `.value` assignments, simplifying thread management within the manager which already uses a dedicated coroutine scope.
- **`GeohashBookmarksStore`**:
- `bookmarks` and `bookmarkNames` are now exposed as `StateFlow` instead of `LiveData`.
- State updates similarly use `.value` assignment.
- **Nullability**:
- The non-nullable nature of `StateFlow`'s value reduces the need for null-checks in both the manager classes and their consumers, leading to safer code.
This commit replaces `LiveData` with `StateFlow` across core Nostr-related classes to align with modern Android architecture and improve state management. This change affects `NostrClient`, `NostrRelayManager`, `LocationNotesManager`, and `GeohashRepository`.
**Key Changes:**
- **`NostrClient`**:
- `isInitialized` and `currentNpub` are now `StateFlow` instead of `LiveData`.
- `relayConnectionStatus` and `relayInfo` now return `StateFlow` from `NostrRelayManager`.
- **`NostrRelayManager`**:
- Public properties `relays` and `isConnected` are migrated from `MutableLiveData` to `MutableStateFlow`.
- Updates are now pushed using `.value` instead of `.postValue()`.
- **`LocationNotesManager`**:
- All public `LiveData` properties (`notes`, `geohash`, `initialLoadComplete`, `state`, `errorMessage`) are converted to `StateFlow`.
- The class documentation is updated to reflect the use of `StateFlow`.
- **`GeohashRepository`**:
- Methods `updateGeohashPeople` and `updateReactiveParticipantCounts` now call `set...` methods on the `state` object instead of `post...`, reflecting the removal of `LiveData` from the underlying state management.
This commit refactors the `ChatState`, `ChatViewModel`, and `GeohashViewModel` to use `StateFlow` instead of `LiveData` for managing and exposing UI state. This migration improves state management by leveraging modern coroutine-based flows.
**Key Changes:**
- **`ChatState.kt`**:
- Replaced all `MutableLiveData` instances with `MutableStateFlow`.
- Exposed state properties as `StateFlow` instead of `LiveData`.
- Removed `MediatorLiveData` for computed properties (`hasUnreadChannels`, `hasUnreadPrivateMessages`) and replaced them with `Flow.combine` to create derivative `StateFlows`.
- Simplified non-nullable `getters` to directly return the `.value` of the `StateFlows`.
- Removed `postValue` helpers that are no longer necessary.
- **`ChatViewModel.kt`**:
- Updated all state properties to be `StateFlow`, reflecting the changes in `ChatState`.
- **`GeohashViewModel.kt`**:
- Changed state properties (`geohashPeople`, `geohashParticipantCounts`, etc.) from `LiveData` to `StateFlow`.
- Replaced `observeForever` on `LiveData` from `LocationChannelManager` with `viewModelScope.launch` blocks that `.collect()` from the underlying flows.
This commit replaces `LiveData.observeAsState()` with `StateFlow.collectAsState()` across various UI components. This change aligns the codebase with modern Android development practices, using Kotlin Flows for reactive UI state management. No functional changes are intended. The primary goal is to remove the dependency on `androidx.lifecycle.livedata` from the composable functions. **Affected Components:** - `ChatScreen` - `SidebarComponents` - `ChatHeader` - `LocationChannelsSheet` - `LocationNotesSheet` - `LocationNotesButton` - `GeohashPeopleList` - `LocationNotesSheetPresenter`
| @Composable | ||
| fun CloseButton( | ||
| onClick: () -> Unit, | ||
| modifier: Modifier = Modifier | ||
| ) { | ||
| IconButton( | ||
| onClick = onClick, | ||
| modifier = modifier | ||
| .size(32.dp), | ||
| colors = IconButtonDefaults.iconButtonColors( | ||
| contentColor = MaterialTheme.colorScheme.onBackground.copy(alpha = 0.6f), | ||
| containerColor = MaterialTheme.colorScheme.onBackground.copy(alpha = 0.1f) | ||
| ) | ||
| ) { | ||
| Icon( | ||
| imageVector = Icons.Default.Close, | ||
| contentDescription = "Close", | ||
| modifier = Modifier.size(18.dp) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CloseButton composable was defined in AboutSheet.kt but is being used in AboutSheet.kt and LocationChannelsSheet.kt, consider moving this into a separate core/ui or core/designsystem if it's gonna be used across the app, but also it's preferable to have PRs focused on 1 thing, for example this is a PR to migrate Live data to Flow. Adding non related stuff would complicate it but would be fine if it wasn't too much changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea. Apparently, when switching from another branch, I accidentally transferred the UI changes to the flow migration.
| val favoritePeers by viewModel.favoritePeers.collectAsState() | ||
| val peerFingerprints by viewModel.peerFingerprints.collectAsState() | ||
| val peerSessionStates by viewModel.peerSessionStates.collectAsState() | ||
| val peerNicknames by viewModel.peerNicknames.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using collectAsStateWithLifecycle() instead as most UI stuff shouldn't keep collecting when app is in background
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsState() | ||
| val geohashPeople by viewModel.geohashPeople.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as https://github.com/permissionlesstech/bitchat-android/pull/518/files#r2574005584
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsState() | |
| val geohashPeople by viewModel.geohashPeople.collectAsState() | |
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsStateWithLifecycle() | |
| val geohashPeople by viewModel.geohashPeople.collectAsStateWithLifeCycle() |
| val connectedPeers by viewModel.connectedPeers.collectAsState() | ||
| val joinedChannels by viewModel.joinedChannels.collectAsState() | ||
| val hasUnreadChannels by viewModel.unreadChannelMessages.collectAsState() | ||
| val hasUnreadPrivateMessages by viewModel.unreadPrivateMessages.collectAsState() | ||
| val isConnected by viewModel.isConnected.collectAsState() | ||
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsState() | ||
| val geohashPeople by viewModel.geohashPeople.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| val connectedPeers by viewModel.connectedPeers.collectAsState() | |
| val joinedChannels by viewModel.joinedChannels.collectAsState() | |
| val hasUnreadChannels by viewModel.unreadChannelMessages.collectAsState() | |
| val hasUnreadPrivateMessages by viewModel.unreadPrivateMessages.collectAsState() | |
| val isConnected by viewModel.isConnected.collectAsState() | |
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsState() | |
| val geohashPeople by viewModel.geohashPeople.collectAsState() | |
| val connectedPeers by viewModel.connectedPeers.collectAsStateWithLifeCycle() | |
| val joinedChannels by viewModel.joinedChannels.collectAsStateWithLifeCycle() | |
| val hasUnreadChannels by viewModel.unreadChannelMessages.collectAsStateWithLifeCycle() | |
| val hasUnreadPrivateMessages by viewModel.unreadPrivateMessages.collectAsStateWithLifeCycle() | |
| val isConnected by viewModel.isConnected.collectAsStateWithLifeCycle() | |
| val selectedLocationChannel by viewModel.selectedLocationChannel.collectAsStateWithLifeCycle() | |
| val geohashPeople by viewModel.geohashPeople.collectAsStateWithLifeCycle() |
| val context = androidx.compose.ui.platform.LocalContext.current | ||
| val bookmarksStore = remember { com.bitchat.android.geohash.GeohashBookmarksStore.getInstance(context) } | ||
| val bookmarks by bookmarksStore.bookmarks.observeAsState(emptyList()) | ||
| val bookmarks by bookmarksStore.bookmarks.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| val bookmarks by bookmarksStore.bookmarks.collectAsState() | |
| val bookmarks by bookmarksStore.bookmarks.collectAsStateWithLifeCycle() |
| } | ||
| } | ||
| .stateIn( | ||
| scope = CoroutineScope(Dispatchers.Default), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass scope to class constructor.
| hasUnreadPrivateMessages.value = unreadSet.isNotEmpty() | ||
| .stateIn( | ||
| scope = CoroutineScope(Dispatchers.Default), | ||
| started = SharingStarted.Eagerly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need Eagerly here? We should aim to use WhileSubscribed(5_000), which keeps subscribing for 5 seconds. So anytime user puts the app in background for more than 5 seconds, we stop collecting to save on resources.
5 seconds is ideal due because if a device configuration happens, we do not pause and recollect again since we are still within the 5 seconds limit.
| } | ||
| } | ||
| .stateIn( | ||
| scope = CoroutineScope(Dispatchers.Default), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before
| } | ||
| .stateIn( | ||
| scope = CoroutineScope(Dispatchers.Default), | ||
| started = SharingStarted.Eagerly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment, we might not need eagerly in here if it's just a UI thing that is not needed in background.
| androidx-compose-runtime-livedata = { module = "androidx.compose.runtime:runtime-livedata" } | ||
| androidx-compose-material-icons-extended = { module = "androidx.compose.material:material-icons-extended" } | ||
|
|
||
| # Lifecycle | ||
| androidx-lifecycle-viewmodel-compose = { module = "androidx.lifecycle:lifecycle-viewmodel-compose", version.ref = "lifecycle-runtime" } | ||
| androidx-lifecycle-livedata-ktx = { module = "androidx.lifecycle:lifecycle-livedata-ktx", version.ref = "lifecycle-runtime" } | ||
|
|
||
| # Navigation | ||
| androidx-navigation-compose = { module = "androidx.navigation:navigation-compose", version.ref = "navigation-compose" } | ||
|
|
||
| # Accompanist | ||
| accompanist-permissions = { module = "com.google.accompanist:accompanist-permissions", version.ref = "accompanist-permissions" } | ||
|
|
||
| # Cryptography | ||
| bouncycastle-bcprov = { module = "org.bouncycastle:bcprov-jdk15on", version.ref = "bouncycastle" } | ||
| google-tink-android = { module = "com.google.crypto.tink:tink-android", version.ref = "tink-android" } | ||
|
|
||
| # JSON | ||
| gson = { module = "com.google.code.gson:gson", version.ref = "gson" } | ||
|
|
||
| # Coroutines | ||
| kotlinx-coroutines-android = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-android", version.ref = "kotlinx-coroutines" } | ||
|
|
||
| # Bluetooth | ||
| nordic-ble = { module = "no.nordicsemi.android:ble", version.ref = "nordic-ble" } | ||
|
|
||
| # WebSocket | ||
| okhttp = { module = "com.squareup.okhttp3:okhttp", version.ref = "okhttp" } | ||
| tor-android-binary = { module = "org.torproject:tor-android-binary", version.ref = "tor-android-binary" } | ||
|
|
||
| # Tor (embed) intentionally not pinned yet; add once repo is chosen | ||
|
|
||
| # Google Play Services | ||
| gms-location = { module = "com.google.android.gms:play-services-location", version.ref = "gms-location" } | ||
|
|
||
| # Security | ||
| androidx-security-crypto = { module = "androidx.security:security-crypto", version.ref = "security-crypto" } | ||
|
|
||
| # Testing | ||
| junit = { module = "junit:junit", version.ref = "junit" } | ||
| androidx-test-ext-junit = { module = "androidx.test.ext:junit", version.ref = "androidx-test-ext" } | ||
| androidx-test-espresso-core = { module = "androidx.test.espresso:espresso-core", version.ref = "espresso" } | ||
| androidx-compose-ui-test-junit4 = { module = "androidx.compose.ui:ui-test-junit4" } | ||
| androidx-compose-ui-test-manifest = { module = "androidx.compose.ui:ui-test-manifest" } | ||
| mockito-kotlin = { module = "org.mockito.kotlin:mockito-kotlin", version.ref = "mockito-kotlin" } | ||
| mockito-inline = { module = "org.mockito:mockito-inline", version.ref = "mockito-inline" } | ||
| roboelectric = { module = "org.robolectric:robolectric", version.ref = "roboelectric"} | ||
| kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "kotlinx-coroutines-test"} | ||
|
|
||
| [plugins] | ||
| android-application = { id = "com.android.application", version.ref = "agp" } | ||
| android-library = { id = "com.android.library", version.ref = "agp" } | ||
| kotlin-android = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" } | ||
| kotlin-parcelize = { id = "kotlin-parcelize" } | ||
| kotlin-compose = { id = "org.jetbrains.kotlin.plugin.compose", version.ref = "kotlin" } | ||
|
|
||
| [bundles] | ||
| compose = [ | ||
| "androidx-compose-ui", | ||
| "androidx-compose-ui-graphics", | ||
| "androidx-compose-ui-tooling-preview", | ||
| "androidx-compose-material3", | ||
| "androidx-compose-runtime-livedata", | ||
| "androidx-compose-material-icons-extended" | ||
| ] | ||
|
|
||
| lifecycle = [ | ||
| "androidx-lifecycle-runtime-ktx", | ||
| "androidx-lifecycle-viewmodel-compose", | ||
| "androidx-lifecycle-livedata-ktx" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no change needed, but consider making such changes as 1 commit in the future, since it's just removing 2 lines and they're related.
This commit refactors the `ChatState` class to accept a `CoroutineScope` in its constructor instead of creating its own. **Key Changes:** - **`ChatState.kt`**: The constructor now requires a `CoroutineScope`. This scope is used for the `stateIn` operators that convert `Flows` into `StateFlows` (`hasUnreadChannels`, `hasUnreadPrivateMessages`), ensuring they operate within the lifecycle of the provided scope. - **`ChatViewModel.kt`**: The `viewModelScope` is now passed to the `ChatState` constructor during its instantiation. This ties the lifecycle of the state's coroutines directly to the `ViewModel`'s lifecycle.
This commit refactors `CommandProcessorTest` to use a `TestScope` and `UnconfinedTestDispatcher` for managing coroutines. This ensures that coroutine-based operations within the test are executed in a controlled and predictable manner, improving test reliability. The `coroutineScope` for `CommandProcessor` and the `scope` for `ChatState` are now both configured to use this test-specific scope.
Description
This pull request completes the migration from LiveData to Kotlin Flow and StateFlow for managing observable data streams throughout the application. This aligns our codebase with modern Android development best practices, improves resource management, and integrates more cleanly with Jetpack Compose.
Summary of Changes