From 196ebdb386fd17482e3be517006da01657a494a1 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Wed, 15 Feb 2023 19:17:59 +0100 Subject: [PATCH] Keep the tabs adapter over the life of the viewpager (#3255) Make `tabs` `var` instead of `val` in `MainPagerAdapter` so it can be updated when tabs change. Then detach the `tabLayoutMediator`, update the tabs, and call `notifyItemRangeChanged` in `setupTabs()`. This fixes a bug (not sure if it's this code, or in ViewPager2) where assigning a new adapter to the view pager seemed to result in a leak of one or more fragments. This wasn't user-visible, but it's a leak, and it becomes user-visible when fragments want to display menus. This also fixes two other bugs: 1. Be on the left-most tab. Scroll down a bit. Then modify the tabs at "Account preferences > tabs", but keep the left-most tab as-is. Then go back to MainActivity. Your reading position in the left-most tab has been jumped to the top. 2. Be on any non-left-most tab. Then modify the tab list by reordering tabs (adding/removing tabs is also OK). Then go back to MainActivity. Your tab selection has been overridden, and the left-most tab has been selected. Because the fragments are not destroyed unnecessarily your reading position is retained. And it remembers the tab you had selected, and as long as that tab is still present you will be returned to it, even if it's changed position in the list. Fixes https://github.com/tuskyapp/Tusky/issues/3251 --- .../com/keylesspalace/tusky/MainActivity.kt | 69 ++++++++++++------- .../tusky/pager/MainPagerAdapter.kt | 2 +- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/MainActivity.kt b/app/src/main/java/com/keylesspalace/tusky/MainActivity.kt index e8595746..fdf7dfc4 100644 --- a/app/src/main/java/com/keylesspalace/tusky/MainActivity.kt +++ b/app/src/main/java/com/keylesspalace/tusky/MainActivity.kt @@ -34,6 +34,7 @@ import android.view.View import android.widget.ImageView import androidx.activity.OnBackPressedCallback import androidx.appcompat.app.AlertDialog +import androidx.appcompat.content.res.AppCompatResources import androidx.coordinatorlayout.widget.CoordinatorLayout import androidx.core.app.ActivityCompat import androidx.core.content.ContextCompat @@ -170,6 +171,12 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje // We need to know if the emoji pack has been changed private var selectedEmojiPack: String? = null + /** Mediate between binding.viewPager and the chosen tab layout */ + private var tabLayoutMediator: TabLayoutMediator? = null + + /** Adapter for the different timeline tabs */ + private lateinit var tabAdapter: MainPagerAdapter + override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -275,6 +282,13 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje fetchAnnouncements() + // Initialise the tab adapter and set to viewpager. Fragments appear to be leaked if the + // adapter changes over the life of the viewPager (the adapter, not its contents), so set + // the initial list of tabs to empty, and set the full list later in setupTabs(). See + // https://github.com/tuskyapp/Tusky/issues/3251 for details. + tabAdapter = MainPagerAdapter(emptyList(), this) + binding.viewPager.adapter = tabAdapter + setupTabs(showNotificationTab) eventHub.events @@ -655,7 +669,7 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje } private fun setupTabs(selectNotificationTab: Boolean) { - val activeTabLayout = if (preferences.getString("mainNavPosition", "top") == "bottom") { + val activeTabLayout = if (preferences.getString(PrefKeys.MAIN_NAV_POSITION, "top") == "bottom") { val actionBarSize = getDimension(this, androidx.appcompat.R.attr.actionBarSize) val fabMargin = resources.getDimensionPixelSize(R.dimen.fabMargin) (binding.composeButton.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = actionBarSize + fabMargin @@ -668,29 +682,36 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje binding.tabLayout } + // Save the previous tab so it can be restored later + val previousTab = tabAdapter.tabs.getOrNull(binding.viewPager.currentItem) + val tabs = accountManager.activeAccount!!.tabPreferences - val adapter = MainPagerAdapter(tabs, this) - binding.viewPager.adapter = adapter - TabLayoutMediator(activeTabLayout, binding.viewPager) { _: TabLayout.Tab?, _: Int -> }.attach() - activeTabLayout.removeAllTabs() - for (i in tabs.indices) { - val tab = activeTabLayout.newTab() - .setIcon(tabs[i].icon) - if (tabs[i].id == LIST) { - tab.contentDescription = tabs[i].arguments[1] - } else { - tab.setContentDescription(tabs[i].text) - } - activeTabLayout.addTab(tab) + // Detach any existing mediator before changing tab contents and attaching a new mediator + tabLayoutMediator?.detach() - if (tabs[i].id == NOTIFICATIONS) { - notificationTabPosition = i - if (selectNotificationTab) { - tab.select() - } + tabAdapter.tabs = tabs + tabAdapter.notifyItemRangeChanged(0, tabs.size) + + tabLayoutMediator = TabLayoutMediator(activeTabLayout, binding.viewPager, true) { + tab: TabLayout.Tab, position: Int -> + tab.icon = AppCompatResources.getDrawable(this@MainActivity, tabs[position].icon) + tab.contentDescription = when (tabs[position].id) { + LIST -> tabs[position].arguments[position] + else -> getString(tabs[position].text) } - } + }.also { it.attach() } + + // Selected tab is either + // - Notification tab (if appropriate) + // - The previously selected tab (if it hasn't been removed) + // - Left-most tab + val position = if (selectNotificationTab) { + tabs.indexOfFirst { it.id == NOTIFICATIONS } + } else { + previousTab?.let { tabs.indexOfFirst { it == previousTab } } + }.takeIf { it != -1 } ?: 0 + binding.viewPager.setCurrentItem(position, false) val pageMargin = resources.getDimensionPixelSize(R.dimen.tab_page_margin) binding.viewPager.setPageTransformer(MarginPageTransformer(pageMargin)) @@ -710,18 +731,18 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje binding.mainToolbar.title = tabs[tab.position].title(this@MainActivity) - refreshComposeButtonState(adapter, tab.position) + refreshComposeButtonState(tabAdapter, tab.position) } override fun onTabUnselected(tab: TabLayout.Tab) {} override fun onTabReselected(tab: TabLayout.Tab) { - val fragment = adapter.getFragment(tab.position) + val fragment = tabAdapter.getFragment(tab.position) if (fragment is ReselectableFragment) { (fragment as ReselectableFragment).onReselect() } - refreshComposeButtonState(adapter, tab.position) + refreshComposeButtonState(tabAdapter, tab.position) } }.also { activeTabLayout.addOnTabSelectedListener(it) @@ -730,7 +751,7 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje val activeTabPosition = if (selectNotificationTab) notificationTabPosition else 0 binding.mainToolbar.title = tabs[activeTabPosition].title(this@MainActivity) binding.mainToolbar.setOnClickListener { - (adapter.getFragment(activeTabLayout.selectedTabPosition) as? ReselectableFragment)?.onReselect() + (tabAdapter.getFragment(activeTabLayout.selectedTabPosition) as? ReselectableFragment)?.onReselect() } updateProfiles() diff --git a/app/src/main/java/com/keylesspalace/tusky/pager/MainPagerAdapter.kt b/app/src/main/java/com/keylesspalace/tusky/pager/MainPagerAdapter.kt index 4fe92660..c635f929 100644 --- a/app/src/main/java/com/keylesspalace/tusky/pager/MainPagerAdapter.kt +++ b/app/src/main/java/com/keylesspalace/tusky/pager/MainPagerAdapter.kt @@ -20,7 +20,7 @@ import androidx.fragment.app.FragmentActivity import com.keylesspalace.tusky.TabData import com.keylesspalace.tusky.util.CustomFragmentStateAdapter -class MainPagerAdapter(val tabs: List, activity: FragmentActivity) : CustomFragmentStateAdapter(activity) { +class MainPagerAdapter(var tabs: List, activity: FragmentActivity) : CustomFragmentStateAdapter(activity) { override fun createFragment(position: Int): Fragment { val tab = tabs[position]