Skip to content

fix : leaks at Media Detail Fragment #6705

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

Open
rovertrack wants to merge 2 commits into
base: main
Choose a base branch
from
+82 −88
Open

fix : leaks at Media Detail Fragment #6705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81ebfc9
fix : MediaDetail and ViewPager Leaks
rovertrack Mar 9, 2026
e50c6c3
Merge branch 'main' into leakcanary
RitikaPahwa4444 Mar 19, 2026
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
7 changes: 5 additions & 2 deletions app/src/main/java/fr/free/nrw/commons/media/MediaDetailAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ class MediaDetailAdapter(
Timber.d("Skipping getItem. Returning as activity is destroyed!")
return Fragment()
}
mediaDetailPagerFragment.binding!!.mediaDetailsPager.postDelayed(
{ mediaDetailPagerFragment.requireActivity().invalidateOptionsMenu() }, 5
mediaDetailPagerFragment.binding?.mediaDetailsPager?.postDelayed(
{
if (mediaDetailPagerFragment.activity != null)
mediaDetailPagerFragment.requireActivity().invalidateOptionsMenu()
}, 5
)
}
return if (mediaDetailPagerFragment.isFromFeaturedRootFragment) {
Expand Down
89 changes: 37 additions & 52 deletions app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import android.content.res.Configuration
import android.graphics.drawable.Animatable
import android.net.Uri
import android.os.Bundle
import android.os.Handler
import android.os.Looper
import android.text.Editable
import android.text.TextUtils
import android.text.TextWatcher
import android.view.KeyEvent
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.view.ViewTreeObserver.OnGlobalLayoutListener
import androidx.core.view.doOnNextLayout
import android.widget.ArrayAdapter
import android.widget.Button
import android.widget.EditText
Expand Down Expand Up @@ -202,6 +204,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C


private val categoryNames: ArrayList<String> = ArrayList()
private val fragmentHandler = Handler(Looper.getMainLooper())

/**
* Depicts is a feature part of Structured data.
Expand All @@ -213,7 +216,6 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
private var oldWidthOfImageView: Int = 0
private var newWidthOfImageView: Int = 0
private var heightVerifyingBoolean: Boolean = true // helps in maintaining aspect ratio
private var layoutListener: OnGlobalLayoutListener? = null // for layout stuff, only used once!

//Had to make this class variable, to implement various onClicks, which access the media,
// also I fell why make separate variables when one can serve the purpose
Expand Down Expand Up @@ -248,7 +250,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C

private val scrollPosition: Unit
get() {
initialListTop = binding.mediaDetailScrollView.scrollY
initialListTop = _binding?.mediaDetailScrollView?.scrollY ?: initialListTop
}

override fun onCreateView(
Expand Down Expand Up @@ -317,7 +319,6 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
)

_binding = FragmentMediaDetailBinding.inflate(inflater, container, false)
val view: View = binding.root

binding.seeMore.setUnderlinedText(R.string.nominated_see_more)

Expand All @@ -336,7 +337,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
binding.coordinateEdit.visibility = View.GONE
}

handleBackEvent(view)
handleBackEvent(_binding?.root!!)

//set onCLick listeners
binding.mediaDetailLicense.setOnClickListener { onMediaDetailLicenceClicked() }
Expand Down Expand Up @@ -403,17 +404,21 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
* Gets the height of the frame layout as soon as the view is ready and updates aspect ratio
* of the picture.
*/
view.post{
fragmentHandler.post {
if (_binding == null) return@post
val width = binding.mediaDetailScrollView.width
if (width > 0) {
frameLayoutHeight = binding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(width)
} else {
view.postDelayed({ updateAspectRatio(binding.root.width) }, 1)
fragmentHandler.postDelayed({
if (_binding == null) return@postDelayed
updateAspectRatio(binding.root.width)
}, 1)
}
}

return view
return _binding?.root!!
}

fun launchZoomActivity(view: View) {
Expand Down Expand Up @@ -493,7 +498,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C

override fun onResume() {
super.onResume()

if (_binding == null) return
val contributionsFragment: ContributionsFragment? = this.getContributionsFragmentParent()
if (contributionsFragment?.binding != null) {
contributionsFragment.binding!!.cardViewNearby.visibility = View.GONE
Expand Down Expand Up @@ -524,51 +529,30 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
binding.sendThanks.visibility = View.VISIBLE
}

binding.mediaDetailScrollView.viewTreeObserver.addOnGlobalLayoutListener(
object : OnGlobalLayoutListener {
override fun onGlobalLayout() {
if (context == null) {
return
}
binding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
this
)
oldWidthOfImageView = binding.mediaDetailScrollView.width
if (media != null) {
displayMediaDetails()
fetchFileUsages(media?.filename!!)
}
}
binding.mediaDetailScrollView.doOnNextLayout { view ->
if (_binding == null) return@doOnNextLayout
oldWidthOfImageView = view.width
if (media != null) {
displayMediaDetails()
fetchFileUsages(media?.filename!!)
}
)
}
binding.progressBarEdit.visibility = View.GONE
binding.descriptionEdit.visibility = View.VISIBLE
}

override fun onConfigurationChanged(newConfig: Configuration) {
super.onConfigurationChanged(newConfig)
binding.mediaDetailScrollView.viewTreeObserver.addOnGlobalLayoutListener(
object : OnGlobalLayoutListener {
override fun onGlobalLayout() {
/**
* We update the height of the frame layout as the configuration changes.
*/
binding.mediaDetailFrameLayout.post {
frameLayoutHeight = binding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(binding.mediaDetailScrollView.width)
}
if (binding.mediaDetailScrollView.width != oldWidthOfImageView) {
if (newWidthOfImageView == 0) {
newWidthOfImageView = binding.mediaDetailScrollView.width
updateAspectRatio(newWidthOfImageView)
}
binding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
this
)
}
}
if (_binding == null) return
binding.mediaDetailScrollView.doOnNextLayout { view ->
if (_binding == null) return@doOnNextLayout
frameLayoutHeight = binding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(view.width)
if (view.width != oldWidthOfImageView && newWidthOfImageView == 0) {
newWidthOfImageView = view.width
updateAspectRatio(newWidthOfImageView)
}
)
}
// Ensuring correct aspect ratio for landscape mode
if (heightVerifyingBoolean) {
updateAspectRatio(newWidthOfImageView)
Expand Down Expand Up @@ -713,6 +697,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
object : BaseControllerListener<ImageInfo?>() {
override fun onIntermediateImageSet(id: String, imageInfo: ImageInfo?) {
imageInfoCache = imageInfo
if (_binding == null) return
updateAspectRatio(binding.mediaDetailScrollView.width)
}

Expand All @@ -722,6 +707,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
animatable: Animatable?
) {
imageInfoCache = imageInfo
if (_binding == null) return
updateAspectRatio(binding.mediaDetailScrollView.width)
}
}
Expand Down Expand Up @@ -786,13 +772,12 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
}

override fun onDestroyView() {
if (layoutListener != null && view != null) {
requireView().viewTreeObserver.removeGlobalOnLayoutListener(layoutListener) // old Android was on crack. CRACK IS WHACK
layoutListener = null
}

fragmentHandler.removeCallbacksAndMessages(null)
compositeDisposable.clear()

_binding?.root?.clearFocus()
_binding?.root?.setOnKeyListener(null)
binding.mediaDetailImageView.controller = null
_binding = null
super.onDestroyView()
}

Expand Down
70 changes: 39 additions & 31 deletions app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,15 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis
var isWikipediaButtonDisplayed: Boolean = false
var adapter: MediaDetailAdapter? = null
var bookmark: Bookmark? = null
val compositeDisposables = CompositeDisposable()
var mediaDetailProvider: MediaDetailProvider? = null
var isFromFeaturedRootFragment: Boolean = false
var position: Int = 0

/**
* ProgressBar used to indicate the loading status of media items.
*/
var imageProgressBar: ProgressBar? = null


var removedItems: ArrayList<Int> = ArrayList()

Expand All @@ -94,8 +95,7 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis
): View? {
binding = FragmentMediaDetailPagerBinding.inflate(inflater, container, false)
binding!!.mediaDetailsPager.addOnPageChangeListener(this)
// Initialize the ProgressBar by finding it in the layout
imageProgressBar = binding!!.root.findViewById(R.id.itemProgressBar)

adapter = MediaDetailAdapter(this, childFragmentManager)

// ActionBar is now supported in both activities - if this crashes something is quite wrong
Expand Down Expand Up @@ -125,7 +125,7 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis

override fun onSaveInstanceState(outState: Bundle) {
super.onSaveInstanceState(outState)
outState.putInt("current-page", binding!!.mediaDetailsPager.currentItem)
outState.putInt("current-page", binding?.mediaDetailsPager?.currentItem ?: 0)
outState.putBoolean("editable", editable)
outState.putBoolean("isFeaturedImage", isFeaturedImage)
}
Expand All @@ -141,13 +141,14 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis
}

override fun onDestroyView() {
binding = null
adapter = null
super.onDestroyView()
compositeDisposables.clear()
if (activity is MainActivity) {
(activity as MainActivity).showTabs()
}

// Temporarily disable it. Ref:https://github.com/commons-app/apps-android-commons/issues/6581#issuecomment-3694210567
// binding = null
}

/**
Expand All @@ -170,7 +171,8 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis
return true
}

val m = mediaDetailProvider!!.getMediaAtPosition(binding!!.mediaDetailsPager.currentItem)
val pager = binding?.mediaDetailsPager ?: return true
val m = mediaDetailProvider!!.getMediaAtPosition(pager.currentItem)
val mediaDetailFragment = adapter!!.currentMediaDetailFragment
when (item.itemId) {
R.id.menu_bookmark_current_image -> {
Expand Down Expand Up @@ -398,20 +400,21 @@ ${m.pageTitle.canonicalUri}"""
setAvatarFromImageUrl(
requireActivity(), media.imageUrl!!,
sessionManager!!.currentAccount!!.name,
okHttpJsonApiClient!!, Companion.compositeDisposable
okHttpJsonApiClient!!, compositeDisposables
)
}

override fun onCreateOptionsMenu(menu: Menu, inflater: MenuInflater) {
if (!editable) { // Disable menu options for editable views
menu.clear() // see http://stackoverflow.com/a/8495697/17865
inflater.inflate(R.menu.fragment_image_detail, menu)
if (binding!!.mediaDetailsPager != null) {
val pager = binding?.mediaDetailsPager
if (pager != null) {
val provider = mediaDetailProvider ?: return
val position = if (isFromFeaturedRootFragment) {
position
} else {
binding!!.mediaDetailsPager.currentItem
pager.currentItem
Copy link
Collaborator

@RitikaPahwa4444 RitikaPahwa4444 Mar 9, 2026

Choose a reason for hiding this comment

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

Sorry, I haven't read the entire code, but just to confirm once, this line addresses the issue I mentioned in this PR: #6624, is it? 🙂

We'll test it a bit intensively as we've seen regressions in at least two PRs. Thanks for your patience.

Copy link
Contributor Author

@rovertrack rovertrack Mar 9, 2026

Choose a reason for hiding this comment

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

mostly yes , because many cases it tries to access that pager after destroyed which results in ANR ,null pointer

sure , anything as long as it solves the issue )

}

val m = provider.getMediaAtPosition(position)
Expand Down Expand Up @@ -497,29 +500,33 @@ ${m.pageTitle.canonicalUri}"""
* @param menu
*/
private fun handleBackgroundColorMenuItems(getBitmap: Callable<Bitmap>, menu: Menu) {
Observable.fromCallable(
getBitmap
).subscribeOn(Schedulers.newThread())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(Consumer { image: Bitmap ->
if (image.hasAlpha()) {
menu.findItem(R.id.menu_view_set_white_background).setVisible(true)
.setEnabled(true)
menu.findItem(R.id.menu_view_set_black_background).setVisible(true)
.setEnabled(true)
}
})
compositeDisposables.add(
Observable.fromCallable(
getBitmap
).subscribeOn(Schedulers.newThread())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(Consumer { image: Bitmap ->
if (image.hasAlpha()) {
menu.findItem(R.id.menu_view_set_white_background).setVisible(true)
.setEnabled(true)
menu.findItem(R.id.menu_view_set_black_background).setVisible(true)
.setEnabled(true)
}
})
)
}

private fun updateBookmarkState(item: MenuItem) {
val isBookmarked = bookmarkDao!!.findBookmark(bookmark)
if (isBookmarked) {
if (removedItems.contains(binding!!.mediaDetailsPager.currentItem)) {
removedItems.remove(binding!!.mediaDetailsPager.currentItem)
val currentItem = binding?.mediaDetailsPager?.currentItem ?: return
if (removedItems.contains(currentItem)) {
removedItems.remove(currentItem)
}
} else {
if (!removedItems.contains(binding!!.mediaDetailsPager.currentItem)) {
removedItems.add(binding!!.mediaDetailsPager.currentItem)
val currentItem = binding?.mediaDetailsPager?.currentItem ?: return
if (!removedItems.contains(currentItem)) {
removedItems.add(currentItem)
}
}

Expand Down Expand Up @@ -547,14 +554,16 @@ ${m.pageTitle.canonicalUri}"""
val handler = Handler(Looper.getMainLooper())
val runnable: Runnable = object : Runnable {
override fun run() {
val b = binding ?: return
val a = adapter ?: return
// Show the ProgressBar while waiting for the item to load
imageProgressBar!!.visibility = View.VISIBLE
b.itemProgressBar.visibility = View.VISIBLE
// Check if the adapter has enough items loaded
if (adapter!!.count > position) {
if (a.count > position) {
// Set the current item in the ViewPager
binding!!.mediaDetailsPager.setCurrentItem(position, false)
b.mediaDetailsPager.setCurrentItem(position, false)
// Hide the ProgressBar once the item is loaded
imageProgressBar!!.visibility = View.GONE
b.itemProgressBar.visibility = View.GONE
} else {
// If the item is not ready yet, post the Runnable again
handler.post(this)
Expand Down Expand Up @@ -605,7 +614,6 @@ ${m.pageTitle.canonicalUri}"""
}

companion object {
private val compositeDisposable = CompositeDisposable()

/**
* Use this factory method to create a new instance of this fragment using the provided
Expand Down
4 changes: 1 addition & 3 deletions app/src/test/kotlin/fr/free/nrw/commons/media/MediaDetailFragmentUnitTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import android.os.Looper
import android.view.LayoutInflater
import android.view.View
import android.view.View.GONE
import android.view.ViewTreeObserver

import android.webkit.WebView
import android.widget.Button
import android.widget.LinearLayout
Expand Down Expand Up @@ -312,8 +312,6 @@ class MediaDetailFragmentUnitTests {
@Test
@Throws(Exception::class)
fun testOnDestroyView() {
val layoutListener = mock(ViewTreeObserver.OnGlobalLayoutListener::class.java)
Whitebox.setInternalState(fragment, "layoutListener", layoutListener)
fragment.onDestroyView()
}

Expand Down
Loading