fix: resolve IndexOutOfBounds error when removing images from top card (#6124)

replace deprecated onBackPressed with onBackPressedCallback

remove unit test for deprecated onBackPressed method

remove if-check before deleting picture to prevent hiding top thumbnail card

hide the thumbnail card on fragments other than MediaDetailFragment

Co-authored-by: Nicolas Raoul <nicolas.raoul@gmail.com>

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.kt

@@ -14,6 +14,7 @@ import android.os.Bundle
 import android.provider.Settings
 import android.view.View
 import android.widget.CheckBox
+import androidx.activity.OnBackPressedCallback
 import androidx.appcompat.app.AlertDialog
 import androidx.fragment.app.Fragment
 import androidx.fragment.app.FragmentManager
@@ -122,7 +123,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     /**
      * Set the value of the showPermissionDialog variable.
      *
-     * @param showPermissionsDialog `true` to indicate to show
+     * @property isShowPermissionsDialog `true` to indicate to show
      * Permissions Dialog if permissions are missing, `false` otherwise.
      */
     /**
@@ -166,6 +167,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     private var _binding: ActivityUploadBinding? = null
     private val binding: ActivityUploadBinding get() = _binding!!
 
+    private lateinit var onBackPressedCallback: OnBackPressedCallback
+
     @SuppressLint("CheckResult")
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
@@ -173,6 +176,23 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         _binding = ActivityUploadBinding.inflate(layoutInflater)
         setContentView(binding.root)
 
+        // Overrides the back button to make sure the user is prepared to lose their progress
+        onBackPressedCallback = object : OnBackPressedCallback(true) {
+            override fun handleOnBackPressed() {
+                showAlertDialog(
+                    this@UploadActivity,
+                    getString(R.string.back_button_warning),
+                    getString(R.string.back_button_warning_desc),
+                    getString(R.string.back_button_continue),
+                    getString(R.string.back_button_warning),
+                    null
+                ) {
+                    finish()
+                }
+            }
+        }
+        onBackPressedDispatcher.addCallback(this, onBackPressedCallback)
+
         /*
          If Configuration of device is changed then get the new fragments
          created by the system and populate the fragments ArrayList
@@ -187,7 +207,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         }
 
         init()
-        binding.rlContainerTitle.setOnClickListener { v: View? -> onRlContainerTitleClicked() }
+        binding.rlContainerTitle.setOnClickListener { _: View? -> onRlContainerTitleClicked() }
         nearbyPopupAnswers = mutableMapOf()
         //getting the current dpi of the device and if it is less than 320dp i.e. overlapping
         //threshold, thumbnails automatically minimizes
@@ -201,7 +221,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         }
         locationManager!!.requestLocationUpdatesFromProvider(LocationManager.GPS_PROVIDER)
         locationManager!!.requestLocationUpdatesFromProvider(LocationManager.NETWORK_PROVIDER)
-        store = BasicKvStore(this, storeNameForCurrentUploadImagesSize).apply {
+        store = BasicKvStore(this, STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE).apply {
             clearAll()
         }
         checkStoragePermissions()
@@ -241,7 +261,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
 
             override fun onPageSelected(position: Int) {
                 currentSelectedPosition = position
-                if (position >= uploadableFiles!!.size) {
+                if (position >= uploadableFiles.size) {
                     binding.cvContainerTopCard.visibility = View.GONE
                 } else {
                     thumbnailsAdapter!!.notifyDataSetChanged()
@@ -274,7 +294,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                 .subscribeOn(Schedulers.io())
                 .observeOn(AndroidSchedulers.mainThread())
                 .filter { result: Boolean? -> result!! }
-                .subscribe { result: Boolean? ->
+                .subscribe { _: Boolean? ->
                     showAlertDialog(
                         this,
                         getString(R.string.block_notification_title),
@@ -284,7 +304,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                 })
     }
 
-    fun checkStoragePermissions() {
+    private fun checkStoragePermissions() {
         // Check if all required permissions are granted
         val hasAllPermissions = hasPermission(this, PERMISSIONS_STORAGE)
         val hasPartialAccess = hasPartialAccess(this)
@@ -355,7 +375,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         showLongToast(this, messageResourceId)
     }
 
-    override fun getUploadableFiles(): List<UploadableFile>? {
+    override fun getUploadableFiles(): List<UploadableFile> {
         return uploadableFiles
     }
 
@@ -367,6 +387,14 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     override fun onUploadMediaDeleted(index: Int) {
         fragments!!.removeAt(index) //Remove the corresponding fragment
         uploadableFiles.removeAt(index) //Remove the files from the list
+
+        val isMediaDetailFragment = fragments!!.getOrNull(currentSelectedPosition)?.let {
+            it is UploadMediaDetailFragment
+        } ?: false
+        if(!isMediaDetailFragment) {
+            // Should hide the top card current fragment is not the media detail fragment
+            showHideTopCard(false)
+        }
         thumbnailsAdapter!!.notifyItemRemoved(index) //Notify the thumbnails adapter
         uploadImagesAdapter!!.notifyDataSetChanged() //Notify the ViewPager
     }
@@ -375,8 +403,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         binding.tvTopCardTitle.text = resources
             .getQuantityString(
                 R.plurals.upload_count_title,
-                uploadableFiles!!.size,
+                uploadableFiles.size,
-                uploadableFiles!!.size
+                uploadableFiles.size
             )
     }
 
@@ -444,15 +472,16 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
             receiveInternalSharedItems()
         }
 
-        if (uploadableFiles == null || uploadableFiles!!.isEmpty()) {
+        if (uploadableFiles.isEmpty()) {
             handleNullMedia()
         } else {
             //Show thumbnails
-            if (uploadableFiles!!.size > 1) {
+            if (uploadableFiles.size > 1) {
-                if (!defaultKvStore.getBoolean("hasAlreadyLaunchedCategoriesDialog")) { //If there is only file, no need to show the image thumbnails
+                if (!defaultKvStore.getBoolean("hasAlreadyLaunchedCategoriesDialog")) {
+                    // If there is only file, no need to show the image thumbnails
                     showAlertDialogForCategories()
                 }
-                if (uploadableFiles!!.size > 3 &&
+                if (uploadableFiles.size > 3 &&
                     !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")
                 ) {
                     showAlertForBattery()
@@ -464,8 +493,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
             binding.tvTopCardTitle.text = resources
                 .getQuantityString(
                     R.plurals.upload_count_title,
-                    uploadableFiles!!.size,
+                    uploadableFiles.size,
-                    uploadableFiles!!.size
+                    uploadableFiles.size
                 )
 
 
@@ -474,7 +503,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
             }
 
 
-            for (uploadableFile in uploadableFiles!!) {
+            for (uploadableFile in uploadableFiles) {
                 val uploadMediaDetailFragment = UploadMediaDetailFragment()
 
                 if (!uploadIsOfAPlace) {
@@ -497,8 +526,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                     object : UploadMediaDetailFragmentCallback {
                         override fun deletePictureAtIndex(index: Int) {
                             store!!.putInt(
-                                keyForCurrentUploadImagesSize,
+                                KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE,
-                                (store!!.getInt(keyForCurrentUploadImagesSize) - 1)
+                                (store!!.getInt(KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE) - 1)
                             )
                             presenter!!.deletePictureAtIndex(index)
                         }
@@ -576,11 +605,11 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                 fragments!!.add(mediaLicenseFragment!!)
             } else {
                 for (i in 1 until fragments!!.size) {
-                    fragments!![i]!!.callback = object : UploadBaseFragment.Callback {
+                    fragments!![i].callback = object : UploadBaseFragment.Callback {
                         override fun onNextButtonClicked(index: Int) {
                             if (index < fragments!!.size - 1) {
                                 binding.vpUpload.setCurrentItem(index + 1, false)
-                                fragments!![index + 1]!!.onBecameVisible()
+                                fragments!![index + 1].onBecameVisible()
                                 (binding.rvThumbnails.layoutManager as LinearLayoutManager)
                                     .scrollToPositionWithOffset(
                                         if ((index > 0)) index - 1 else 0,
@@ -594,7 +623,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                         override fun onPreviousButtonClicked(index: Int) {
                             if (index != 0) {
                                 binding.vpUpload.setCurrentItem(index - 1, true)
-                                fragments!![index - 1]!!.onBecameVisible()
+                                fragments!![index - 1].onBecameVisible()
                                 (binding.rvThumbnails.layoutManager as LinearLayoutManager)
                                     .scrollToPositionWithOffset(
                                         if ((index > 3)) index - 2 else 0,
@@ -632,11 +661,12 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
             binding.vpUpload.offscreenPageLimit = fragments!!.size
         }
         // Saving size of uploadableFiles
-        store!!.putInt(keyForCurrentUploadImagesSize, uploadableFiles!!.size)
+        store!!.putInt(KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE, uploadableFiles.size)
     }
 
     /**
-     * Changes current image when one image upload is cancelled, to highlight next image in the top thumbnail.
+     * Changes current image when one image upload is cancelled, to highlight next image in the top
+     * thumbnail.
      * Fixes: [Issue](https://github.com/commons-app/apps-android-commons/issues/5511)
      *
      * @param index Index of image to be removed
@@ -771,7 +801,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     override fun onNextButtonClicked(index: Int) {
         if (index < fragments!!.size - 1) {
             binding.vpUpload.setCurrentItem(index + 1, false)
-            fragments!![index + 1]!!.onBecameVisible()
+            fragments!![index + 1].onBecameVisible()
             (binding.rvThumbnails.layoutManager as LinearLayoutManager)
                 .scrollToPositionWithOffset(if ((index > 0)) index - 1 else 0, 0)
             if (index < fragments!!.size - 4) {
@@ -786,10 +816,10 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     override fun onPreviousButtonClicked(index: Int) {
         if (index != 0) {
             binding.vpUpload.setCurrentItem(index - 1, true)
-            fragments!![index - 1]!!.onBecameVisible()
+            fragments!![index - 1].onBecameVisible()
             (binding.rvThumbnails.layoutManager as LinearLayoutManager)
                 .scrollToPositionWithOffset(if ((index > 3)) index - 2 else 0, 0)
-            if ((index != 1) && ((index - 1) < uploadableFiles!!.size)) {
+            if ((index != 1) && ((index - 1) < uploadableFiles.size)) {
                 // Shows the top card if it was hidden because of the last image being deleted and
                 // now the user has hit previous button to go back to the media details
                 showHideTopCard(true)
@@ -797,7 +827,10 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         }
     }
 
-    override fun onThumbnailDeleted(position: Int) = presenter!!.deletePictureAtIndex(position)
+    override fun onThumbnailDeleted(position: Int) {
+        presenter!!.deletePictureAtIndex(position)
+        thumbnailsAdapter?.notifyDataSetChanged()
+    }
 
     /**
      * The adapter used to show image upload intermediate fragments
@@ -824,11 +857,11 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
     }
 
 
-    fun onRlContainerTitleClicked() {
+    private fun onRlContainerTitleClicked() {
         binding.rvThumbnails.visibility =
             if (isTitleExpanded) View.GONE else View.VISIBLE
         isTitleExpanded = !isTitleExpanded
-        binding.ibToggleTopCard.rotation = binding.ibToggleTopCard.rotation + 180
+        binding.ibToggleTopCard.rotation += 180
     }
 
     override fun onDestroy() {
@@ -845,21 +878,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         if (uploadCategoriesFragment != null) {
             uploadCategoriesFragment!!.callback = null
         }
-    }
+        onBackPressedCallback.remove()
-
-    /**
-     * Overrides the back button to make sure the user is prepared to lose their progress
-     */
-    @SuppressLint("MissingSuperCall")
-    override fun onBackPressed() {
-        showAlertDialog(
-            this,
-            getString(R.string.back_button_warning),
-            getString(R.string.back_button_warning_desc),
-            getString(R.string.back_button_continue),
-            getString(R.string.back_button_warning),
-            null
-        ) { finish() }
     }
 
     /**
@@ -879,7 +898,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
             .setView(view)
             .setTitle(getString(R.string.multiple_files_depiction_header))
             .setMessage(getString(R.string.multiple_files_depiction))
-            .setPositiveButton("OK") { dialog: DialogInterface?, which: Int ->
+            .setPositiveButton("OK") { _: DialogInterface?, _: Int ->
                 if (checkBox.isChecked) {
                     // Save the user's choice to not show the dialog again
                     defaultKvStore.putBoolean("hasAlreadyLaunchedCategoriesDialog", true)
@@ -913,14 +932,14 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                 getString(R.string.cancel),
                 {
                     /* Since opening the right settings page might be device dependent, using
-                                              https://github.com/WaseemSabir/BatteryPermissionHelper
+                      https://github.com/WaseemSabir/BatteryPermissionHelper
-                                              directly appeared like a promising idea.
+                      directly appeared like a promising idea.
-                                              However, this simply closed the popup and did not make
+                      However, this simply closed the popup and did not make
-                                              the settings page appear on a Pixel as well as a Xiaomi device.
+                      the settings page appear on a Pixel as well as a Xiaomi device.
-                                              Used the standard intent instead of using this library as
+                      Used the standard intent instead of using this library as
-                                              it shows a list of all the apps on the device and allows users to
+                      it shows a list of all the apps on the device and allows users to
-                                              turn battery optimisation off.
+                      turn battery optimisation off.
-                                            */
+                    */
                     val batteryOptimisationSettingsIntent = Intent(
                         Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS
                     )
@@ -958,7 +977,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
                        Also, location information is discarded if the difference between
                        current location and location recorded just before capturing the image
                        is greater than 100 meters */
-            if (isLocationTagUnchecked || locationDifference > 100 || !defaultKvStore.getBoolean("inAppCameraLocationPref")
+            if (isLocationTagUnchecked || locationDifference > 100
+                || !defaultKvStore.getBoolean("inAppCameraLocationPref")
                 || !isInAppCameraUpload
             ) {
                 currLocation = null
@@ -979,8 +999,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
         @JvmField
         var nearbyPopupAnswers: MutableMap<Place, Boolean>? = null
 
-        const val keyForCurrentUploadImagesSize: String = "CurrentUploadImagesSize"
+        const val KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE: String = "CurrentUploadImagesSize"
-        const val storeNameForCurrentUploadImagesSize: String = "CurrentUploadImageQualities"
+        const val STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE: String = "CurrentUploadImageQualities"
 
         /**
          * Sets the flag indicating whether the upload is of a specific place.

app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.kt

@@ -146,34 +146,31 @@ class UploadPresenter @Inject internal constructor(
 
     override fun deletePictureAtIndex(index: Int) {
         val uploadableFiles = view.getUploadableFiles()
-        if (index == uploadableFiles!!.size - 1) {
+        uploadableFiles?.let {
-            // If the next fragment to be shown is not one of the MediaDetailsFragment
+            view.setImageCancelled(true)
-            // lets hide the top card so that it doesn't appear on the other fragments
+            repository.deletePicture(uploadableFiles[index].getFilePath())
-            view.showHideTopCard(false)
+            if (uploadableFiles.size == 1) {
-        }
+                view.showMessage(R.string.upload_cancelled)
-        view.setImageCancelled(true)
+                view.finish()
-        repository.deletePicture(uploadableFiles[index].getFilePath())
+                return
-        if (uploadableFiles.size == 1) {
-            view.showMessage(R.string.upload_cancelled)
-            view.finish()
-            return
-        }
-
-        presenter.updateImageQualitiesJSON(uploadableFiles.size, index)
-        view.onUploadMediaDeleted(index)
-        if (index != uploadableFiles.size && index != 0) {
-            // if the deleted image was not the last item to be uploaded, check quality of next
-            repository.getUploadItem(index)?.let {
-                presenter.checkImageQuality(it, index)
             }
-        }
 
-        if (uploadableFiles.size < 2) {
+            presenter.updateImageQualitiesJSON(uploadableFiles.size, index)
-            view.showHideTopCard(false)
+            view.onUploadMediaDeleted(index)
-        }
+            if (index != uploadableFiles.size && index != 0) {
+                // if the deleted image was not the last item to be uploaded, check quality of next
+                repository.getUploadItem(index)?.let {
+                    presenter.checkImageQuality(it, index)
+                }
+            }
 
-        //In case lets update the number of uploadable media
+            if (uploadableFiles.size < 2) {
-        view.updateTopCardTitle()
+                view.showHideTopCard(false)
+            }
+
+            //In case lets update the number of uploadable media
+            view.updateTopCardTitle()
+        }
     }
 
     override fun onAttachView(view: UploadContract.View) {

app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.kt

@@ -532,7 +532,7 @@ class UploadMediaDetailFragment : UploadBaseFragment(), UploadMediaDetailsContra
                         basicKvStore!!.putBoolean(keyForShowingAlertDialog, false)
                         if (isInternetConnectionEstablished(requireActivity())) {
                             val sizeOfUploads = basicKvStore!!.getInt(
-                                UploadActivity.keyForCurrentUploadImagesSize
+                                UploadActivity.KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE
                             )
                             for (i in indexOfFragment until sizeOfUploads) {
                                 presenter.getImageQuality(

app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.kt

@@ -310,7 +310,7 @@ class UploadMediaPresenter @Inject constructor(
     private fun storeImageQuality(
         imageResult: Int, uploadItemIndex: Int, activity: Activity, uploadItem: UploadItem
     ) {
-        val store = BasicKvStore(activity, UploadActivity.storeNameForCurrentUploadImagesSize)
+        val store = BasicKvStore(activity, UploadActivity.STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE)
         val value = store.getString(UPLOAD_QUALITIES_KEY, null)
         try {
             val jsonObject = value.asJsonObject().apply {
@@ -339,8 +339,10 @@ class UploadMediaPresenter @Inject constructor(
      */
     override fun checkImageQuality(uploadItem: UploadItem, index: Int) {
         if ((uploadItem.imageQuality != IMAGE_OK) && (uploadItem.imageQuality != IMAGE_KEEP)) {
-            val value = basicKvStoreFactory?.let { it(UploadActivity.storeNameForCurrentUploadImagesSize) }
+          
+            val value = basicKvStoreFactory?.let { it(UploadActivity.STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE) }
                 ?.getString(UPLOAD_QUALITIES_KEY, null)
+                
             try {
                 val imageQuality = value.asJsonObject()["UploadItem$index"] as Int
                 view.showProgress(false)
@@ -363,8 +365,9 @@ class UploadMediaPresenter @Inject constructor(
      * @param index Index of the UploadItem which was deleted
      */
     override fun updateImageQualitiesJSON(size: Int, index: Int) {
-        val value = basicKvStoreFactory?.let { it(UploadActivity.storeNameForCurrentUploadImagesSize) }
+        val value = basicKvStoreFactory?.let { it(UploadActivity.STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE) }
             ?.getString(UPLOAD_QUALITIES_KEY, null)
+
         try {
             val jsonObject = value.asJsonObject().apply {
                 for (i in index until (size - 1)) {
@@ -372,7 +375,8 @@ class UploadMediaPresenter @Inject constructor(
                 }
                 remove("UploadItem" + (size - 1))
             }
-            basicKvStoreFactory?.let { it(UploadActivity.storeNameForCurrentUploadImagesSize) }
+
+            basicKvStoreFactory?.let { it(UploadActivity.STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE) }
                 ?.putString(UPLOAD_QUALITIES_KEY, jsonObject.toString())
         } catch (e: Exception) {
             Timber.e(e)

app/src/test/kotlin/fr/free/nrw/commons/upload/UploadActivityUnitTests.kt

@@ -262,15 +262,4 @@ class UploadActivityUnitTests {
         method.isAccessible = true
         method.invoke(activity)
     }
-
-    @Test
-    @Throws(Exception::class)
-    fun testOnBackPressed() {
-        val method: Method =
-            UploadActivity::class.java.getDeclaredMethod(
-                "onBackPressed",
-            )
-        method.isAccessible = true
-        method.invoke(activity)
-    }
 }