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.

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

Leaking fragments? #3251

Closed
1 task done
nikclayton opened this issue Feb 2, 2023 · 2 comments · Fixed by #3255
Closed
1 task done

Leaking fragments? #3251

nikclayton opened this issue Feb 2, 2023 · 2 comments · Fixed by #3255

Comments

@nikclayton
Copy link
Contributor

I think the code might be leaking fragments in at least MainActivity.

To reproduce, modify TimelineFragment to log on every call to onResume, onPause, onStop, and onDestroy.

With that done, do the following:

  1. Restart the app
  2. Open the nav drawer, choose "Account preferences"
  3. Press the back button

You should see the following logged

onResume()  // <-- from initially opening the app
onPause()   // <-- when "Account preferences" is opened
onStop()    // <-- ditto
onResume()  // <-- when pressing "Back" out of "Account preferences"

That's as expected.

Now do this:

  1. Restart the app
  2. Open the nav drawer, choose "Account preferences"
  3. Open "Tabs"
  4. Modify the tab list (either add or remove a tab)
  5. Press the back button twice to go back to MainActivity and the timeline.

I see the following:

onResume()
onPause()
onStop()
onResume()
onResume()  // <-- the first four are the same, what's this extra onResume doing here?

I discovered this while working on #3121 -- if you do the above steps the "Refresh" menu item is added multiple times. But I've reproduced it on develop.

I suspect that the code in MainActivity.setupTabs() is not disposing of the previous fragments when the list of tabs change, and at least one of them is being retained. I haven't got a fix for it yet though.


  • Tusky Version: develop at 6249dba (that commit isn't the culprit, it's the version I had checked out)

  • Android Version: API 30

  • Android Device: Pixel 4 emulator

  • I searched or browsed the repo’s other issues to ensure this is not a duplicate.

@nikclayton
Copy link
Contributor Author

tl;dr: This is something to do with how setupTabs() recreates the adapter and tab layout every time it's called.

Additional debugging (chiefly, including the kind and hashcode of the fragment in the onResume debug output) confirmed that this really was two different versions of the HOME fragment resuming, the initial one, and then a new one).

Things which didn't work to fix this include (all of these produced code that still worked, in as much as the tabs showed with the correct contents and behaved correctly, but all of them still have the "two fragments" problem):

  • Thinking that this might be the FragmentStateAdapter not using the correct lifecycle, rewrote CustomFragmentStateAdapter(activity) to CustomFragmentStateAdapater(FragmentManager, Lifecycle), passing in this@MainActivity and this.lifecycle.

  • Saving tabLayoutMediator in a property, and explicitly calling tabLayoutMediator?.detach() in setupTabs() before assigning the adapter to binding.viewPager.adapter

  • ViewPager2 is supposed to be at the same level in the view hierarchy as TabLayout, and immediately after it. It's not (e.g., see https://developer.android.com/develop/ui/views/animations/vp2-migration#tablayout which says "with ViewPager2, the TabLayout element is declared directly above the ViewPager2 element, on the same level"). Rewriting the layout had no effect (and this would also break the bottom tab bar, which is another issue to consider).

  • Making the adapter contain a mutable list, creating it with an empty list, assigning it to the viewpager, and then setting the list in the adapter and calling notifyDataSetChanged()

  • MainPagerAdapter's list is a List. Tried changing to a List, and doing the conversion from TabData to Fragment in MainActivity (MainPagerAdapter(tabs.map { it.fragment(it.arguments) }, this).

What did work is moving:

  • The creation of the adapter (which now has a mutable list of tabs)
  • Adding the adapter to binding.viewPager
  • Attaching the tablayoutmediator to the viewpager

to onCreate. Then setupTabs gets the list of tabs from the account, assigns them to adapter.tabs, and calls adapter.notifyDataSetChanged().

Should have a more refined fix later.

@nikclayton
Copy link
Contributor Author

I think this is also why modifying unrelated tabs (e.g., adding or removing the DM tab) jumps you back to the top of your home timeline when you "back" out of "Account preferences > tabs",

connyduck pushed a commit that referenced this issue Feb 15, 2023
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 #3251
connyduck pushed a commit that referenced this issue Mar 1, 2023
* Add "Refresh" accessibility menu to TimelineFragment

Per https://developer.android.com/reference/androidx/swiperefreshlayout/widget/SwipeRefreshLayout
the layout does not provide accessibility events, and a menu item should be
provided as an alternative method for refreshing the content.

In `TimelineFragment`:
- Implement the `MenuProvider` interface so it can populate the action bar
  menu in activities that host the fragment
- Create a "Refresh" menu item, and refresh the state when it is selected

`MainActivity` has to change how the menu is created, so that fragments
can add items to it.

In `MainActivity`:
- Call `setSupportActionBar` so `mainToolbar` participates in menus
- Implement the `MenuProvider` interface, and move menu creation there
- Set the title via supportActionBar

* Never show the refresh item as a menubar action

Per guidelines in https://developer.android.com/develop/ui/views/touch-and-input/swipe/add-swipe-interface#AddRefreshAction

* Add "Refresh" menu item for AccountMediaFragment

Also, fix the colour of the refresh progress indicator

* Implement "Refresh" for AnnouncementsActivity

* Add "Refresh" menu for ConversationsFragment

* Keep the tabs adapter over the life of the viewpager

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 #3251

* Add "Refresh" menu for ScheduledStatusActivity

* Lint

* Add "Refresh" menu for SearchFragment / SearchActivity

* Explicitly set the searchview width

Using "collapseActionView" requires the user to press "Back" twice to exit
the activity, which is not acceptable.

* Move toolbar handling in to ViewThreadActivity

Previous code had the toolbar in the fragment's layout. Refactor to make
consistent with other activities, and move the toolbar in to the activity
layout.

Implement MenuProvider in ViewThreadFragment to adjust the menu in the
activity.

* Add "Refresh" menu to ViewThreadFragment

* Implement "Refresh" for ViewEditsFragment

* Lint

* Add "Refresh" menu to ReportStatusesFragment

* Add "Refresh" menu to NotificationsFragment

* Rename menu resource files

Be consistent with the layout resource files, which have an activity/fragment
prefix, then the lower_snake_case name of the activity or fragment it's for.

* Only enable refresh menu if swiptorefresh is enabled

Some timelines don't have swipetorefresh enabled (e.g., those shown on
AccountActivity). In those cases don't add the refresh menu, rely on the
hosting activity to provide it.

Update AccountActivity to provide the refresh menu item.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant