From 2f2b32cc74a7c2e9543d0cad11d0d88930c3c980 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 2 Feb 2024 16:15:34 -0300 Subject: [PATCH 1/5] Fix using null topBarUiState and add wait for a non-null value The added code tries to wait until the topBarUiState is not null by polling for its data for a few times. If it's still null it simply doesn't run the code, to avoid calling topbar updates that should only happen if there is already a top bar visible. If it's not null after that check (normal scenario) then it calls the block and executes the logic. --- .../ui/reader/viewmodels/ReaderViewModel.kt | 85 +++++++++++-------- 1 file changed, 50 insertions(+), 35 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt index 4b6d3b5ce45f..2b96169eb402 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt @@ -4,11 +4,10 @@ import androidx.annotation.DrawableRes import androidx.annotation.StringRes import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData -import androidx.lifecycle.viewModelScope import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay -import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe @@ -52,6 +51,7 @@ import org.wordpress.android.viewmodel.Event import org.wordpress.android.viewmodel.ScopedViewModel import javax.inject.Inject import javax.inject.Named +import kotlin.coroutines.CoroutineContext const val UPDATE_TAGS_THRESHOLD = 1000 * 60 * 60 // 1 hr const val TRACK_TAB_CHANGED_THROTTLE = 100L @@ -428,55 +428,70 @@ class ReaderViewModel @Inject constructor( } } - private fun clearTopBarFilter() { - val filterUiState = _topBarUiState.value?.filterUiState - ?.copy(selectedItem = null) + private fun tryWaitNonNullTopBarUiStateThenRun( + initialDelay: Long = 0L, + retryTime: Long = 50L, + maxRetries: Int = 10, + runContext: CoroutineContext = mainDispatcher, + block: suspend CoroutineScope.(topBarUiState: TopBarUiState) -> Unit + ) { + launch(bgDispatcher) { + if (initialDelay > 0L) delay(initialDelay) - viewModelScope.launch(mainDispatcher) { - delay(FILTER_UPDATE_DELAY) // small delay to achieve a fluid animation since other UI updates are happening - _topBarUiState.postValue( - _topBarUiState.value - ?.copy(filterUiState = filterUiState) - ) + var remainingTries = maxRetries + while (_topBarUiState.value == null && remainingTries > 0) { + delay(retryTime) + remainingTries-- + } + + // only run the block if the topBarUiState is not null, otherwise do nothing + _topBarUiState.value?.let { topBarUiState -> + withContext(runContext) { + block(topBarUiState) + } + } } } - private fun updateTopBarFilter(itemName: String, type: ReaderFilterType) { - val filterUiState = _topBarUiState.value?.filterUiState - ?.copy(selectedItem = ReaderFilterSelectedItem(UiStringText(itemName), type)) + private fun clearTopBarFilter() { + // small delay to achieve a fluid animation since other UI updates are happening + tryWaitNonNullTopBarUiStateThenRun(initialDelay = FILTER_UPDATE_DELAY) { topBarUiState -> + val filterUiState = topBarUiState.filterUiState?.copy(selectedItem = null) + _topBarUiState.postValue(topBarUiState.copy(filterUiState = filterUiState)) + } + } - viewModelScope.launch(mainDispatcher) { - delay(FILTER_UPDATE_DELAY) // small delay to achieve a fluid animation since other UI updates are happening - _topBarUiState.postValue( - _topBarUiState.value - ?.copy(filterUiState = filterUiState) - ) + private fun updateTopBarFilter(itemName: String, type: ReaderFilterType) { + // small delay to achieve a fluid animation since other UI updates are happening + tryWaitNonNullTopBarUiStateThenRun(initialDelay = FILTER_UPDATE_DELAY) { topBarUiState -> + val filterUiState = topBarUiState.filterUiState + ?.copy(selectedItem = ReaderFilterSelectedItem(UiStringText(itemName), type)) + _topBarUiState.postValue(topBarUiState.copy(filterUiState = filterUiState)) } } - fun hideTopBarFilterGroup(readerTab: ReaderTag) { - val selectedReaderTag = _topBarUiState.value?.selectedItem?.let { - readerTagsList[readerTopBarMenuHelper.getReaderTagIndexFromMenuItem(it)] - } ?: return + fun hideTopBarFilterGroup(readerTab: ReaderTag) = tryWaitNonNullTopBarUiStateThenRun { topBarUiState -> + val readerTagIndex = readerTopBarMenuHelper.getReaderTagIndexFromMenuItem(topBarUiState.selectedItem) + val selectedReaderTag = readerTagsList[readerTagIndex] - if (readerTab != selectedReaderTag) return + if (readerTab != selectedReaderTag) return@tryWaitNonNullTopBarUiStateThenRun - _topBarUiState.postValue( - topBarUiState.value?.copy(filterUiState = null) - ) + _topBarUiState.postValue(topBarUiState.copy(filterUiState = null)) } - fun showTopBarFilterGroup(readerTab: ReaderTag, subFilterItems: List) { - val selectedReaderTag = _topBarUiState.value?.selectedItem?.let { - readerTagsList[readerTopBarMenuHelper.getReaderTagIndexFromMenuItem(it)] - } ?: return + fun showTopBarFilterGroup( + readerTab: ReaderTag, + subFilterItems: List + ) = tryWaitNonNullTopBarUiStateThenRun { topBarUiState -> + val readerTagIndex = readerTopBarMenuHelper.getReaderTagIndexFromMenuItem(topBarUiState.selectedItem) + val selectedReaderTag = readerTagsList[readerTagIndex] - if (readerTab != selectedReaderTag) return + if (readerTab != selectedReaderTag) return@tryWaitNonNullTopBarUiStateThenRun val blogsFilterCount = subFilterItems.filterIsInstance().size val tagsFilterCount = subFilterItems.filterIsInstance().size - val filterState = _topBarUiState.value?.filterUiState + val filterState = topBarUiState.filterUiState ?.copy( blogsFilterCount = blogsFilterCount, tagsFilterCount = tagsFilterCount, @@ -491,7 +506,7 @@ class ReaderViewModel @Inject constructor( ) _topBarUiState.postValue( - topBarUiState.value?.copy(filterUiState = filterState) + topBarUiState.copy(filterUiState = filterState) ) } From 09f08bd333d355e8424d189a66a476d50f25ac8a Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 2 Feb 2024 17:17:59 -0300 Subject: [PATCH 2/5] Make UiString parcelable --- .../main/java/org/wordpress/android/ui/utils/UiString.kt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/utils/UiString.kt b/WordPress/src/main/java/org/wordpress/android/ui/utils/UiString.kt index bdddae84344a..b3260b617a31 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/utils/UiString.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/utils/UiString.kt @@ -1,20 +1,26 @@ package org.wordpress.android.ui.utils +import android.os.Parcelable import androidx.annotation.StringRes +import kotlinx.parcelize.Parcelize /** * [UiString] is a utility sealed class that represents a string to be used in the UI. It allows a string to be * represented as both string resource and text. */ -sealed class UiString { +sealed class UiString : Parcelable { + @Parcelize data class UiStringText(val text: CharSequence) : UiString() + @Parcelize data class UiStringRes(@StringRes val stringRes: Int) : UiString() + @Parcelize data class UiStringResWithParams(@StringRes val stringRes: Int, val params: List) : UiString() { constructor(@StringRes stringRes: Int, vararg varargParams: UiString) : this(stringRes, varargParams.toList()) } // Current localization process does not support resource strings, // so we need to use multiple string resources. Switch to @PluralsRes when it is supported by localization process. + @Parcelize data class UiStringPluralRes( @StringRes val zeroRes: Int, @StringRes val oneRes: Int, From e24367b3838bc824279f2e8dfb99e5cc15490357 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 2 Feb 2024 17:18:15 -0300 Subject: [PATCH 3/5] Make ReaderFilterSelectedItem parcelable --- .../ui/reader/views/compose/filter/ReaderFilterChipGroup.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/filter/ReaderFilterChipGroup.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/filter/ReaderFilterChipGroup.kt index b5f1d47db68d..8e810354e404 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/filter/ReaderFilterChipGroup.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/filter/ReaderFilterChipGroup.kt @@ -1,6 +1,7 @@ package org.wordpress.android.ui.reader.views.compose.filter import android.content.res.Configuration.UI_MODE_NIGHT_YES +import android.os.Parcelable import androidx.compose.animation.AnimatedVisibility import androidx.compose.animation.animateColorAsState import androidx.compose.animation.animateContentSize @@ -37,6 +38,7 @@ import androidx.compose.ui.semantics.Role import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.dp +import kotlinx.parcelize.Parcelize import org.wordpress.android.R import org.wordpress.android.ui.compose.theme.AppThemeWithoutBackground import org.wordpress.android.ui.compose.unit.Margin @@ -210,10 +212,11 @@ enum class ReaderFilterType { TAG, } +@Parcelize data class ReaderFilterSelectedItem( val text: UiString, val type: ReaderFilterType, -) +) : Parcelable @Preview(name = "Light Mode", showBackground = true) @Preview(name = "Dark Mode", showBackground = true, uiMode = UI_MODE_NIGHT_YES) From 0b0fa9e26073c64751a0673a4f9ab32d99a04703 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 2 Feb 2024 17:18:59 -0300 Subject: [PATCH 4/5] Save and retrieve top bar state in SavedInstanceState bundle Save information about the selected feed id and top bar filter ui state in the SavedInstanceState Bundle and recover it when the activity is destroyed due to system-initiated destruction (low-memory or "Don't keep activities" dev setting) to keep the experience consistent. --- .../android/ui/reader/ReaderFragment.kt | 7 ++- .../ui/reader/viewmodels/ReaderViewModel.kt | 47 +++++++++++++++---- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt index f11743b16f17..49bc8607a84c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt @@ -210,7 +210,12 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), MenuProvider, observeJetpackOverlayEvent(savedInstanceState) - viewModel.start() + viewModel.start(savedInstanceState) + } + + override fun onSaveInstanceState(outState: Bundle) { + super.onSaveInstanceState(outState) + viewModel.onSaveInstanceState(outState) } private fun updateUiState(uiState: ReaderViewModel.ReaderUiState) { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt index 2b96169eb402..9f6512c17a93 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt @@ -1,7 +1,10 @@ package org.wordpress.android.ui.reader.viewmodels +import android.os.Bundle +import android.os.Parcelable import androidx.annotation.DrawableRes import androidx.annotation.StringRes +import androidx.core.os.BundleCompat import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.CoroutineDispatcher @@ -9,6 +12,7 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.withContext +import kotlinx.parcelize.Parcelize import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode.MAIN @@ -114,24 +118,31 @@ class ReaderViewModel @Inject constructor( EventBus.getDefault().register(this) } - fun start() { + fun start(savedInstanceState: Bundle?) { if (tagsRequireUpdate()) _updateTags.value = Event(Unit) if (initialized) return - loadTabs() + loadTabs(savedInstanceState) if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()) showJetpackPoweredBottomSheet() } + fun onSaveInstanceState(out: Bundle) { + _topBarUiState.value?.let { + out.putString(KEY_TOP_BAR_UI_STATE_SELECTED_ITEM_ID, it.selectedItem.id) + out.putParcelable(KEY_TOP_BAR_UI_STATE_FILTER_UI_STATE, it.filterUiState) + } + } + private fun showJetpackPoweredBottomSheet() { // _showJetpackPoweredBottomSheet.value = Event(true) } - private fun loadTabs() { + private fun loadTabs(savedInstanceState: Bundle? = null) { launch { val currentContentUiState = _uiState.value as? ContentUiState val tagList = loadReaderTabsUseCase.loadTabs() if (tagList.isNotEmpty() && readerTagsList != tagList) { updateReaderTagsList(tagList) - updateTopBarUiState() + updateTopBarUiState(savedInstanceState) _uiState.value = ContentUiState( tabUiStates = tagList.map { TabUiState(label = UiStringText(it.label)) }, selectedReaderTag = selectedReaderTag(), @@ -352,20 +363,34 @@ class ReaderViewModel @Inject constructor( readerTagsList[readerTopBarMenuHelper.getReaderTagIndexFromMenuItem(it.selectedItem)] } - private suspend fun updateTopBarUiState() { + private suspend fun updateTopBarUiState(savedInstanceState: Bundle? = null) { withContext(bgDispatcher) { val menuItems = readerTopBarMenuHelper.createMenu(readerTagsList) // if menu is exactly the same as before, don't update if (_topBarUiState.value?.menuItems == menuItems) return@withContext - // if there's already a selected item, use it, otherwise use the first item + + // if there's already a selected item, use it, otherwise use the first item, also try to use the saved state + val savedStateSelectedId = savedInstanceState?.getString(KEY_TOP_BAR_UI_STATE_SELECTED_ITEM_ID) val selectedItem = _topBarUiState.value?.selectedItem - ?: menuItems.first { it is MenuElementData.Item.Single } as MenuElementData.Item.Single + ?: menuItems.filterSingleItems() + .let { singleItems -> + singleItems.firstOrNull { it.id == savedStateSelectedId } ?: singleItems.first() + } - // if there's a selected item and filter state, also use the filter state + // if there's a selected item and filter state, also use the filter state, also try to use the saved state val filterUiState = _topBarUiState.value?.filterUiState ?.takeIf { _topBarUiState.value?.selectedItem != null } + ?: savedInstanceState + ?.let { + BundleCompat.getParcelable( + it, + KEY_TOP_BAR_UI_STATE_FILTER_UI_STATE, + TopBarUiState.FilterUiState::class.java + ) + } + ?.takeIf { selectedItem.id == savedStateSelectedId } _topBarUiState.postValue( TopBarUiState( @@ -525,13 +550,14 @@ class ReaderViewModel @Inject constructor( val onDropdownMenuClick: () -> Unit, val isSearchActionVisible: Boolean = false, ) { + @Parcelize data class FilterUiState( val blogsFilterCount: Int, val tagsFilterCount: Int, val selectedItem: ReaderFilterSelectedItem? = null, val showBlogsFilter: Boolean = blogsFilterCount > 0, val showTagsFilter: Boolean = tagsFilterCount > 0, - ) + ) : Parcelable } sealed class ReaderUiState( @@ -573,6 +599,9 @@ class ReaderViewModel @Inject constructor( private const val QUICK_START_DISCOVER_TAB_STEP_DELAY = 2000L private const val QUICK_START_PROMPT_DURATION = 5000 private const val FILTER_UPDATE_DELAY = 50L + + private const val KEY_TOP_BAR_UI_STATE_SELECTED_ITEM_ID = "topBarUiState_selectedItem_id" + private const val KEY_TOP_BAR_UI_STATE_FILTER_UI_STATE = "topBarUiState_filterUiState" } } From 777633fae97a9299f1120c531aaf776559d0e941 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 2 Feb 2024 18:36:32 -0300 Subject: [PATCH 5/5] Add default value in start method Avoid breaking existing unit tests. --- .../wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt index 9f6512c17a93..2f97cf4fcbea 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt @@ -118,7 +118,7 @@ class ReaderViewModel @Inject constructor( EventBus.getDefault().register(this) } - fun start(savedInstanceState: Bundle?) { + fun start(savedInstanceState: Bundle? = null) { if (tagsRequireUpdate()) _updateTags.value = Event(Unit) if (initialized) return loadTabs(savedInstanceState)