diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailAdapter.kt b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailAdapter.kt index ccc1761540..dbb664b827 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailAdapter.kt +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailAdapter.kt @@ -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) { diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt index 1ae954293f..066156f982 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt @@ -9,6 +9,8 @@ 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 @@ -16,7 +18,7 @@ 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 @@ -202,6 +204,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C private val categoryNames: ArrayList = ArrayList() + private val fragmentHandler = Handler(Looper.getMainLooper()) /** * Depicts is a feature part of Structured data. @@ -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 @@ -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( @@ -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) @@ -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() } @@ -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) { @@ -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 @@ -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) @@ -713,6 +697,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C object : BaseControllerListener() { override fun onIntermediateImageSet(id: String, imageInfo: ImageInfo?) { imageInfoCache = imageInfo + if (_binding == null) return updateAspectRatio(binding.mediaDetailScrollView.width) } @@ -722,6 +707,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C animatable: Animatable? ) { imageInfoCache = imageInfo + if (_binding == null) return updateAspectRatio(binding.mediaDetailScrollView.width) } } @@ -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() } diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.kt b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.kt index d1e1d2aad1..9e234c95ba 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.kt +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.kt @@ -74,6 +74,7 @@ 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 @@ -81,7 +82,7 @@ class MediaDetailPagerFragment : CommonsDaggerSupportFragment(), OnPageChangeLis /** * ProgressBar used to indicate the loading status of media items. */ - var imageProgressBar: ProgressBar? = null + var removedItems: ArrayList = ArrayList() @@ -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 @@ -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) } @@ -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 } /** @@ -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 -> { @@ -398,7 +400,7 @@ ${m.pageTitle.canonicalUri}""" setAvatarFromImageUrl( requireActivity(), media.imageUrl!!, sessionManager!!.currentAccount!!.name, - okHttpJsonApiClient!!, Companion.compositeDisposable + okHttpJsonApiClient!!, compositeDisposables ) } @@ -406,12 +408,13 @@ ${m.pageTitle.canonicalUri}""" 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 } val m = provider.getMediaAtPosition(position) @@ -497,29 +500,33 @@ ${m.pageTitle.canonicalUri}""" * @param menu */ private fun handleBackgroundColorMenuItems(getBitmap: Callable, 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) } } @@ -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) @@ -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 diff --git a/app/src/test/kotlin/fr/free/nrw/commons/media/MediaDetailFragmentUnitTests.kt b/app/src/test/kotlin/fr/free/nrw/commons/media/MediaDetailFragmentUnitTests.kt index 6159a3ccf0..91555bc496 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/media/MediaDetailFragmentUnitTests.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/media/MediaDetailFragmentUnitTests.kt @@ -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 @@ -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() }