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.

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

Fix: IndexOutOfBoundsException crash when removing last two images of multiupload #6124

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+76 −74
Open

Fix: IndexOutOfBoundsException crash when removing last two images of multiupload #6124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
50096cc
fix IndexOutOfBounds error when removing thumbnails
rohit9625 Jan 14, 2025
a182741
refactor: resolve compiler warnings
rohit9625 Jan 14, 2025
43eb577
refactor: replace deprecated onBackPressed with onBackPressedCallback
rohit9625 Jan 14, 2025
bff3bd8
test: remove unit test as no longer needed
rohit9625 Jan 14, 2025
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
129 changes: 71 additions & 58 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
*/
/**
Expand Down Expand Up @@ -166,13 +167,32 @@ 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)

_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
Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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),
Expand All @@ -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)
Expand Down Expand Up @@ -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
}

Expand All @@ -375,8 +395,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
)
}

Expand Down Expand Up @@ -444,15 +464,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 (!defaultKvStore.getBoolean("hasAlreadyLaunchedCategoriesDialog")) { //If there is only file, no need to show the image thumbnails
if (uploadableFiles.size > 1) {
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()
Expand All @@ -464,8 +485,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
)


Expand All @@ -474,7 +495,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
}


for (uploadableFile in uploadableFiles!!) {
for (uploadableFile in uploadableFiles) {
val uploadMediaDetailFragment = UploadMediaDetailFragment()

if (!uploadIsOfAPlace) {
Expand All @@ -497,8 +518,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
object : UploadMediaDetailFragmentCallback {
override fun deletePictureAtIndex(index: Int) {
store!!.putInt(
keyForCurrentUploadImagesSize,
(store!!.getInt(keyForCurrentUploadImagesSize) - 1)
KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE,
(store!!.getInt(KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE) - 1)
)
presenter!!.deletePictureAtIndex(index)
}
Expand Down Expand Up @@ -576,11 +597,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,
Expand All @@ -594,7 +615,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,
Expand Down Expand Up @@ -632,11 +653,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
Expand Down Expand Up @@ -690,10 +712,10 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
uploadableFiles = mutableListOf<UploadableFile>().apply {
addAll(intent.getParcelableArrayListExtra(EXTRA_FILES) ?: emptyList())
}
isMultipleFilesSelected = uploadableFiles!!.size > 1
Timber.i("Received multiple upload %s", uploadableFiles!!.size)
isMultipleFilesSelected = uploadableFiles.size > 1
Timber.i("Received multiple upload %s", uploadableFiles.size)

place = intent.getParcelableExtra<Place>(PLACE_OBJECT)
place = intent.getParcelableExtra(PLACE_OBJECT)
prevLocation = intent.getParcelableExtra(LOCATION_BEFORE_IMAGE_CAPTURE)
isInAppCameraUpload = intent.getBooleanExtra(IN_APP_CAMERA_UPLOAD, false)
resetDirectPrefs()
Expand Down Expand Up @@ -724,7 +746,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) {
Expand All @@ -739,18 +761,21 @@ 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)
}
}
}

override fun onThumbnailDeleted(position: Int) = presenter!!.deletePictureAtIndex(position)
override fun onThumbnailDeleted(position: Int) {
presenter!!.deletePictureAtIndex(position)
thumbnailsAdapter?.notifyItemRemoved(position)
}

/**
* The adapter used to show image upload intermediate fragments
Expand All @@ -777,11 +802,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() {
Expand All @@ -798,20 +823,7 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
if (uploadCategoriesFragment != null) {
uploadCategoriesFragment!!.callback = null
}
}

/**
* Overrides the back button to make sure the user is prepared to lose their progress
*/
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() }
onBackPressedCallback.remove()
}

/**
Expand All @@ -831,7 +843,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)
Expand Down Expand Up @@ -865,14 +877,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
directly appeared like a promising idea.
However, this simply closed the popup and did not make
the settings page appear on a Pixel as well as a Xiaomi device.
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
turn battery optimisation off.
*/
https://github.com/WaseemSabir/BatteryPermissionHelper
directly appeared like a promising idea.
However, this simply closed the popup and did not make
the settings page appear on a Pixel as well as a Xiaomi device.
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
turn battery optimisation off.
*/
val batteryOptimisationSettingsIntent = Intent(
Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS
)
Expand Down Expand Up @@ -910,7 +922,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
Expand All @@ -931,8 +944,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
@JvmField
var nearbyPopupAnswers: MutableMap<Place, Boolean>? = null

const val keyForCurrentUploadImagesSize: String = "CurrentUploadImagesSize"
const val storeNameForCurrentUploadImagesSize: String = "CurrentUploadImageQualities"
const val KEY_FOR_CURRENT_UPLOAD_IMAGE_SIZE: String = "CurrentUploadImagesSize"
const val STORE_NAME_FOR_CURRENT_UPLOAD_IMAGE_SIZE: String = "CurrentUploadImageQualities"

/**
* Sets the flag indicating whether the upload is of a specific place.
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,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(
Expand Down
Loading
Loading