From 4855ec7153e7eca9299a0a991c84bef2203c9dc5 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 17 May 2024 17:48:59 -0300 Subject: [PATCH 1/7] Add mechanism to initialize observer only once Also making sure it works for Fragment recreation by the system (such as screen rotation) and ViewModel destruction due to owner being destroyed. --- .../android/ui/reader/ReaderFragment.kt | 27 +++++++++++++++++-- .../ui/reader/subfilter/SubFilterViewModel.kt | 11 +++++--- 2 files changed, 32 insertions(+), 6 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 474ef518d722..70267519379e 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 @@ -4,6 +4,7 @@ import android.app.Activity import android.content.Context import android.content.Intent import android.os.Bundle +import android.util.Log import android.view.View import androidx.activity.result.ActivityResult import androidx.activity.result.ActivityResultLauncher @@ -96,6 +97,14 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView private var readerSubsActivityResultLauncher: ActivityResultLauncher? = null + /** + * Stores the keys for [SubFilterViewModel] that have already been initialized (observers) and started. + * It is only cleared when: + * - the [ReaderFragment] is destroyed, to allow proper initialization in cases like screen rotation + * - the [SubFilterViewModel] is cleared, to allow proper initialization when the ViewModel is created again + */ + private val initializedSubFilterViewModelKeys: MutableList = mutableListOf() + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { binding = ReaderFragmentLayoutBinding.bind(view).apply { initTopAppBar() @@ -444,11 +453,21 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView } override fun getSubFilterViewModelForTag(tag: ReaderTag, savedInstanceState: Bundle?): SubFilterViewModel { + val keyForTag = SubFilterViewModel.getViewModelKeyForTag(tag) return ViewModelProvider(getSubFilterViewModelOwner(), viewModelFactory)[ - SubFilterViewModel.getViewModelKeyForTag(tag), + keyForTag, SubFilterViewModel::class.java ].also { - it.initSubFilterViewModel(tag, savedInstanceState) + // only initialize if it hasn't been initialized yet + if (keyForTag !in initializedSubFilterViewModelKeys) { + it.initSubFilterViewModel(tag, savedInstanceState) + + // setup mechanism for proper one-time initialization and clearing + initializedSubFilterViewModelKeys.add(keyForTag) + it.onCleared.observeEvent(viewLifecycleOwner) { + initializedSubFilterViewModelKeys.remove(keyForTag) + } + } } } @@ -458,6 +477,7 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView currentSubFilter.observe( viewLifecycleOwner ) { subfilterListItem: SubfilterListItem -> + Log.d("ReaderFragment", "currentSubFilter.observe") onSubfilterSelected(subfilterListItem) viewModel.onSubFilterItemSelected(subfilterListItem) } @@ -466,6 +486,7 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView updateTagsAndSites.observe( viewLifecycleOwner ) { event: Event> -> + Log.d("ReaderFragment", "updateTagsAndSites.observe") event.applyIfNotHandled { if (NetworkUtils.isNetworkAvailable(activity)) { ReaderUpdateServiceStarter.startService(activity, this) @@ -503,6 +524,7 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView bottomSheetUiState.observe( viewLifecycleOwner ) { event: Event -> + Log.d("ReaderFragment", "bottomSheetUiState.observe") event.applyIfNotHandled { val fm = childFragmentManager var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? @@ -524,6 +546,7 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView bottomSheetAction.observe( viewLifecycleOwner ) { event: Event -> + Log.d("ReaderFragment", "bottomSheetAction.observe") event.applyIfNotHandled { when (this) { is OpenSubsAtPage -> { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index eefb58a67f82..d35c18d86cb8 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -38,7 +38,6 @@ import org.wordpress.android.util.UrlUtils import org.wordpress.android.viewmodel.Event import org.wordpress.android.viewmodel.ScopedViewModel import org.wordpress.android.viewmodel.SingleLiveEvent -import java.util.Comparator import java.util.EnumSet import javax.inject.Inject import javax.inject.Named @@ -70,6 +69,9 @@ class SubFilterViewModel @Inject constructor( private val _updateTagsAndSites = MutableLiveData>>() val updateTagsAndSites: LiveData>> = _updateTagsAndSites + private val _onCleared = MutableLiveData>() + val onCleared: LiveData> = _onCleared + private val _isTitleContainerVisible = MutableLiveData(true) val isTitleContainerVisible: LiveData = _isTitleContainerVisible @@ -139,12 +141,12 @@ class SubFilterViewModel @Inject constructor( blog.organizationId == organization.orgId } ?: false } - }.sortedWith(Comparator { blog1, blog2 -> + }.sortedWith { blog1, blog2 -> // sort followed blogs by name/domain to match display val blogOneName = getBlogNameForComparison(blog1) val blogTwoName = getBlogNameForComparison(blog2) blogOneName.compareTo(blogTwoName, true) - }) + } filterList.addAll( followedBlogs.map { blog -> @@ -414,7 +416,7 @@ class SubFilterViewModel @Inject constructor( loadSubFilters() } - @Suppress("unused", "UNUSED_PARAMETER") + @Suppress("unused") @Subscribe(threadMode = ThreadMode.MAIN) fun onEventMainThread(event: ReaderEvents.FollowedBlogsFetched) { if(event.didChange()) { @@ -426,6 +428,7 @@ class SubFilterViewModel @Inject constructor( override fun onCleared() { super.onCleared() eventBusWrapper.unregister(this) + _onCleared.value = Event(Unit) } companion object { From da1d249573ec12c8fc7dea61cddce6a06aba612e Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 17 May 2024 18:20:28 -0300 Subject: [PATCH 2/7] Fix item selected tracked when first opening Subscriptions/Tags feeds --- .../ui/reader/subfilter/SubFilterViewModel.kt | 6 +++--- .../subfilter/SubFilterViewModelTest.kt | 19 +++++++++++-------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index d35c18d86cb8..a586c810e68f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -325,8 +325,8 @@ class SubFilterViewModel @Inject constructor( } fun onSubfilterSelected(subfilterListItem: SubfilterListItem) { - // We should not track subfilter selected if we're clearing a filter that is currently applied. - if (!subfilterListItem.isClearingFilter) { + // We should only track the selection of a subfilter if it's a tracked item (meaning it's a valid tag or site) + if (subfilterListItem.isTrackedItem) { val filterItemType = FilterItemType.fromSubfilterListItem(subfilterListItem) if (filterItemType != null) { readerTracker.track( @@ -334,7 +334,7 @@ class SubFilterViewModel @Inject constructor( mutableMapOf(FilterItemType.trackingEntry(filterItemType)) ) } else { - readerTracker.track(Stat.READER_FILTER_SHEET_ITEM_SELECTED,) + readerTracker.track(Stat.READER_FILTER_SHEET_ITEM_SELECTED) } } changeSubfilter(subfilterListItem, true, mTagFragmentStartedWith) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt index bad452b02fd0..7c8f2807e5f6 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt @@ -430,9 +430,8 @@ class SubFilterViewModelTest : BaseUnitTest() { } @Test - fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if clearing filter when onSubfilterSelected is called`() { + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if SiteAll when onSubfilterSelected is called`() { val filter = SiteAll( - isClearingFilter = true, onClickAction = {}, ) viewModel.onSubfilterSelected(filter) @@ -440,13 +439,17 @@ class SubFilterViewModelTest : BaseUnitTest() { } @Test - fun `Should track READER_FILTER_SHEET_ITEM_SELECTED if NOT clearing filter when onSubfilterSelected is called`() { - val filter = SiteAll( - isClearingFilter = false, - onClickAction = {}, - ) + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if Divider when onSubfilterSelected is called`() { + val filter = SubfilterListItem.Divider viewModel.onSubfilterSelected(filter) - verify(readerTracker).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) + verify(readerTracker, times(0)).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) + } + + @Test + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if SectionTitle when onSubfilterSelected is called`() { + val filter = SubfilterListItem.SectionTitle(UiStringText("test")) + viewModel.onSubfilterSelected(filter) + verify(readerTracker, times(0)).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) } @Test From 590230f154c84a7d5a558faf82e4887777e35adb Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 20 May 2024 08:39:43 -0300 Subject: [PATCH 3/7] Revert "Add mechanism to initialize observer only once" This reverts commit 4855ec7153e7eca9299a0a991c84bef2203c9dc5. --- .../android/ui/reader/ReaderFragment.kt | 27 ++----------------- .../ui/reader/subfilter/SubFilterViewModel.kt | 11 +++----- 2 files changed, 6 insertions(+), 32 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 70267519379e..474ef518d722 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 @@ -4,7 +4,6 @@ import android.app.Activity import android.content.Context import android.content.Intent import android.os.Bundle -import android.util.Log import android.view.View import androidx.activity.result.ActivityResult import androidx.activity.result.ActivityResultLauncher @@ -97,14 +96,6 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView private var readerSubsActivityResultLauncher: ActivityResultLauncher? = null - /** - * Stores the keys for [SubFilterViewModel] that have already been initialized (observers) and started. - * It is only cleared when: - * - the [ReaderFragment] is destroyed, to allow proper initialization in cases like screen rotation - * - the [SubFilterViewModel] is cleared, to allow proper initialization when the ViewModel is created again - */ - private val initializedSubFilterViewModelKeys: MutableList = mutableListOf() - override fun onViewCreated(view: View, savedInstanceState: Bundle?) { binding = ReaderFragmentLayoutBinding.bind(view).apply { initTopAppBar() @@ -453,21 +444,11 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView } override fun getSubFilterViewModelForTag(tag: ReaderTag, savedInstanceState: Bundle?): SubFilterViewModel { - val keyForTag = SubFilterViewModel.getViewModelKeyForTag(tag) return ViewModelProvider(getSubFilterViewModelOwner(), viewModelFactory)[ - keyForTag, + SubFilterViewModel.getViewModelKeyForTag(tag), SubFilterViewModel::class.java ].also { - // only initialize if it hasn't been initialized yet - if (keyForTag !in initializedSubFilterViewModelKeys) { - it.initSubFilterViewModel(tag, savedInstanceState) - - // setup mechanism for proper one-time initialization and clearing - initializedSubFilterViewModelKeys.add(keyForTag) - it.onCleared.observeEvent(viewLifecycleOwner) { - initializedSubFilterViewModelKeys.remove(keyForTag) - } - } + it.initSubFilterViewModel(tag, savedInstanceState) } } @@ -477,7 +458,6 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView currentSubFilter.observe( viewLifecycleOwner ) { subfilterListItem: SubfilterListItem -> - Log.d("ReaderFragment", "currentSubFilter.observe") onSubfilterSelected(subfilterListItem) viewModel.onSubFilterItemSelected(subfilterListItem) } @@ -486,7 +466,6 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView updateTagsAndSites.observe( viewLifecycleOwner ) { event: Event> -> - Log.d("ReaderFragment", "updateTagsAndSites.observe") event.applyIfNotHandled { if (NetworkUtils.isNetworkAvailable(activity)) { ReaderUpdateServiceStarter.startService(activity, this) @@ -524,7 +503,6 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView bottomSheetUiState.observe( viewLifecycleOwner ) { event: Event -> - Log.d("ReaderFragment", "bottomSheetUiState.observe") event.applyIfNotHandled { val fm = childFragmentManager var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? @@ -546,7 +524,6 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView bottomSheetAction.observe( viewLifecycleOwner ) { event: Event -> - Log.d("ReaderFragment", "bottomSheetAction.observe") event.applyIfNotHandled { when (this) { is OpenSubsAtPage -> { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index a586c810e68f..2f8164966098 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -38,6 +38,7 @@ import org.wordpress.android.util.UrlUtils import org.wordpress.android.viewmodel.Event import org.wordpress.android.viewmodel.ScopedViewModel import org.wordpress.android.viewmodel.SingleLiveEvent +import java.util.Comparator import java.util.EnumSet import javax.inject.Inject import javax.inject.Named @@ -69,9 +70,6 @@ class SubFilterViewModel @Inject constructor( private val _updateTagsAndSites = MutableLiveData>>() val updateTagsAndSites: LiveData>> = _updateTagsAndSites - private val _onCleared = MutableLiveData>() - val onCleared: LiveData> = _onCleared - private val _isTitleContainerVisible = MutableLiveData(true) val isTitleContainerVisible: LiveData = _isTitleContainerVisible @@ -141,12 +139,12 @@ class SubFilterViewModel @Inject constructor( blog.organizationId == organization.orgId } ?: false } - }.sortedWith { blog1, blog2 -> + }.sortedWith(Comparator { blog1, blog2 -> // sort followed blogs by name/domain to match display val blogOneName = getBlogNameForComparison(blog1) val blogTwoName = getBlogNameForComparison(blog2) blogOneName.compareTo(blogTwoName, true) - } + }) filterList.addAll( followedBlogs.map { blog -> @@ -416,7 +414,7 @@ class SubFilterViewModel @Inject constructor( loadSubFilters() } - @Suppress("unused") + @Suppress("unused", "UNUSED_PARAMETER") @Subscribe(threadMode = ThreadMode.MAIN) fun onEventMainThread(event: ReaderEvents.FollowedBlogsFetched) { if(event.didChange()) { @@ -428,7 +426,6 @@ class SubFilterViewModel @Inject constructor( override fun onCleared() { super.onCleared() eventBusWrapper.unregister(this) - _onCleared.value = Event(Unit) } companion object { From 88b734768e3d5fac4a862c463321401eaf04d508 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 20 May 2024 09:49:37 -0300 Subject: [PATCH 4/7] Initialize subfilter observers as fields to keep unique reference This makes sure that the Lifecycle and ViewModel Owners handle everything correctly when observing LiveData, which avoids duplicate observers and allows for correct auto removal/re-observation due to lifecycle and owner state change. --- .../android/ui/reader/ReaderFragment.kt | 180 ++++++++++-------- .../ui/reader/subfilter/SubFilterViewModel.kt | 16 +- 2 files changed, 108 insertions(+), 88 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 474ef518d722..ec1427374780 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 @@ -13,6 +13,7 @@ import androidx.compose.runtime.livedata.observeAsState import androidx.compose.ui.platform.ViewCompositionStrategy import androidx.core.view.isVisible import androidx.fragment.app.Fragment +import androidx.lifecycle.Observer import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.ViewModelStoreOwner import androidx.recyclerview.widget.RecyclerView @@ -96,6 +97,86 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView private var readerSubsActivityResultLauncher: ActivityResultLauncher? = null + private val wpMainActivityViewModel by lazy { + ViewModelProvider( + requireActivity(), + viewModelFactory + )[WPMainActivityViewModel::class.java] + } + + // region SubgroupFilterViewModel Observers + // we need a reference to the observers so they are properly handled by the lifecycle and ViewModel owners, avoiding + // duplication, and ensuring they are properly removed when the Fragment is destroyed + private val currentSubfilterObserver = Observer { subfilterListItem -> + viewModel.onSubFilterItemSelected(subfilterListItem) + } + + private val updateTagsAndSitesObserver = Observer>> { event -> + event.applyIfNotHandled { + if (NetworkUtils.isNetworkAvailable(activity)) { + ReaderUpdateServiceStarter.startService(activity, this) + } + } + } + + private val subFiltersObserver = Observer> { subFilters -> + val selectedTag = (viewModel.uiState.value as? ContentUiState)?.selectedReaderTag ?: return@Observer + viewModel.showTopBarFilterGroup( + selectedTag, + subFilters + ) + } + + private val bottomSheetUiStateObserver = Observer> { event -> + event.applyIfNotHandled { + val selectedTag = (viewModel.uiState.value as? ContentUiState)?.selectedReaderTag + ?: return@applyIfNotHandled + val viewModelKey = SubFilterViewModel.getViewModelKeyForTag(selectedTag) + + val fm = childFragmentManager + var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? + if (isVisible && bottomSheet == null) { + val (title, category) = this as BottomSheetVisible + bottomSheet = newInstance( + viewModelKey, + category, + uiHelpers.getTextOfUiString(requireContext(), title) + ) + bottomSheet.show(childFragmentManager, SUBFILTER_BOTTOM_SHEET_TAG) + } else if (!isVisible && bottomSheet != null) { + bottomSheet.dismiss() + } + } + } + + private val bottomSheetActionObserver = Observer> { event -> + event.applyIfNotHandled { + when (this) { + is OpenSubsAtPage -> { + readerSubsActivityResultLauncher?.launch( + ReaderActivityLauncher.createIntentShowReaderSubs( + requireActivity(), + tabIndex + ) + ) + } + + is OpenLoginPage -> { + wpMainActivityViewModel.onOpenLoginPage() + } + + is OpenSearchPage -> { + ReaderActivityLauncher.showReaderSearch(requireActivity()) + } + + is OpenSuggestedTagsPage -> { + ReaderActivityLauncher.showReaderInterests(requireActivity()) + } + } + } + } + // endregion + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { binding = ReaderFragmentLayoutBinding.bind(view).apply { initTopAppBar() @@ -453,104 +534,41 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView } private fun SubFilterViewModel.initSubFilterViewModel(startedTag: ReaderTag, savedInstanceState: Bundle?) { - setupSubFilterBottomSheetObservers(startedTag) + bottomSheetUiState.observe( + viewLifecycleOwner, + bottomSheetUiStateObserver + ) + + bottomSheetAction.observe( + viewLifecycleOwner, + bottomSheetActionObserver + ) currentSubFilter.observe( - viewLifecycleOwner - ) { subfilterListItem: SubfilterListItem -> - onSubfilterSelected(subfilterListItem) - viewModel.onSubFilterItemSelected(subfilterListItem) - } + viewLifecycleOwner, + currentSubfilterObserver + ) updateTagsAndSites.observe( - viewLifecycleOwner - ) { event: Event> -> - event.applyIfNotHandled { - if (NetworkUtils.isNetworkAvailable(activity)) { - ReaderUpdateServiceStarter.startService(activity, this) - } - } - } + viewLifecycleOwner, + updateTagsAndSitesObserver + ) if (startedTag.isFilterable) { subFilters.observe( - viewLifecycleOwner - ) { subFilters: List -> - viewModel.showTopBarFilterGroup( - startedTag, - subFilters - ) - } + viewLifecycleOwner, + subFiltersObserver + ) updateTagsAndSites() } else { viewModel.hideTopBarFilterGroup(startedTag) } - // thomashortadev: not sure if always passing the same tags can cause problems start(startedTag, startedTag, savedInstanceState) } - private fun SubFilterViewModel.setupSubFilterBottomSheetObservers(startedTag: ReaderTag) { - val wpMainActivityViewModel = ViewModelProvider( - requireActivity(), - viewModelFactory - )[WPMainActivityViewModel::class.java] - - val viewModelKey = SubFilterViewModel.getViewModelKeyForTag(startedTag) - - bottomSheetUiState.observe( - viewLifecycleOwner - ) { event: Event -> - event.applyIfNotHandled { - val fm = childFragmentManager - var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? - if (isVisible && bottomSheet == null) { - loadSubFilters() - val (title, category) = this as BottomSheetVisible - bottomSheet = newInstance( - viewModelKey, - category, - uiHelpers.getTextOfUiString(requireContext(), title) - ) - bottomSheet.show(childFragmentManager, SUBFILTER_BOTTOM_SHEET_TAG) - } else if (!isVisible && bottomSheet != null) { - bottomSheet.dismiss() - } - } - } - - bottomSheetAction.observe( - viewLifecycleOwner - ) { event: Event -> - event.applyIfNotHandled { - when (this) { - is OpenSubsAtPage -> { - readerSubsActivityResultLauncher?.launch( - ReaderActivityLauncher.createIntentShowReaderSubs( - requireActivity(), - tabIndex - ) - ) - } - - is OpenLoginPage -> { - wpMainActivityViewModel.onOpenLoginPage() - } - - is OpenSearchPage -> { - ReaderActivityLauncher.showReaderSearch(requireActivity()) - } - - is OpenSuggestedTagsPage -> { - ReaderActivityLauncher.showReaderInterests(requireActivity()) - } - } - } - } - } - companion object { private const val SUBFILTER_BOTTOM_SHEET_TAG = "SUBFILTER_BOTTOM_SHEET_TAG" } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index 2f8164966098..c197234dad0e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -38,7 +38,6 @@ import org.wordpress.android.util.UrlUtils import org.wordpress.android.viewmodel.Event import org.wordpress.android.viewmodel.ScopedViewModel import org.wordpress.android.viewmodel.SingleLiveEvent -import java.util.Comparator import java.util.EnumSet import javax.inject.Inject import javax.inject.Named @@ -125,6 +124,7 @@ class SubFilterViewModel @Inject constructor( "" } } + fun loadSubFilters() { launch { val filterList = ArrayList() @@ -139,12 +139,12 @@ class SubFilterViewModel @Inject constructor( blog.organizationId == organization.orgId } ?: false } - }.sortedWith(Comparator { blog1, blog2 -> + }.sortedWith { blog1, blog2 -> // sort followed blogs by name/domain to match display val blogOneName = getBlogNameForComparison(blog1) val blogTwoName = getBlogNameForComparison(blog2) blogOneName.compareTo(blogTwoName, true) - }) + } filterList.addAll( followedBlogs.map { blog -> @@ -239,13 +239,14 @@ class SubFilterViewModel @Inject constructor( category: SubfilterCategory, ) { updateTagsAndSites() + loadSubFilters() _bottomSheetUiState.value = Event( BottomSheetVisible( UiStringRes(category.titleRes), category ) ) - val source = when(category) { + val source = when (category) { SubfilterCategory.SITES -> "blogs" SubfilterCategory.TAGS -> "tags" } @@ -353,6 +354,7 @@ class SubFilterViewModel @Inject constructor( } else { readerTracker.stop(ReaderTrackerType.SUBFILTERED_LIST) } + onSubfilterSelected(filter) _currentSubFilter.value = filter } @@ -414,10 +416,10 @@ class SubFilterViewModel @Inject constructor( loadSubFilters() } - @Suppress("unused", "UNUSED_PARAMETER") + @Suppress("unused") @Subscribe(threadMode = ThreadMode.MAIN) fun onEventMainThread(event: ReaderEvents.FollowedBlogsFetched) { - if(event.didChange()) { + if (event.didChange()) { AppLog.d(T.READER, "Subfilter bottom sheet > followed blogs changed") loadSubFilters() } @@ -449,7 +451,7 @@ class SubFilterViewModel @Inject constructor( companion object { fun fromSubfilterListItem(subfilterListItem: SubfilterListItem): FilterItemType? = - when(subfilterListItem.type) { + when (subfilterListItem.type) { SubfilterListItem.ItemType.SITE -> Blog SubfilterListItem.ItemType.TAG -> Tag else -> null From 456283f98c9ab51bc9e8c4d867e070e89250eb72 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 20 May 2024 11:38:33 -0300 Subject: [PATCH 5/7] Update unit tests --- .../ui/reader/subfilter/SubFilterViewModel.kt | 14 ++-- .../subfilter/SubFilterViewModelTest.kt | 69 ++++++++++++++++--- 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index c197234dad0e..8f41c16c70e7 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -9,8 +9,8 @@ import kotlinx.coroutines.withContext import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import org.wordpress.android.analytics.AnalyticsTracker.Stat -import org.wordpress.android.datasets.ReaderBlogTable -import org.wordpress.android.datasets.ReaderTagTable +import org.wordpress.android.datasets.ReaderBlogTableWrapper +import org.wordpress.android.datasets.wrappers.ReaderTagTableWrapper import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.models.ReaderBlog import org.wordpress.android.models.ReaderTag @@ -49,7 +49,9 @@ class SubFilterViewModel @Inject constructor( private val subfilterListItemMapper: SubfilterListItemMapper, private val eventBusWrapper: EventBusWrapper, private val accountStore: AccountStore, - private val readerTracker: ReaderTracker + private val readerTracker: ReaderTracker, + private val readerTagTableWrapper: ReaderTagTableWrapper, + private val readerBlogTableWrapper: ReaderBlogTableWrapper, ) : ScopedViewModel(bgDispatcher) { private val _subFilters = MutableLiveData>() val subFilters: LiveData> = _subFilters @@ -132,7 +134,7 @@ class SubFilterViewModel @Inject constructor( if (accountStore.hasAccessToken()) { val organization = mTagFragmentStartedWith?.organization - val followedBlogs = ReaderBlogTable.getFollowedBlogs().let { blogList -> + val followedBlogs = readerBlogTableWrapper.getFollowedBlogs().let { blogList -> // Filtering out all blogs not belonging to this VM organization if valid blogList.filter { blog -> organization?.let { @@ -157,7 +159,7 @@ class SubFilterViewModel @Inject constructor( ) } - val tags = ReaderTagTable.getFollowedTags() + val tags = readerTagTableWrapper.getFollowedTags() for (tag in tags) { filterList.add( @@ -354,8 +356,8 @@ class SubFilterViewModel @Inject constructor( } else { readerTracker.stop(ReaderTrackerType.SUBFILTERED_LIST) } - onSubfilterSelected(filter) _currentSubFilter.value = filter + onSubfilterSelected(filter) } fun onUserComesToReader() { diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt index 7c8f2807e5f6..c2a70cc8c21f 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt @@ -17,17 +17,20 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.analytics.AnalyticsTracker +import org.wordpress.android.datasets.ReaderBlogTableWrapper +import org.wordpress.android.datasets.wrappers.ReaderTagTableWrapper import org.wordpress.android.fluxc.model.AccountModel import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.getOrAwaitValue import org.wordpress.android.models.ReaderBlog import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagList import org.wordpress.android.models.ReaderTagType.BOOKMARKED import org.wordpress.android.models.ReaderTagType.TAGS +import org.wordpress.android.ui.Organization import org.wordpress.android.ui.prefs.AppPrefsWrapper import org.wordpress.android.ui.reader.ReaderSubsActivity import org.wordpress.android.ui.reader.ReaderTypes.ReaderPostListType -import org.wordpress.android.ui.reader.services.update.ReaderUpdateLogic import org.wordpress.android.ui.reader.services.update.ReaderUpdateLogic.UpdateTask import org.wordpress.android.ui.reader.subfilter.ActionType.OpenLoginPage import org.wordpress.android.ui.reader.subfilter.ActionType.OpenSubsAtPage @@ -78,7 +81,10 @@ class SubFilterViewModelTest : BaseUnitTest() { private lateinit var savedState: Bundle @Mock - private lateinit var filter: SubfilterListItem + private lateinit var readerTagTableWrapper: ReaderTagTableWrapper + + @Mock + private lateinit var readerBlogTableWrapper: ReaderBlogTableWrapper private lateinit var viewModel: SubFilterViewModel @@ -93,7 +99,9 @@ class SubFilterViewModelTest : BaseUnitTest() { subfilterListItemMapper, eventBusWrapper, accountStore, - readerTracker + readerTracker, + readerTagTableWrapper, + readerBlogTableWrapper, ) viewModel.start(initialTag, savedTag, savedState) @@ -102,6 +110,10 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `current subfilter is set back when we have a previous intance state`() { val json = "{\"blogId\":0,\"feedId\":0,\"tagSlug\":\"news\",\"tagType\":1,\"type\":4}" + val filter = Site( + blog = ReaderBlog(), + onClickAction = mock(), + ) whenever(savedState.getString(SubFilterViewModel.ARG_CURRENT_SUBFILTER_JSON)).thenReturn(json) whenever(subfilterListItemMapper.fromJson(eq(json), any(), any())).thenReturn(filter) @@ -113,7 +125,9 @@ class SubFilterViewModelTest : BaseUnitTest() { subfilterListItemMapper, eventBusWrapper, accountStore, - readerTracker + readerTracker, + readerTagTableWrapper, + readerBlogTableWrapper, ) viewModel.start(initialTag, savedTag, savedState) @@ -288,11 +302,9 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `view model updates the tags and sites and asks to show the bottom sheet when filters button is tapped`() { - var updateTasks: EnumSet? = null - var uiState: BottomSheetUiState? = null + var updateTasks: EnumSet? = null viewModel.updateTagsAndSites.observeForever { updateTasks = it.peekContent() } - viewModel.bottomSheetUiState.observeForever { uiState = it.peekContent() } viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) @@ -302,10 +314,47 @@ class SubFilterViewModelTest : BaseUnitTest() { UpdateTask.FOLLOWED_BLOGS ) ) + } + + @Test + fun `view model asks to show the bottom sheet when filters button is tapped`() { + var uiState: BottomSheetUiState? = null + + viewModel.bottomSheetUiState.observeForever { uiState = it.peekContent() } + + viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) assertThat(uiState).isInstanceOf(BottomSheetVisible::class.java) } + @Test + fun `view model updates subfilters when filters button is tapped`() = test { + whenever(initialTag.organization).thenReturn(Organization.NO_ORGANIZATION) + whenever(accountStore.hasAccessToken()).thenReturn(true) + whenever(readerTagTableWrapper.getFollowedTags()).thenReturn( + ReaderTagList().apply { + add(ReaderTag("a", "a", "a", "endpoint-a", TAGS)) + add(ReaderTag("b", "b", "b", "endpoint-b", TAGS)) + add(ReaderTag("c", "c", "c", "endpoint-c", TAGS)) + } + ) + + whenever(readerBlogTableWrapper.getFollowedBlogs()).thenReturn( + List(2) { + ReaderBlog().apply { organizationId = Organization.NO_ORGANIZATION.orgId } + } + ) + + var subFilters: List? = null + + viewModel.subFilters.observeForever { subFilters = it } + + viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) + advanceUntilIdle() + + assertThat(subFilters).hasSize(5) + } + @Test fun `view model hides the bottom sheet when it is cancelled`() { var uiState: BottomSheetUiState? = null @@ -320,7 +369,11 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `bottom sheet is hidden when a filter is tapped on`() { var uiState: BottomSheetUiState? = null - val filter: SubfilterListItem = mock() + val filter: SubfilterListItem = Site( + isSelected = false, + onClickAction = mock(), + blog = ReaderBlog(), + ) viewModel.setSubfilterFromTag(savedTag) From ff3f56105b043033b5c851f56a811d32b4d13ebe Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 20 May 2024 15:01:47 -0300 Subject: [PATCH 6/7] Remove unnecessary coroutine in tests --- .../android/ui/reader/subfilter/SubFilterViewModelTest.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt index c2a70cc8c21f..78ce1e841766 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt @@ -328,7 +328,7 @@ class SubFilterViewModelTest : BaseUnitTest() { } @Test - fun `view model updates subfilters when filters button is tapped`() = test { + fun `view model updates subfilters when filters button is tapped`() { whenever(initialTag.organization).thenReturn(Organization.NO_ORGANIZATION) whenever(accountStore.hasAccessToken()).thenReturn(true) whenever(readerTagTableWrapper.getFollowedTags()).thenReturn( @@ -350,7 +350,6 @@ class SubFilterViewModelTest : BaseUnitTest() { viewModel.subFilters.observeForever { subFilters = it } viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) - advanceUntilIdle() assertThat(subFilters).hasSize(5) } From 12e9ef704540160162af759d5c2bd1f64bbcedd1 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 20 May 2024 16:00:01 -0300 Subject: [PATCH 7/7] Fix unit tests --- .../ui/reader/subfilter/SubFilterViewModelTest.kt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt index 78ce1e841766..23e3a40f8b30 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt @@ -302,6 +302,8 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `view model updates the tags and sites and asks to show the bottom sheet when filters button is tapped`() { + mockReaderTableEmpty() + var updateTasks: EnumSet? = null viewModel.updateTagsAndSites.observeForever { updateTasks = it.peekContent() } @@ -318,6 +320,8 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `view model asks to show the bottom sheet when filters button is tapped`() { + mockReaderTableEmpty() + var uiState: BottomSheetUiState? = null viewModel.bottomSheetUiState.observeForever { uiState = it.peekContent() } @@ -539,4 +543,11 @@ class SubFilterViewModelTest : BaseUnitTest() { assertThat(viewModel.isTitleContainerVisible.getOrAwaitValue()).isEqualTo(isTitleContainerVisible) } } + + private fun mockReaderTableEmpty() { + whenever(initialTag.organization).thenReturn(Organization.NO_ORGANIZATION) + whenever(accountStore.hasAccessToken()).thenReturn(true) + whenever(readerTagTableWrapper.getFollowedTags()).thenReturn(ReaderTagList()) + whenever(readerBlogTableWrapper.getFollowedBlogs()).thenReturn(emptyList()) + } }