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

fix: reusing PagingData crash [WPB-15055][WPB-15079][WPB-15064] #3758

Open
wants to merge 4 commits into
base: release/candidate
Choose a base branch
from
Open

fix: reusing PagingData crash [WPB-15055][WPB-15079][WPB-15064] #3758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c7645c
fix: reusing PagingData crash [WPB-15055][WPB-15079][WPB-15064]
saleniuk Dec 18, 2024
455ee08
fix: add LoadStates to preview PagingData
saleniuk Dec 18, 2024
3351627
add tests for the PagingData flow
saleniuk Dec 18, 2024
94fccf7
fix detekt
saleniuk Dec 18, 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
22 changes: 16 additions & 6 deletions app/src/main/kotlin/com/wire/android/ui/common/topappbar/search/SearchBarState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ import androidx.compose.runtime.setValue

@Composable
fun rememberSearchbarState(
initialIsSearchActive: Boolean = false,
searchQueryTextState: TextFieldState = rememberTextFieldState()
): SearchBarState = rememberSaveable(
saver = SearchBarState.saver(searchQueryTextState)
saver = SearchBarState.saver()
) {
SearchBarState(searchQueryTextState = searchQueryTextState)
SearchBarState(isSearchActive = initialIsSearchActive, searchQueryTextState = searchQueryTextState)
}

class SearchBarState(
Expand All @@ -57,14 +58,23 @@ class SearchBarState(
}

companion object {
fun saver(searchQueryTextState: TextFieldState): Saver<SearchBarState, *> = Saver(
fun saver(): Saver<SearchBarState, *> = Saver(
save = {
listOf(it.isSearchActive)
listOf(
it.isSearchActive,
with(TextFieldState.Saver) {
save(it.searchQueryTextState)
}
)
},
restore = {
SearchBarState(
isSearchActive = it[0],
searchQueryTextState = searchQueryTextState
isSearchActive = it[0] as Boolean,
searchQueryTextState = it[1]?.let {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Are there chances of getting an IndexOutOfBounds here ? Asking just in case we can make it safer (for example using a Pair instead of a List?), otherwise it looks good to me.

with(TextFieldState.Saver) {
restore(it)
}
} ?: TextFieldState()
)
}
)
Expand Down
9 changes: 4 additions & 5 deletions app/src/main/kotlin/com/wire/android/ui/home/archive/ArchiveScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import com.wire.android.ui.home.conversationslist.common.previewConversationFold
import com.wire.android.ui.home.conversationslist.model.ConversationsSource
import com.wire.android.ui.theme.WireTheme
import com.wire.android.util.ui.PreviewMultipleThemes
import kotlinx.coroutines.flow.flowOf

@HomeNavGraph
@WireDestination
Expand All @@ -57,7 +56,7 @@ fun PreviewArchiveEmptyScreen() = WireTheme {
searchBarState = rememberSearchbarState(),
conversationsSource = ConversationsSource.ARCHIVE,
emptyListContent = { ArchiveEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(flowOf()),
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow(list = listOf())),
)
}

Expand All @@ -66,10 +65,10 @@ fun PreviewArchiveEmptyScreen() = WireTheme {
fun PreviewArchiveEmptySearchScreen() = WireTheme {
ConversationsScreenContent(
navigator = rememberNavigator {},
searchBarState = rememberSearchbarState(searchQueryTextState = TextFieldState(initialText = "er")),
searchBarState = rememberSearchbarState(initialIsSearchActive = true, searchQueryTextState = TextFieldState(initialText = "er")),
conversationsSource = ConversationsSource.ARCHIVE,
emptyListContent = { ArchiveEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(flowOf()),
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow(searchQuery = "er", list = listOf())),
)
}

Expand All @@ -78,7 +77,7 @@ fun PreviewArchiveEmptySearchScreen() = WireTheme {
fun PreviewArchiveScreen() = WireTheme {
ConversationsScreenContent(
navigator = rememberNavigator {},
searchBarState = rememberSearchbarState(searchQueryTextState = TextFieldState(initialText = "er")),
searchBarState = rememberSearchbarState(initialIsSearchActive = true, searchQueryTextState = TextFieldState(initialText = "er")),
conversationsSource = ConversationsSource.ARCHIVE,
emptyListContent = { ArchiveEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow(searchQuery = "er")),
Expand Down
167 changes: 91 additions & 76 deletions app/src/main/kotlin/com/wire/android/ui/home/conversationslist/ConversationListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import androidx.compose.runtime.setValue
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.paging.PagingData
import androidx.paging.cachedIn
import androidx.paging.insertSeparators
import androidx.paging.map
import com.wire.android.BuildConfig
Expand Down Expand Up @@ -126,7 +127,7 @@ class ConversationListViewModelPreview(
class ConversationListViewModelImpl @AssistedInject constructor(
@Assisted val conversationsSource: ConversationsSource,
@Assisted private val usePagination: Boolean = BuildConfig.PAGINATED_CONVERSATION_LIST_ENABLED,
dispatcher: DispatcherProvider,
private val dispatcher: DispatcherProvider,
private val updateConversationMutedStatus: UpdateConversationMutedStatusUseCase,
private val getConversationsPaginated: GetConversationsFromSearchUseCase,
private val observeConversationListDetailsWithEvents: ObserveConversationListDetailsWithEventsUseCase,
Expand Down Expand Up @@ -161,6 +162,7 @@ class ConversationListViewModelImpl @AssistedInject constructor(
override val closeBottomSheet = MutableSharedFlow<Unit>()

private val searchQueryFlow: MutableStateFlow<String> = MutableStateFlow("")
private val isSelfUserUnderLegalHoldFlow = MutableSharedFlow<Boolean>(replay = 1)

private val containsNewActivitiesSection = when (conversationsSource) {
ConversationsSource.MAIN,
Expand All @@ -174,94 +176,105 @@ class ConversationListViewModelImpl @AssistedInject constructor(
private val conversationsPaginatedFlow: Flow<PagingData<ConversationFolderItem>> = searchQueryFlow
.debounce { if (it.isEmpty()) 0L else DEFAULT_SEARCH_QUERY_DEBOUNCE }
.onStart { emit("") }
.combine(isSelfUserUnderLegalHoldFlow, ::Pair)
.distinctUntilChanged()
.flatMapLatest { searchQuery ->
.flatMapLatest { (searchQuery, isSelfUserUnderLegalHold) ->
getConversationsPaginated(
searchQuery = searchQuery,
fromArchive = conversationsSource == ConversationsSource.ARCHIVE,
conversationFilter = conversationsSource.toFilter(),
onlyInteractionEnabled = false,
newActivitiesOnTop = containsNewActivitiesSection,
).combine(observeLegalHoldStateForSelfUser()) { conversations, selfUserLegalHoldStatus ->
conversations.map {
it.hideIndicatorForSelfUserUnderLegalHold(selfUserLegalHoldStatus)
}
}.map {
it.insertSeparators { before, after ->
when {
// do not add separators if the list shouldn't show conversations grouped into different folders
!containsNewActivitiesSection -> null

before == null && after != null && after.hasNewActivitiesToShow ->
// list starts with items with "new activities"
ConversationFolder.Predefined.NewActivities

before == null && after != null && !after.hasNewActivitiesToShow ->
// list doesn't contain any items with "new activities"
ConversationFolder.Predefined.Conversations

before != null && before.hasNewActivitiesToShow && after != null && !after.hasNewActivitiesToShow ->
// end of "new activities" section and beginning of "conversations" section
ConversationFolder.Predefined.Conversations

else -> null
).map { pagingData ->
pagingData
.map { it.hideIndicatorForSelfUserUnderLegalHold(isSelfUserUnderLegalHold) }
.insertSeparators { before, after ->
when {
// do not add separators if the list shouldn't show conversations grouped into different folders
!containsNewActivitiesSection -> null

before == null && after != null && after.hasNewActivitiesToShow ->
// list starts with items with "new activities"
ConversationFolder.Predefined.NewActivities

before == null && after != null && !after.hasNewActivitiesToShow ->
// list doesn't contain any items with "new activities"
ConversationFolder.Predefined.Conversations

before != null && before.hasNewActivitiesToShow && after != null && !after.hasNewActivitiesToShow ->
// end of "new activities" section and beginning of "conversations" section
ConversationFolder.Predefined.Conversations

else -> null
}
}
}
}
}
.flowOn(dispatcher.io())
.cachedIn(viewModelScope)

private var notPaginatedConversationListState by mutableStateOf(ConversationListState.NotPaginated())
override val conversationListState: ConversationListState
get() = if (usePagination) {
ConversationListState.Paginated(
conversations = conversationsPaginatedFlow,
domain = currentAccount.domain
)
} else {
notPaginatedConversationListState
override var conversationListState by mutableStateOf(
when (usePagination) {
true -> ConversationListState.Paginated(conversations = conversationsPaginatedFlow, domain = currentAccount.domain)
false -> ConversationListState.NotPaginated()
}
)
private set

init {
observeSelfUserLegalHoldState()
if (!usePagination) {
viewModelScope.launch {
searchQueryFlow
.debounce { if (it.isEmpty()) 0L else DEFAULT_SEARCH_QUERY_DEBOUNCE }
.onStart { emit("") }
.distinctUntilChanged()
.flatMapLatest { searchQuery: String ->
observeConversationListDetailsWithEvents(
fromArchive = conversationsSource == ConversationsSource.ARCHIVE,
conversationFilter = conversationsSource.toFilter()
).combine(observeLegalHoldStateForSelfUser()) { conversations, selfUserLegalHoldStatus ->
conversations.map { conversationDetails ->
conversationDetails.toConversationItem(
userTypeMapper = userTypeMapper,
searchQuery = searchQuery,
selfUserTeamId = observeSelfUser().firstOrNull()?.teamId
).hideIndicatorForSelfUserUnderLegalHold(selfUserLegalHoldStatus)
} to searchQuery
}
}
.map { (conversationItems, searchQuery) ->
if (searchQuery.isEmpty()) {
conversationItems.withFolders(source = conversationsSource).toImmutableMap()
} else {
searchConversation(
conversationDetails = conversationItems,
searchQuery = searchQuery
).withFolders(source = conversationsSource).toImmutableMap()
}
observeNonPaginatedSearchConversationList()
}
}

private fun observeSelfUserLegalHoldState() {
viewModelScope.launch {
observeLegalHoldStateForSelfUser()
.map { it is LegalHoldStateForSelfUser.Enabled }
.flowOn(dispatcher.io())
.collect { isSelfUserUnderLegalHoldFlow.emit(it) }
}
}
Comment on lines +231 to +238
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a separate fun with collect you can have:

val isSelfUserUnderLegalHoldFlow = observeLegalHoldStateForSelfUser()
    .map { it is LegalHoldStateForSelfUser.Enabled }
    .flowOn(dispatcher.io())
    .shareIn(viewModelScope, SharingStarted.WhileSubscribed(), 1)


private fun observeNonPaginatedSearchConversationList() {
viewModelScope.launch {
searchQueryFlow
.debounce { if (it.isEmpty()) 0L else DEFAULT_SEARCH_QUERY_DEBOUNCE }
.onStart { emit("") }
.distinctUntilChanged()
.flatMapLatest { searchQuery: String ->
observeConversationListDetailsWithEvents(
fromArchive = conversationsSource == ConversationsSource.ARCHIVE,
conversationFilter = conversationsSource.toFilter()
).combine(isSelfUserUnderLegalHoldFlow) { conversations, isSelfUserUnderLegalHold ->
conversations.map { conversationDetails ->
conversationDetails.toConversationItem(
userTypeMapper = userTypeMapper,
searchQuery = searchQuery,
selfUserTeamId = observeSelfUser().firstOrNull()?.teamId
).hideIndicatorForSelfUserUnderLegalHold(isSelfUserUnderLegalHold)
} to searchQuery
}
.flowOn(dispatcher.io())
.collect {
notPaginatedConversationListState = notPaginatedConversationListState.copy(
isLoading = false,
conversations = it,
domain = currentAccount.domain
)
}
.map { (conversationItems, searchQuery) ->
if (searchQuery.isEmpty()) {
conversationItems.withFolders(source = conversationsSource).toImmutableMap()
} else {
searchConversation(
conversationDetails = conversationItems,
searchQuery = searchQuery
).withFolders(source = conversationsSource).toImmutableMap()
}
}
}
.flowOn(dispatcher.io())
.collect {
conversationListState = ConversationListState.NotPaginated(
isLoading = false,
conversations = it,
domain = currentAccount.domain
)
}
}
}

Expand Down Expand Up @@ -446,11 +459,13 @@ private fun ConversationsSource.toFilter(): ConversationFilter = when (this) {
ConversationsSource.ONE_ON_ONE -> ConversationFilter.ONE_ON_ONE
}

private fun ConversationItem.hideIndicatorForSelfUserUnderLegalHold(selfUserLegalHoldStatus: LegalHoldStateForSelfUser) =
// if self user is under legal hold then we shouldn't show legal hold indicator next to every conversation
// the indication is shown in the header of the conversation list for self user in that case and it's enough
when (selfUserLegalHoldStatus) {
is LegalHoldStateForSelfUser.Enabled -> when (this) {
/**
* If self user is under legal hold then we shouldn't show legal hold indicator next to every conversation as in that case
* the legal hold indication is shown in the header of the conversation list for self user in that case and it's enough.
*/
private fun ConversationItem.hideIndicatorForSelfUserUnderLegalHold(isSelfUserUnderLegalHold: Boolean) =
when (isSelfUserUnderLegalHold) {
true -> when (this) {
is ConversationItem.ConnectionConversation -> this.copy(showLegalHoldIndicator = false)
is ConversationItem.GroupConversation -> this.copy(showLegalHoldIndicator = false)
is ConversationItem.PrivateConversation -> this.copy(showLegalHoldIndicator = false)
Expand Down
6 changes: 1 addition & 5 deletions app/src/main/kotlin/com/wire/android/ui/home/conversationslist/ConversationsScreenContent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ fun ConversationsScreenContent(
lazyListState: LazyListState = rememberLazyListState(),
loadingListContent: @Composable (LazyListState) -> Unit = { ConversationListLoadingContent(it) },
conversationsSource: ConversationsSource = ConversationsSource.MAIN,
initiallyLoaded: Boolean = LocalInspectionMode.current,
conversationListViewModel: ConversationListViewModel = when {
LocalInspectionMode.current -> ConversationListViewModelPreview()
else -> hiltViewModel<ConversationListViewModelImpl, ConversationListViewModelImpl.Factory>(
Expand Down Expand Up @@ -189,10 +188,7 @@ fun ConversationsScreenContent(
when (val state = conversationListViewModel.conversationListState) {
is ConversationListState.Paginated -> {
val lazyPagingItems = state.conversations.collectAsLazyPagingItems()
var showLoading by remember { mutableStateOf(!initiallyLoaded) }
if (lazyPagingItems.loadState.refresh != LoadState.Loading && showLoading) {
showLoading = false
}
val showLoading = lazyPagingItems.loadState.refresh == LoadState.Loading && lazyPagingItems.itemCount == 0

when {
// when conversation list is not yet fetched, show loading indicator
Expand Down
9 changes: 4 additions & 5 deletions app/src/main/kotlin/com/wire/android/ui/home/conversationslist/all/AllConversationsScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import com.wire.android.ui.home.conversationslist.model.ConversationsSource
import com.wire.android.ui.theme.WireTheme
import com.wire.android.util.ui.PreviewMultipleThemes
import com.wire.kalium.logic.data.conversation.ConversationFilter
import kotlinx.coroutines.flow.flowOf

@HomeNavGraph(start = true)
@WireDestination
Expand Down Expand Up @@ -103,7 +102,7 @@ fun PreviewAllConversationsEmptyScreen() = WireTheme {
searchBarState = rememberSearchbarState(),
conversationsSource = ConversationsSource.MAIN,
emptyListContent = { ConversationsEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(flowOf()),
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow(list = listOf())),
)
}

Expand All @@ -112,10 +111,10 @@ fun PreviewAllConversationsEmptyScreen() = WireTheme {
fun PreviewAllConversationsEmptySearchScreen() = WireTheme {
ConversationsScreenContent(
navigator = rememberNavigator {},
searchBarState = rememberSearchbarState(searchQueryTextState = TextFieldState(initialText = "er")),
searchBarState = rememberSearchbarState(initialIsSearchActive = true, searchQueryTextState = TextFieldState(initialText = "er")),
conversationsSource = ConversationsSource.MAIN,
emptyListContent = { ConversationsEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(flowOf()),
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow(searchQuery = "er", list = listOf())),
)
}

Expand All @@ -124,7 +123,7 @@ fun PreviewAllConversationsEmptySearchScreen() = WireTheme {
fun PreviewAllConversationsSearchScreen() = WireTheme {
ConversationsScreenContent(
navigator = rememberNavigator {},
searchBarState = rememberSearchbarState(searchQueryTextState = TextFieldState(initialText = "er")),
searchBarState = rememberSearchbarState(initialIsSearchActive = true, searchQueryTextState = TextFieldState(initialText = "er")),
conversationsSource = ConversationsSource.MAIN,
emptyListContent = { ConversationsEmptyContent() },
conversationListViewModel = ConversationListViewModelPreview(previewConversationFoldersFlow("er")),
Expand Down
Loading
Loading